Skip to content

Fix for issue #5423#5528

Merged
keveleigh merged 3 commits intomicrosoft:prerelease/2.0.0_stabilizationfrom
anuraag016:fix_issue5423
Aug 8, 2019
Merged

Fix for issue #5423#5528
keveleigh merged 3 commits intomicrosoft:prerelease/2.0.0_stabilizationfrom
anuraag016:fix_issue5423

Conversation

@anuraag016
Copy link
Contributor

Fix for issue #5423 Controllers shown at original place after teleporting in VR.

Overview

This fix addresses the issue #5423 Controllers shown at original place after teleporting in VR.

Changes

The fix is in the BaseController.cs file.
In the function:
protected virtual bool TryRenderControllerModel(Type controllerType, InputSourceType inputSourceType)

we add MixedRealityPlayspace.AddChild(controllerObject.transform);

This line should actually be moved inside TryAddControllerModelToSceneHierarchy function

so that even if we use the platform controller visualization, they will be correctly added to under MixedRealityPlaySpace game object.

@anuraag016 anuraag016 marked this pull request as ready for review August 6, 2019 10:14
@wiwei wiwei requested a review from keveleigh August 6, 2019 15:19
@wiwei
Copy link
Contributor

wiwei commented Aug 6, 2019

@keveleigh I think we were just chatting about this last week - was this one of the options you were thinking about?

@keveleigh
Copy link
Contributor

keveleigh commented Aug 6, 2019

Unfortunately, I don't believe just this change fixes it (after #5250). I implemented this locally last week (keveleigh@ba647fc) and saw incorrect behavior in mrtk_dev.

I'm thinking through a combination of reverting #5250 and adding this change as part of more clearly defining the expected data from source pose events (world space vs local space).

david-c-kline
david-c-kline previously approved these changes Aug 6, 2019
@david-c-kline david-c-kline dismissed their stale review August 6, 2019 17:28

dimissing per @keveleigh's comments

@ritijain
Copy link
Contributor

ritijain commented Aug 7, 2019

Unfortunately, I don't believe just this change fixes it (after #5250). I implemented this locally last week (keveleigh@ba647fc) and saw incorrect behavior in mrtk_dev.

I'm thinking through a combination of reverting #5250 and adding this change as part of more clearly defining the expected data from source pose events (world space vs local space).

@keveleigh is this something you have the bandwith to try? What was the undesirable behavior that you observed, so that @anuraag016 can try it out as well?

@keveleigh
Copy link
Contributor

keveleigh commented Aug 7, 2019

@ritijain

is this something you have the bandwith to try?

I've blocked out some time tomorrow to focus on some MRTK issues, including this one!

What was the undesirable behavior that you observed

After teleporting, the controllers were in an unexpected location. I had originally fixed that with #5250, which is in mrtk_dev but not RC2.1, but I'm no longer convinced I fixed it correctly, after the issue @anuraag016 reported here. Combining the two fixes breaks things again though, from what I experienced

keveleigh added a commit to anuraag016/MixedRealityToolkit-Unity that referenced this pull request Aug 7, 2019
Due to microsoft#5250's change plus microsoft#5528's change to parent the controllers to the playspace
@keveleigh
Copy link
Contributor

keveleigh commented Aug 7, 2019

@wiwei @davidkline-ms
I merged my work into this PR, to account for both this more correct fix and the changes I made in #5250. I decided to take the path of defining both events as world space data, since that's likely more useful for an end developer.

I believe this PR is ready for review now!

(also just force pushed this to be built off stabilization instead of dev)

@keveleigh keveleigh requested a review from david-c-kline August 7, 2019 20:30
@keveleigh keveleigh self-assigned this Aug 7, 2019
@keveleigh keveleigh requested a review from wiwei August 7, 2019 20:30
@keveleigh keveleigh changed the base branch from mrtk_development to prerelease/2.0.0_stabilization August 7, 2019 20:30
@keveleigh keveleigh changed the base branch from prerelease/2.0.0_stabilization to mrtk_development August 7, 2019 20:30
anuraag016 and others added 3 commits August 7, 2019 13:33
Fix for issue microsoft#5423 Controllers shown at original place after teleporting in VR.
The fix is in the BaseController.cs file.
In the function:
protected virtual bool TryRenderControllerModel(Type controllerType, InputSourceType inputSourceType)

we add MixedRealityPlayspace.AddChild(controllerObject.transform);

This line should actually be moved inside TryAddControllerModelToSceneHierarchy function

so that even if we use the platform controller visualization, they will be correctly added to under MixedRealityPlaySpace game object.
Due to microsoft#5250's change plus microsoft#5528's change to parent the controllers to the playspace
@keveleigh keveleigh changed the base branch from mrtk_development to prerelease/2.0.0_stabilization August 7, 2019 20:33
@keveleigh keveleigh closed this Aug 7, 2019
@keveleigh keveleigh reopened this Aug 7, 2019
@keveleigh
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link

@david-c-kline david-c-kline left a comment

Choose a reason for hiding this comment

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

Good fixes, nice collaboration!

@keveleigh keveleigh merged commit ccd4145 into microsoft:prerelease/2.0.0_stabilization Aug 8, 2019
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.

5 participants