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 grounded skybox rotation #4752

Merged
merged 1 commit into from
Apr 15, 2024
Merged

fix grounded skybox rotation #4752

merged 1 commit into from
Apr 15, 2024

Conversation

elalish
Copy link
Collaborator

@elalish elalish commented Apr 15, 2024

Unlike the normal skybox, the grounded skybox was rotating with the model during auto-rotate, which is inconsistent and caused the background to be not in the same place as the applied environment lighting, since that was correctly not rotating.

This now means the model moves relative to the ground when panning or changing the turntable rotation. I don't totally love that, but it's the only way to be consistent with our existing API.

@elalish elalish self-assigned this Apr 15, 2024
@elalish elalish merged commit 69d3689 into master Apr 15, 2024
6 checks passed
@elalish elalish deleted the skyboxRotation branch April 15, 2024 22:40
@ArchieBattley
Copy link

I understand why this had to be done to keep consistency. However, in the future will the auto-rotate and pan functions be separated. eg. could there be an option to stick the model to the ground plane when panning? Or would it involve a rewrite of the current camera functionality.

@elalish
Copy link
Collaborator Author

elalish commented Apr 19, 2024

The real problem is that pan operates by affecting camera-target - which is what makes pan emulatable via JS. That's an important feature, but camera-target is about centering the object as well, which should certainly keep the skybox centered. I'm somewhat regretting now that these things are so linked, but this also only affects a pretty narrow use case. I think I'm going to leave it as is.

JL-Vidinoti pushed a commit to vidinoti/model-viewer that referenced this pull request Apr 22, 2024
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.

None yet

2 participants