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

Added handle types for 2d/3d translation #711

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Daniel4144
Copy link
Contributor

implemented #707

added HandleType.Translation2D and HandleType.Translation3D

sample:
https://github.com/Daniel4144/MixedRealityToolkit-Unity/tree/handles-sample
scene handles-sample.unity in project MRTKDevTemplate

@MaxPalmer-UH
Copy link
Contributor

I took a look at the example scene and interactions and have added this video as context.

Unity_lUxR2tyGBh.mp4

@Daniel4144
Copy link
Contributor Author

Side note to the video: Grabbing one of the three planar elements enables movement along two dimensions, which I think is not shown in the video.
The colliders for the other handles are quite big in that example so it might be difficult to grab the planes (I have only tested it with the far interactor)

@MaxPalmer-UH
Copy link
Contributor

Side note to the video: Grabbing one of the three planar elements enables movement along two dimensions, which I think is not shown in the video. The colliders for the other handles are quite big in that example so it might be difficult to grab the planes (I have only tested it with the far interactor)

Thanks for that clarification.

@keveleigh
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@AMollis AMollis requested a review from ms-RistoRK April 16, 2024 16:50
@Daniel4144
Copy link
Contributor Author

Any update, will this be merged?

@ms-RistoRK
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@ms-RistoRK ms-RistoRK 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 for your contribution, this change looks great. We are trying to keep the codebase as clean as possible so we encourage contributors to avoid leaving commented code out of main branch. Could you please remove the commented code?

Comment on lines +961 to +966
// TODO: Elastics integration (soon!)

// if (elasticsManager != null)
// {
// transformUpdated = elasticsManager.ApplyTargetTransform(constraintTranslate, TransformFlags.Move);
// }
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for your contribution, this change looks great. We are trying to keep the codebase as clean as possible so we encourage contributors to avoid leaving commented code out of main branch. Could you please remove the commented code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The same commented code is in all other if-branches, so I decided to keep it in the two branches I added. I think removing the comments only here would cause more headache, in case that code is still relevant in the future.
In fact, the new branches are just copies of the existing branch for HandleType.Translation where only the first line of the branch changed, which is arguably not the ideal solution. Alternatively, I could merge the three branches for the translation types and add another if-statement inside to calculate the correct translation vector.
Let me know what you think about it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand your point of view, I too leave commented sections in my personal branches. However, since this is an open-source project I think it is best to leave the code as clean as possible because not everyone in the community may have the same context as the author. @MixedRealityToolkit/mixedrealitytoolkit-unity-maintainers What are your thoughts on this?

Comment on lines +936 to +941
// TODO: Elastics integration (soon!)

// if (elasticsManager != null)
// {
// transformUpdated = elasticsManager.ApplyTargetTransform(constraintTranslate, TransformFlags.Move);
// }
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for your contribution, this change looks great. We are trying to keep the codebase as clean as possible so we encourage contributors to avoid leaving commented code out of main branch. Could you please remove the commented code?

@Daniel4144
Copy link
Contributor Author

Just noticed that there were some bigger changes in the involved script in the meantime, that make it impossible to merge, so I guess this PR is obsolete...
Although these other changes seem to make it possible to add new, arbitrary translation types (even without touching the existing code), adding several different translation handles to the same boundscontrol as seen in the video above is not possible.

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.

4 participants