Skip to content

[bugfix] - moveUp moveDown#1763

Merged
aclegg3 merged 1 commit intomainfrom
moveUp-fix
Jun 1, 2022
Merged

[bugfix] - moveUp moveDown#1763
aclegg3 merged 1 commit intomainfrom
moveUp-fix

Conversation

@aclegg3
Copy link
Copy Markdown
Contributor

@aclegg3 aclegg3 commented May 25, 2022

Motivation and Context

moveUp and moveDown actions (in C++) are used exclusively in the viewer application to raise and lower the agent camera. Current implementation uses camera local "up" translation. When the camera is pitched this results in moving the camera node off-axis, changing the axis of rotation.

This PR changes moveUp and moveDown action to apply parent coordinate system +Y translation instead of local "up" so the camera always stays constrained to the agent's "up" axis.

If moving the camera off-axis is desirable it should be implemented as a separate action.

How Has This Been Tested

Local in viewer.

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have completed my CLA (see CONTRIBUTING)
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label May 25, 2022
@aclegg3 aclegg3 marked this pull request as ready for review May 25, 2022 20:38
object.translateLocal(object.transformation().up() * distance);
// Note: this is not a body action and is applied to the sensor rather than
// the Agent, so it will move the sensor in Agent's +Y (up) direction
object.translate(Magnum::Vector3(0, 1, 0) * distance);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@aclegg3 This is a magic number (or rather vector). May want to make it a global constant somewhere.

@aclegg3 aclegg3 merged commit a776e4f into main Jun 1, 2022
@aclegg3 aclegg3 deleted the moveUp-fix branch June 1, 2022 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed Do not delete this pull request or issue due to inactivity.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants