Skip to content
This repository has been archived by the owner on Aug 11, 2024. It is now read-only.

Fixed the ambiguous interaction mapping constructors #347

Merged
merged 4 commits into from
Sep 18, 2019

Conversation

StephenHodgson
Copy link
Contributor

@StephenHodgson StephenHodgson commented Sep 18, 2019

XRTK - Mixed Reality Toolkit Change Request

Overview

Fixed the ambiguous interaction mapping constructors.

Target of the change:

Is this enhancement for:

  • Core (core framework, interfaces and definitions)

Changes:

Brief list of the targeted features that are being changed.

Breaking Changes:

  • Update the MixedRealityInteractonMapping constructors but afaik the Oculus package is the only one taking advantage of it atm.

@SimonDarksideJ
Copy link
Contributor

This addresses only the changes for oculus, however, it also breaks all existing mappings for all other controllers. This needs to be based off this fix branch.
Also, it will need the default assets updating with this fix. Let's close this PR and I'll incorporate your changes in my branch.

@StephenHodgson
Copy link
Contributor Author

however, it also breaks all existing mappings for all other controllers. This needs to be based off this fix branch.

I don't understand. This shouldn't break any existing mappings.

Let's close this PR and I'll incorporate your changes in my branch.

I don't think that's a good idea.

@StephenHodgson
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link
Contributor

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

@StephenHodgson StephenHodgson changed the base branch from fix/newprofilefix to development September 18, 2019 16:01
@StephenHodgson
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link
Contributor

Azure Pipelines successfully started running 1 pipeline(s).

SimonDarksideJ
SimonDarksideJ previously approved these changes Sep 18, 2019
Copy link
Contributor

@SimonDarksideJ SimonDarksideJ left a comment

Choose a reason for hiding this comment

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

Not a fan of mixing up the order of parameters like this, but it works.

@StephenHodgson
Copy link
Contributor Author

StephenHodgson commented Sep 18, 2019

Not a fan of mixing up the order of parameters like this, but it works.

Any suggestions on how this could be better?

Mainly the issue is that the wrong Constructor is being called because of the order of the parameters.
One possible fix is to make the type different, but I'm not sure what would be an appropreate change.

The only other possibility is removing the inputName altogether and using the established inputAxisX and inputAxisY values that essentially function for the same purpose.

@SimonDarksideJ
Copy link
Contributor

The only other possibility is removing the inputName altogether and using the established inputAxisX and inputAxisY values that essentially function for the same purpose.

No, they are defined concepts and should not be reused.

It's an acceptable fix

@StephenHodgson StephenHodgson merged commit 6d9aedf into development Sep 18, 2019
@StephenHodgson StephenHodgson deleted the fix/input-mapping-constructors branch September 18, 2019 17:31
XRTK-Build-Bot pushed a commit that referenced this pull request Sep 19, 2019
* Fixed the ambiguous interaction mapping constructors

* Update XRTK-Core/Packages/com.xrtk.core/Definitions/Devices/MixedRealityInteractionMapping.cs

* Added assets for mapping axes
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Breaking Change Ready for review PR finished primary development, open for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants