Mixedrealitytoolkit-unity: PinchSlider orientation doesn't change when setting Slider Axis to Y or Z

[Kent1LG]

Describe the bug

In PinchSlider script, Slider Axis property can be set to X, Y or Z axis.
While this setting do modify the way you can move ThumbRoot, it doesn't change TrackVisuals and TickMarks orientation.

To reproduce

Steps to reproduce the behavior:

1. Create a simple HoloLens 2 scene
2. Add PinchSlider prefab
3. Set Slider Axis to Y or Z axis

Expected behavior

Changing Slider Axis on the Pinch Slider script should also change TrackVisuals and TickMarks orientation, if TrackVisuals and TickMarks object is supplied.

When user changes the SliderAxis of the PinchSlider:

1. The TrackVisuals orientation will change to match the new axis
2. Its position will be reset so its at the center of the slider and doesn't retain any faulty position based on previous orientation.
3. The TickMarks layout of the grid will be updated to match the new axis
4. The rotation as well will change in case of slider axis being in z axis.
5. The position will be reset so its at the center of the slider and doesn't retain any faulty position based on previous orientation.

The position of TrackVisuals and TickMarks will be reset every time the user changes the SliderAxis. So the user will need to position these manually.

The scale will NOT be updated in TrackVisuals and TickMarks.

If TrackVisuals and TickMarks are not specified, then TrackVisuals and TickMarks are not updated.

Tests to Add

• [ ] There should be tests to verify that track visuals and tickmarks are oriented properly given slider orientation. Additionally, there should be a test to verify that if the visuals are not specified, that the slider will still work.

Documentation to Add

• [ ] Finally, documentation should state that visuals will be oriented to match orientation, but will not scale to match the endpoints of the slider.

Screenshots

Your Setup (please complete the following information)

• Unity Version 2018.4.12f1
• MRTK Version 2.1

Target Platform (please complete the following information)

• HoloLens 2
• Unity Editor

Additional context

Should be great to add:

• Possibility to modify Min and Max value
• Possibility to switch between Float and Whole Numbers

[Alexees]

Recognized that as well

[Troy-Ferrell]

@julenka FYI

[artsouflMS]

Hello,
I am interested to fix this
Is this currently assigned to someone?
If someone is already working on it or was planning to work on it then I will pick another bug.
Please let me know
Artemis

[julenka]

Hi, thank you for offering to help! Nobody is looking at it, I'll assign it to you.

[julenka]

Hi @artsouflMS , it would be good to clarify the behavior of slider when visuals are / are not present beefore making the PR, just be clear on what you'll be doing. Trying to avoid case where you make a PR and it ends up being something that might not be what others want. I've added a few additionally specs to the Expected Behavior section. Could you add more details for the behavior you plan to implement for the slider?

Thank you again for your help!

Here are the changes I made:

Expected behavior

Changing Slider Axis on the Pinch Slider script should also change TrackVisuals and TickMarks orientation, if TrackVisuals and TickMarks object is supplied.

There should be tests to verify that track visuals and tickmarks are oriented properly given slider orientation. Additionally, there should be a test to verify that if the visuals are not specified, that the slider will still work.

Finally, documentation should state that visuals will be oriented to match orientation, but will not scale to match the endpoints of the slider.

[artsouflMS]

Hi @julenka , thank you for your update
The behaviour that I am planning to implement is the following:

When user changes the SliderAxis of the PinchSlider:
The TrackVisuals orientation will change to match the new axis
Its position will be reset so its at the center of the slider and doesn't retain any faulty position based on previous orientation.
The TickMarks layout of the grid will be updated to match the new axis
The rotation as well will change in case of slider axis being in z axis.
The position will be reset so its at the center of the slider and doesn't retain any faulty position based on previous orientation.

The position of TrackVisuals and TickMarks will be reset every time the user changes the SliderAxis. So the user will need to position these manually.
The scale will NOT be updated in TrackVisuals and TickMarks.

Please let me know how does that sound.
Could I start implementing this behaviour?

Kind Regards,
Artemis

[Kent1LG]

Hey @artsouflMS , @julenka ,

If you are interrested i modified the PinchSlider script :

• Possibility to choose a minimum and maximum value.
• Possibility to choose to slide with whole or float value.
• The TicksMarks are dynamically setup (UpdateTickMarks()).
  (optional)- SliderSounds's "TickEvery" variable is dynamically change too (Need a SetTickEvery function because of the private access)

The script : https://gist.github.com/Kent1LG/97fd90ad5b6858a2d55def8cbd2f99f5
I left all the Scripts names like before but i don't know if it's gonna be an issue, let me know.

I add comments with "QL" where i modified.

Keep me in touch !

[artsouflMS]

Hello @Kent1LG

thanks for letting me know
Are you planning to merge your changes into mrtk_development?

Kind regards
Artemis

[Kent1LG]

I'm planning to, yes. But i just don't want to slow you down with new scripts version or something else ahah.
In your opinion , Should i merge?

[artsouflMS]

You don't slow me down, don't worry :).
I cant use your changes if these aren't in mrtk_development
So go ahead and create a PR
And when its accepted then I will be able to use your changes
Thanks
Artemis

[julenka]

Hi @Kent1LG and @artsouflMS it's great to see this collaboration and discussion! I just finished reviewing the PR, and after reading this thread I understand that one of my suggestions in the PR comments (to create a custom slider that extends PinchSlider) might not work. In that case I think it would be possible to put all the fields like tick marks, sounds, into the PinchSlider class, as long as they degrade gracefully when the fields are not filled in (in case people do not want these), and as long as there are tests to verify behavior. Thank you again for contributing!

[julenka]

Hi @artsouflMS, couple questions on the behavior:

The TickMarks layout of the grid will be updated to match the new axis
What is the behavior if no TickMarks are present?

The position of TrackVisuals and TickMarks will be reset every time the user changes the SliderAxis. So the user will need to position these manually.

What if TrackVisuals or TickMarks are not provided?

For now I assume that if these are not provided, then TrackVisuals and TickMarks do not get updated.

Other than those follow ups, I think this sounds good. You can go ahead an implement, I've updated the bug with the expected behavior, and tests / documentation to add

[artsouflMS]

Hi @julenka

thank you for your update
I will start implementing the above behaviour
As you said, if TickMarks or TrackVisuals aren't provided then they are not updated

Thank you,
Artemis