Partial fix for BoundsControl collider issues and inconsistency #9973
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Zee2
added
Input System - Near Interaction
UX Controls - Bounds Control
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
keveleigh
reviewed
Jun 15, 2021
Assets/MRTK/SDK/Features/UX/Scripts/BoundsControl/Visuals/PerAxisHandles.cs
Outdated
Show resolved
Hide resolved
Could we add a test with a strangely shaped collider as an additional scenario to cover? |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
Do we want this test before we merge? |
@davidkline-ms Yes, I'll add the test tomorrow morning. Should be quick. |
david-c-kline
approved these changes
Jun 16, 2021
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
keveleigh
pushed a commit
to keveleigh/HoloToolkit-Unity
that referenced
this pull request
Dec 7, 2021
Partial fix for BoundsControl collider issues and inconsistency
keveleigh
pushed a commit
to keveleigh/HoloToolkit-Unity
that referenced
this pull request
Dec 7, 2021
Partial fix for BoundsControl collider issues and inconsistency
keveleigh
pushed a commit
to keveleigh/HoloToolkit-Unity
that referenced
this pull request
Dec 7, 2021
Partial fix for BoundsControl collider issues and inconsistency
keveleigh
pushed a commit
to keveleigh/HoloToolkit-Unity
that referenced
this pull request
Dec 7, 2021
Partial fix for BoundsControl collider issues and inconsistency
keveleigh
pushed a commit
to keveleigh/HoloToolkit-Unity
that referenced
this pull request
Dec 8, 2021
Partial fix for BoundsControl collider issues and inconsistency
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
Currently,
BoundsControlhas several issues and inconsistencies regarding the handles and activation mechanisms. This PR is the first in a series to address these problems.This PR adjusts how the colliders on the per-axis handles are constructed and aligned, resolving the collider alignment issues for the rotate and translate handle types only. Currently, scale handles are a different type of Handle entirely, and this PR will not resolve scale handle collider issues. A future PR will address those issues.
BoundsControlconstructs a container transform for each handle and an accompanying child transform which holds the "visual" of the handle. This separation allowsBoundsControlto independently scale the handle visual without affecting the collider, which improves reliability and collision detection. (Scaling of the handle is necessary forProximityEffectand the "grow" animation that triggers on proximity.) However, this separation induces an issue where per-axis handle visuals are rotated to fit the object, but the colliders cannot be rotated to match their visual counterparts. (Colliders are axis-alignedBounds, and therefore cannot be rotated easily).Originally, these container transforms that held the colliders would not be rotated, but the underlying visual object would be rotated. To solve the issue of not being able to rotate the axis-aligned colliders, this PR revises the handle construction code to instead rotate the container transform, and leave the handle visual un-rotated. By letting the container transform perform the rotation instead of the visual, the collider remains properly aligned with the handle visual.
Old: the handle is rotated separately from the collider, causing misalignment:
New: the transform container performs the rotation, keeping the collider aligned:
Included in this PR is also several amendments to variable naming schemes and inline comments/doccomments. Due to a previous refactor, the
PerAxisHandlesclass was previously exclusively used for rotation handles. This PR removes several instances of comments/variables referring exclusively to rotation affordances.A TODO is marked to fix a hard-coded cursor icon issue. A future PR in this series will resolve that as well.
This PR partially solves #9946, but requires additional PRs to cover all functionalities affected by this issue (including scale handles!)
Changes
PerAxisHandlesto more accurately and consistently reflect the current usageVisualUtilsso thatGetMaxComponentis always used to determine handle size, rather than an arbitrary axisVerification
Existing BoundsControl tests cover this functionality, but they previously passed due to the margin of error/tolerance in the test case. Tighter test tolerances, in my opinion, are not strictly necessary. The issue this solves is more visible with larger/more strangely shaped colliders, as seen in the linked issue.