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

Fixing BC OnEnable issue when non-default activation type specified in-editor #10039

Merged
merged 1 commit into from
Jun 28, 2021

Conversation

Zee2
Copy link
Contributor

@Zee2 Zee2 commented Jun 28, 2021

Overview

A regression was introduced in a previous BoundsControl fix that impacted activation behavior when a non-default activation type was specified in-editor. Behavior was correct when a BoundsControl was set up at runtime, as OnEnable would run with the default activation type, and any subsequent sets to BoundsControlActivationType would properly evaluate the activation policy. The regression only impacted scenarios when a non-default activation type was specified in-editor, and OnEnable ran with that activation type.

As a result, this is technically a symptom of missing test coverage. However, this bug is only visible when an option is specified in edit-mode, and then the scene is played. As we can't really write tests that cover a transition from edit to play mode, @CDiaz-MS confirmed that this should be alright to merge without a specific test. This issue can be smoked out by running the BoundsControlExamples example scene and checking that the BCs do not activate when they're not supposed to.

Root cause: the order of CreateRig and SetActivationFlags had been swapped in a previous fix, as SetActivationFlags requires the TargetBounds to be set in order to be able to determine the flattening result. CreateRig was moved to be called before SetActivationFlags, so that DetermineTargetBounds would be called before SetActivationFlags needed the TargetBounds. However, this introduced a regression wherein CreateRig would not respect a non-default activation type if specified in-editor, as it was not calculated yet. The fix is to revert to the prior ordering of CreateRig and SetActivationFlags, but introduce a preemptive call to DetermineTargetBounds, to make sure TargetBounds has been initialized before any other functions need it.

Changes

  • Reverts the initialization order of CreateRig and SetActivationFlags to a prior version
  • Adds a pre-emptive call to DetermineTargetBounds to resolve target bounds before any other functions need it

Verification

As specified above, a test to cover this behavior is not practically implementable. Verification can be done by smoke testing the BoundsControlExamples scene.

@Zee2 Zee2 requested review from keveleigh and thalbern June 28, 2021 21:34
@Zee2 Zee2 self-assigned this Jun 28, 2021
@Zee2 Zee2 added Bug UX Controls - Bounds Control formerly known as BoundingBox labels Jun 28, 2021
@Zee2 Zee2 added this to the MRTK 2.x future milestone Jun 28, 2021
@Zee2
Copy link
Contributor Author

Zee2 commented Jun 28, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@Zee2 Zee2 merged commit fae0401 into microsoft:main Jun 28, 2021
@polar-kev polar-kev modified the milestones: MRTK 2.x future, MRTK 2.7.3 Nov 22, 2021
keveleigh pushed a commit to keveleigh/HoloToolkit-Unity that referenced this pull request Dec 7, 2021
Fixing BC OnEnable issue when non-default activation type specified in-editor
keveleigh pushed a commit to keveleigh/HoloToolkit-Unity that referenced this pull request Dec 7, 2021
Fixing BC OnEnable issue when non-default activation type specified in-editor
keveleigh pushed a commit to keveleigh/HoloToolkit-Unity that referenced this pull request Dec 7, 2021
Fixing BC OnEnable issue when non-default activation type specified in-editor
keveleigh pushed a commit to keveleigh/HoloToolkit-Unity that referenced this pull request Dec 7, 2021
Fixing BC OnEnable issue when non-default activation type specified in-editor
keveleigh pushed a commit to keveleigh/HoloToolkit-Unity that referenced this pull request Dec 8, 2021
Fixing BC OnEnable issue when non-default activation type specified in-editor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug UX Controls - Bounds Control formerly known as BoundingBox
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants