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

#1741 Rolling camera and then interacting with the view provide incorrect interactions #1879

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

samoncrief
Copy link
Contributor

A fix for the following bug: #1741

Copy link

codecov bot commented Jan 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.69%. Comparing base (5f7a99c) to head (cc789cb).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1879      +/-   ##
==========================================
+ Coverage   95.68%   95.69%   +0.01%     
==========================================
  Files         125      125              
  Lines        9961     9988      +27     
==========================================
+ Hits         9531     9558      +27     
  Misses        430      430              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@mwestphal mwestphal left a comment

Choose a reason for hiding this comment

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

Nice! Please add a test. A simple interaction test should do the trick

Copy link
Member

@Meakk Meakk left a comment

Choose a reason for hiding this comment

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

I'm trying to understand the behavior with this PR. When the camera is rolled, the temporary up is used, but when do we restore the old up value? Only when we reset the camera?
I think we should slowing converge back the temporary up direction to the initial up direction by doing a slerp

library/src/interactor_impl.cxx Outdated Show resolved Hide resolved
@mwestphal
Copy link
Contributor

I'm trying to understand the behavior with this PR. When the camera is rolled, the temporary up is used, but when do we restore the old up value? Only when we reset the camera? I think we should slowing converge back the temporary up direction to the initial up direction by doing a slerp

Thats not what we want here I think.

@samoncrief
Copy link
Contributor Author

I'm trying to understand the behavior with this PR. When the camera is rolled, the temporary up is used, but when do we restore the old up value? Only when we reset the camera? I think we should slowing converge back the temporary up direction to the initial up direction by doing a slerp

The bug was that rotating the camera around the object while rolled causes the camera to jerk back to its original orientation. This PR does not change up in the renderer, it gives the interactor a definition to use after rolling with roll_camera that reflects the view up of the camera. When the camera is reset, the camera is oriented in a way where the renderer's up value is correct again, so at that point the flag is set to false.

@samoncrief samoncrief force-pushed the Roll-camera-view-interaction-bug branch from 43d07b6 to 449bc86 Compare January 6, 2025 16:57
@samoncrief samoncrief force-pushed the Roll-camera-view-interaction-bug branch from 449bc86 to f0a4eee Compare January 8, 2025 17:57
@samoncrief samoncrief force-pushed the Roll-camera-view-interaction-bug branch from d1b423d to 39305c5 Compare January 12, 2025 15:17
void SetTemporaryUp(const double *tempUp);
void InterpolateTemporaryUp(const double factor, const double* input);


Copy link
Contributor

Choose a reason for hiding this comment

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

you can delete this line

Copy link
Contributor

@mwestphal mwestphal left a comment

Choose a reason for hiding this comment

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

small style change then gtg

@samoncrief samoncrief force-pushed the Roll-camera-view-interaction-bug branch from 6680f6d to cc789cb Compare January 12, 2025 17:30
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.

3 participants