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

CMCL-0000: Fix OrbitalFollow inappropriate vertical damping #942

Merged
merged 5 commits into from Mar 8, 2024

Conversation

glabute
Copy link
Collaborator

@glabute glabute commented Mar 4, 2024

Purpose of this PR

Bug:

Create the following:

  • OrbitalFollow with default 3-ring setup.
  • RotationComposer with large H and V damping (say 5 seconds).
  • Follow target at player's feet, LookAt target at player's shoulder.
  • Play scene, move mouse vertically: observe vertical damping is visible, not horizontal damping.

No damping should happen in response to mouse movement (only player movement).

This PR fixes that. OrbitalFollow was calculating the damping bypass for the follow target when it should be for the lookat target.

Testing status

  • Added an automated test
  • Passed all automated tests
  • Manually tested

Documentation status

  • Updated CHANGELOG
  • Updated README (if applicable)
  • Commented all public classes, properties, and methods
  • Updated user documentation

Technical risk

low

@codecov-commenter
Copy link

codecov-commenter commented Mar 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 26.68%. Comparing base (c97b6a7) to head (7d32f6a).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #942   +/-   ##
=======================================
  Coverage   26.67%   26.68%           
=======================================
  Files         246      246           
  Lines       27572    27573    +1     
=======================================
+ Hits         7356     7358    +2     
+ Misses      20216    20215    -1     

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

@unity-cla-assistant
Copy link

unity-cla-assistant commented Mar 8, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@AntoineCharton AntoineCharton left a comment

Choose a reason for hiding this comment

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

The damping for the mouse seems accurate however the damping for the rotation doesn't look like 5 seconds. Is it expected?

Mar-08-2024 13-48-08

Screenshot 2024-03-08 at 1 49 45 PM

@glabute
Copy link
Collaborator Author

glabute commented Mar 8, 2024

hmmmm nice catch!
Back to the drawing board with tis one....

@glabute glabute closed this Mar 8, 2024
@glabute glabute reopened this Mar 8, 2024
Copy link
Collaborator

@AntoineCharton AntoineCharton left a comment

Choose a reason for hiding this comment

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

Makes more sense with the update you made.

@glabute glabute merged commit b82688f into main Mar 8, 2024
4 of 9 checks passed
@glabute glabute deleted the dev/fix-orbital-follow-vertical-lag branch March 8, 2024 21:46
glabute added a commit that referenced this pull request Mar 11, 2024
* Add missing length check to 3rdPersonFollow

* CMCL-1551: add missing empty check when overriding a blend stack (#930)

* add missing empty check

* Update CHANGELOG.md

* Better filtering for vcam Navel Gazing warning (#927)

* Revert "Update CinemachineThirdPersonFollow.cs"

This reverts commit c15efbe.

* Update CMWaveform.compute

* Update CMWaveform.compute

* Update README.md

* another tweak - binding modes this time

* Revert "another tweak - binding modes this time"

This reverts commit 91a0b6c.

* Fix typo in Editor min req version (#936)

* CMCL-0000: FreeLookOnSphericalSurface sample is improved (#935)

* FreeLookOnSphericalSurface sample is improved, adding a moving surface and second camera.

* forgot to commit

* tewaking blend hints

* binding mode tweaks

* Cleanup, tweaking

* cosmetic

* bugfix in AddCustomBlendable

* deprecate RangeSliderAttribute (#944)

* Add some missing null checks in shot editor to handle missing scripts

* CMCL-0000: add isDelayed to some fields (#939)

* add isDelayed to some fields

* Fix outdated tooltip, add "Custom" label to SensorSize presets dropdown

* Update LensSettings.cs

* statedrivencamera bugfix for minactivationtime, add CancelWait (#938)

* CMCL-1565: Deoccluder improvements, and add CinemachineShotQualityEvaluator (#899)

* Add CinemachineDecollider

* Added Cinemachine Terran Decollider

* Update RuntimeUtility.cs

* Fix OrbitalFollow inappropriate vertical damping

* Update CinemachineDecollider.cs

* small fixes

* Add CinemachineShotQualityEvaluator

* Add deoccluder Resolve towards follow target option

* Terrain Decollider improvements

* Deoccluder evaluates shot quality only when evaluation is enabled

* Add doc and editor for shot quality evaluator

* Remove TerrainDecollider

* Add terrain functionality to Decollider

* Remove decollider (to another PR)

* Update CinemachineShotQualityEvaluator.md

* Update TableOfContents.md

* Update CHANGELOG.md

* Update CHANGELOG.md

* Remove ResolveTowardsFollowTarget

* Update CHANGELOG.md

* Update CinemachineOrbitalFollow.cs

* Update CinemachineOrbitalFollow.cs

* Update CinemachineDeoccluder.cs

* Add a comment

* CMCL-0000: Fix OrbitalFollow inappropriate vertical damping (#942)

* Fix OrbitalFollow inappropriate vertical damping

* small fixes

* Revert "small fixes"

This reverts commit 7d32f6a.

* new strategy

* Setting isDelayed was causing some exceptions on domain reload

* Add another missing isDelayed

* Axis recentering happens independently on axes (#937)

---------

Co-authored-by: Gregory Labute <gregoryl@unity3d.com>
Co-authored-by: Sébastien Duverne <55094336+sebastienduverne@users.noreply.github.com>
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

4 participants