sub / settings menu haptics update#392
Conversation
|
this should resolve #383 |
There was a problem hiding this comment.
Changes to this PR should not be addressed currently. They can/should be addressed in December, unless all November tasks have already been completed.
That said the following is my feedback:
*Enabling/Disabling a button at the top-level of the settings face doesn't perform any haptic feedback (the "Feedback Enabled/Disabled" button). However, toggling such a button in the snap-settings sub-face does perform a selection/toggle haptic pulse. Both actions should perform the same haptic pulse.
*Clicking on a disabled button should not perform the toggle/selected haptic pulse. Currently you can click on a disabled button (not interactable), which results in the clicked/selected haptic-pulse being performed, this should not occur. In the snapping-settings sub-menu, when disabling snapping (via the top-button), you can click the disabled buttons below to see this effect in action.
AndrewTHEManeri
left a comment
AndrewTHEManeri
left a comment
There was a problem hiding this comment.
Same comments as dylan. This is pretty close
|
|
||
| public void OnRayEnter(RayEventData eventData) | ||
| { | ||
| #if INCLUDE_TEXT_MESH_PRO |
There was a problem hiding this comment.
Don't think this is #if block is needed?
There was a problem hiding this comment.
Yeah, there are no TMP references in this entire class. Looks like it's left over from before the refactor to MainMenuSelectable
mtschoen-unity
left a comment
mtschoen-unity
left a comment
There was a problem hiding this comment.
Great job! I'm glad someone took a high-level view of all this. As I said, the submenu feature was a means to an end for the Locomotion update, so it was done in haste. MainMenuSelectable was a good call!
Just some #if define stuff
| protected TextMeshProUGUI m_Description; | ||
| #endif | ||
|
|
||
| #if INCLUDE_TEXT_MESH_PRO |
There was a problem hiding this comment.
Group these fields in the same #ifdef. I know this was my code but I did it wrong :)
|
|
||
| public void OnRayEnter(RayEventData eventData) | ||
| { | ||
| #if INCLUDE_TEXT_MESH_PRO |
There was a problem hiding this comment.
Yeah, there are no TMP references in this entire class. Looks like it's left over from before the refactor to MainMenuSelectable
|
think that should handle updating to merge with latest dev & PR feedback. removed un-needed if defs. |
|
There appear to be conflicts with the SettingsUI prefab & this branch. Please be careful not to overwrite any recent fixes/changes on that prefab when merging latest development. |
…ditorVR into improvements/stella/submenu-haptics # Conflicts: # Tools/LocomotionTool/Prefabs/SettingsUI.prefab # Tools/SelectionTool/Prefabs/SettingsUI.prefab Also disable haptics on non-interactable elements and use a MainMenuButton on SubmenuFace back button
|
@dunity I merged in dev, updated TMP on the settings UI that needed it, disabled hovers on non-interactable MenuButtons and MenuToggles, and added hovers to Toggles that aren't on. Basically we check interactable, rather than isOn to block hovers. I also added a MainMenuButton to the "breadcrumb" that says "Snapping" so that it sends hover/click pulses. Also deleted unused pulses from MainMenuButton. Approved on my end. Waiting on @dunity's approval to merge do dev. |
|
Manual merge successful. Performing Github merge. |
4 participants
This adds haptic feedback to all the Toggles that are on the main menu that did not have them.
In the process, i made a base class for the menu toggles and buttons. The events aren't in the base class because working with inherited events proved to not work well. Toggles have simpler hover actions since they don't generally need / have tooltips.
I used a weaker click pulse, because the longer pulse was too long for toggles, it felt like. this one feels good to me.
I also had to update the LocomotionTool's settings UI to use TextMeshPro.