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.

Sign up for GitHub

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

Jump to bottom

Fix spatial observer and validate with tests #6919

Merged
merged 5 commits into from
Dec 23, 2019
Merged

Fix spatial observer and validate with tests #6919

merged 5 commits into from
Dec 23, 2019

Conversation

Troy-Ferrell
Copy link
Contributor

Overview

WMRObserver had a bug where it would auto-start itself in enable() even when set to manual start.

This change move the functionality to BaseObjectObserver and creates some tests for basic observer functionality.

Changes

Verification

As a reviewer, it is possible to check out this change locally by using the following
commands (substituting {PR_ID} with the ID of this pull request):

git fetch origin pull/{PR_ID}/head:name_of_local_branch

git checkout name_of_local_branch

@Troy-Ferrell Troy-Ferrell self-assigned this Dec 17, 2019
@Troy-Ferrell
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

julenka
julenka reviewed Dec 17, 2019
View reviewed changes
Copy link
Contributor

@julenka julenka left a comment

Choose a reason for hiding this comment

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

Thank you so much for following our guidelines and adding a test for this fix :-D I don't know enough about this system to sign off but had a suggestion for the tests.

Assets/MixedRealityToolkit.Tests/PlayModeTests/SpatialObserverTests.cs Outdated Show resolved Hide resolved
Assets/MixedRealityToolkit.Tests/PlayModeTests/SpatialObserverTests.cs Show resolved Hide resolved
keveleigh
keveleigh approved these changes Dec 17, 2019
View reviewed changes
Copy link
Contributor

@keveleigh keveleigh left a comment

Choose a reason for hiding this comment

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

Approved pending the missing summary Julia pointed out

@Troy-Ferrell
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@Troy-Ferrell Troy-Ferrell mentioned this pull request Dec 22, 2019
Closed
@Troy-Ferrell
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@Troy-Ferrell Troy-Ferrell merged commit 8536657 into microsoft:mrtk_development Dec 23, 2019
@keveleigh keveleigh mentioned this pull request Jan 3, 2020
Merged
@keveleigh keveleigh modified the milestone: MRTK 2.3.0 Jan 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@julenka julenka julenka left review comments

@keveleigh keveleigh keveleigh approved these changes

@wiwei wiwei Awaiting requested review from wiwei

@david-c-kline david-c-kline Awaiting requested review from david-c-kline

@CDiaz-MS CDiaz-MS Awaiting requested review from CDiaz-MS

@thalbern thalbern Awaiting requested review from thalbern

Assignees

@Troy-Ferrell Troy-Ferrell

Labels
Spatial Mapping / Awareness
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

MRTK 2.2 - Spatial Mapping starts automatically
4 participants
@Troy-Ferrell @julenka @keveleigh @david-c-kline