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 orbit controls of ui.scene after changing the camera's up vector #3544

Merged
merged 3 commits into from
Sep 6, 2024

Conversation

voigta
Copy link
Contributor

@voigta voigta commented Aug 20, 2024

This PR solves the problem that scenes need to have their Z-Axis "upwards" in order to have working orbit controls. If one tries to manually move the camera with scene.move_camera(up_z=-1), the orbit controls are not updated and the mouse-interaction gets weird.

PS: Thanks for creating and sharing this awesome library! 💪

@falkoschindler falkoschindler self-requested a review August 20, 2024 11:20
Copy link
Contributor

@falkoschindler falkoschindler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the pull request, @voigta!

But if I'm not mistaken, you can already change the camera's "up" vector via the move_camera() method:

with ui.scene() as scene:
    scene.move_camera(up_x=1, up_y=0, up_z=0)
    scene.cylinder(0, 1)

Therefore I don't think this PR is necessary.

@falkoschindler falkoschindler added the question Further information is requested label Aug 29, 2024
@voigta
Copy link
Contributor Author

voigta commented Aug 29, 2024

Yes, you can already change the "up" vector via move_camera but the OrbitControls are not updated accordingly. It looks like OrbitControls doesn't allow for a change in "up" because it only uses the object.up property once here:

const quat = new Quaternion().setFromUnitVectors( object.up, new Vector3( 0, 1, 0 ) );

An example to show the problem I want to solve:

from nicegui import ui

ui.label("A regular scene with correct mouse interaction:")
with ui.scene() as scene_regular:
    scene_regular.cylinder(0, 1)

ui.label("An inverted scene with inverted mouse interaction:")
with ui.scene() as scene_inverted:
    scene_inverted.move_camera(up_z=-1)
    scene_inverted.cylinder(0, 1)

ui.run()

Another (maybe cleaner?) solution would be to create a new OrbitControls object in case camera.up is changed here:

.onUpdate((p) => {
this.camera.position.set(p[0], p[1], p[2]);
this.camera.up.set(p[3], p[4], p[5]); // NOTE: before calling lookAt
this.look_at.set(p[6], p[7], p[8]);
this.camera.lookAt(p[6], p[7], p[8]);
this.controls.target.set(p[6], p[7], p[8]);
})

@falkoschindler falkoschindler removed the question Further information is requested label Sep 6, 2024
Copy link
Contributor

@falkoschindler falkoschindler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! This solution seems reasonable. Let's merge!

@falkoschindler falkoschindler added the bug Something isn't working label Sep 6, 2024
@falkoschindler falkoschindler added this to the 2.2 milestone Sep 6, 2024
@falkoschindler falkoschindler changed the title Remove hardcoded z-up for scene orbit controls Fix orbit controls of ui.scene after changing the camera's up vector Sep 6, 2024
@falkoschindler falkoschindler merged commit d136bfa into zauberzeug:main Sep 6, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants