Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix sky shader at non-zero origins #26045

Closed
wants to merge 1 commit into from
Closed

fix sky shader at non-zero origins #26045

wants to merge 1 commit into from

Conversation

makc
Copy link
Contributor

@makc makc commented May 12, 2023

ok, so the sky "as is" only works when placed at 0,0,0

when you move it with the camera somewhere far, far away - you get this:

Снимок экрана (1122)

this change fixes it to what it looks like at 0,0,0 again:

Снимок экрана (1123)

personally, I would also replace BoxGeometry with SphereGeometry because the box edges are too visible, but that is separate issue (and you could maybe also address that in the shader instead).

@makc
Copy link
Contributor Author

makc commented May 12, 2023

you could maybe also address that in the shader instead

looking back at this, the shader takes a uniform sunPosition and interpolates it via vSunDirection for no reason - maybe that's why. I will test and add another commit if this is the case.

edit: no, that was not it. oh well, SphereGeometry it is then, maybe someone else will figure that out.

edit 2: figured it out, the sky had fixed vertical offset from the camera 😅 (app mistake)

@makc
Copy link
Contributor Author

makc commented May 13, 2023

@Mugen87 here go some fiddles to reproduce the issue:

current sky at 0,0,0
Screenshot 2023-05-13 at 19-45-39 Edit fiddle - JSFiddle - Code Playground

current sky at -4.2,0,0
Screenshot 2023-05-13 at 19-46-08 Edit fiddle - JSFiddle - Code Playground

fixed sky at -4.2,0,0
Screenshot 2023-05-13 at 19-48-35 Edit fiddle - JSFiddle - Code Playground

@makc
Copy link
Contributor Author

makc commented May 13, 2023

ha. I see it still moved a bit to the right 🤔 not sure why. imho still better than nothing, but up to you.

@Mugen87
Copy link
Collaborator

Mugen87 commented May 15, 2023

The problem is that this change also affects all users who do not translate skybox. For clarity:

PR: https://jsfiddle.net/sg0La6nc/
Prod: https://jsfiddle.net/evd671wy/

I fear the PR will lead to more confusion that it actually helps. Ideally, a fix for translated skyboxes does not affect current setups.

when you move it with the camera somewhere far, far away - you get this:

Do you mind explaining a bit the use case where this happens?

@makc
Copy link
Contributor Author

makc commented May 15, 2023

the use case where this happens?

that one is pretty obvious. if the player is allowed to move around with no limits, he will eventually come close to the sky box or even walk right through it. so what you do then? you move the skybox along with the player (camera).

@makc
Copy link
Contributor Author

makc commented May 15, 2023

I fear the PR will lead to more confusion that it actually helps.

Well, you probaably remember that I am quite lax with my PRs. No big deal, l will let someone else try to fix this when they run into it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants