Persistent undo menu by jono-unity · Pull Request #407 · Unity-Technologies/EditorXR

jono-unity · 2017-11-21T01:05:46Z

Purpose of this PR

Implement a persistent undo/redo menu: any controller without a radial menu currently open can undo/redo. On Touch, click in the joystick and then flick left or right; on Vive, click in left/right on the touchpad.

This change necessitated changing the main menu behavior on Vive: it is now controlled by swiping the touchpad, rather than clicking in.

Testing status

Tested on Vive and Rift in various scenes. Some objects and operations take a long time to undo/redo, and undo/redo behavior can be inconsistent. This seems to be true across all undo/redo functionality in EVR, as I saw the same inconsistency & delay on some objects when also testing with the existing radial menu actions.

Technical risk

Medium: As this changes behavior in MainMenu, and also is a nearly-always-open menu consuming stick/pad input, the UX & interaction with other tools impact is non-trivial.

Comments to reviewers

-Note the inconsistent undo/redo behavior I described in testing status -- this is a big issue, out of the scope of this PR.
-I'm not fully satisfied with the MainMenu swipe behavior yet and intend to work that some more (and would love feedback about it) -- but wanted to get this in front of everyone asap.

…ditorVR into features/jono/undo-redo-flick

…se it's hard to keep it pressed in while flicking.

…nu to work together as expected.

…te face.

dunity

Overall nice work!

IMO there should be a visual means of denoting (perhaps with an arrow color and/or size change, etc) that there is an available undo or redo action that can be performed. I'm not really sure when I can or can't undo (mostly due to the broken undo/redo issue).

Aside from the PR comments, I ran into an issue that randomly showed itself when enabling and disabling the Radial menu. "Coroutine couldn't be started because the the game object 'UndoMenu(Clone)' is inactive!" Likely the undo menu needs to stopAllCoroutines on disable, or something along those lines.

In addition, on a few occassions, when quickly undoing and redoing on the left touch controller, I would get the Undo tooltip/feedback to lock, and not fade away, or stop displaying any longer. Though I couldn't find a reliable repro for that.

dunity · 2017-11-21T02:12:48Z

@@ -1,5 +1,6 @@
 #if UNITY_EDITOR
 using System;
+using System.Collections;


Remove using

dunity · 2017-11-21T02:14:17Z

            {
-                m_MainMenuUI.targetFaceIndex += (int)Mathf.Sign(rotationInput);
-                this.Pulse(node, m_FaceRotationPulse);
+                if (rotationInput != 0f)


!Mathf.Approximately(rotationInput, 0f)

dunity · 2017-11-21T02:14:42Z

-                this.Pulse(node, m_FaceRotationPulse);
+                if (rotationInput != 0f)
+                {
+                    if (m_LastRotationInput == 0f)


Mathf.Approximately(m_LastRotationInput, 0f)

dunity · 2017-11-21T02:25:21Z

+            var currentDuration = 0f;
+            while (transitionAmount < 1f)
+            {
+                m_UndoButtonMaterial.SetColor(k_MaterialColorProperty, Color.Lerp(undoStartingColor, targetColor, transitionAmount));


Use SmoothInOutLerpFloat()
var smoothAmount = MathUtilsExt.SmoothInOutLerpFloat(transitionAmount);
m_UndoButtonMaterial.SetColor(k_MaterialColorProperty, Color.Lerp(undoStartingColor, targetColor, smoothAmount));
m_RedoButtonMaterial.SetColor(k_MaterialColorProperty, Color.Lerp(redoStartingColor, targetColor, smoothAmount));

Orrrr maybe AnimateProperty, once that lands? I'm inclined to punt for that reason.

I've switched to SmoothInOutLerp for now, but yep, that AnimateProperty sounds pretty good. Where's that branchwise?

It's in Dev now. https://github.com/Unity-Technologies/EditorVR/blob/development/Scripts/Utilities/TransitionUtils.cs

Here's how it's used https://github.com/Unity-Technologies/EditorVR/blob/development/Scripts/Proxies/ProxyNode.cs#L146

dunity · 2017-11-21T02:25:59Z

+            targetColor.a = k_DisengagedAlpha;
+            while (transitionAmount < 1f)
+            {
+                var currentColor = Color.Lerp(startingColor, targetColor, transitionAmount);


Lerp using MathUtilsExt.SmoothInOutLerpFloat(transitionAmount), as mentioned above.

dunity · 2017-11-21T02:37:29Z

+            ObjectUtils.Destroy(m_RedoButtonMaterial);
+        }
+
+        public void Setup()


Remove Setup function. Disable the UI gameObject in UndoMenu's Start() function.

dunity · 2017-11-21T02:37:55Z

+            m_UndoMenuUI.alternateMenuOrigin = alternateMenuOrigin;
+            m_UndoMenuUI.actions = menuActions;
+            this.ConnectInterfaces(m_UndoMenuUI); // Connect interfaces before performing setup on the UI
+            m_UndoMenuUI.Setup();


Remove setup method, just disable UI gameobject here.

Yep! Though we use them in quite a few places, our goal is to avoid Setup methods where possible.

dunity · 2017-11-21T02:38:58Z

+        void ShowFeedback()
+        {
+            List<VRInputDevice.VRControl> controls;
+            if (m_Controls.TryGetValue("Engage", out controls))


Use singular class-level const string for "Engage"

dunity · 2017-11-21T02:39:21Z

+        void ShowUndoPerformedFeedback(bool undo)
+        {
+            List<VRInputDevice.VRControl> controls;
+            if (m_Controls.TryGetValue("Engage", out controls))


Use singular class-level const string for "Engage"

dunity · 2017-11-21T02:43:43Z

+
+            if (undoRedoPerformed)
+            {
+                consumeControl(undoMenuInput.navigateX);


Should the AcceptInputAfterStickReleased coroutine be stopped here if currently running?

mtschoen-unity

Great work! Way to grok the coventions and common patterns right off the bat.

I was able to find an error after monkeying around for a bit (the student becomes the master!)

Coroutine couldn't be started because the the game object 'UndoMenu(Clone)' is inactive!
UnityEngine.SetupCoroutine:InvokeMoveNext(IEnumerator, IntPtr)

My guess is that it's from setting the gameobject to inactive in the visibility property without stopping all coroutines.

mtschoen-unity · 2017-11-21T07:05:10Z

+
+        public Node node { get; set; }
+
+        public event Action<Transform> itemWasSelected;


There's a warning about this being unused. Does UndoMenu need to be an IAlternateMenu? Could it just be an IMenu? I think we can still just attach it to the alternate menu origin but make it not actually be a IAlternateMenu. Same goes with IUsesRayOrigin (Included in IAlternateMenu). We don't seem to use/need it.

It looks like this will require a refactor of SpawnAlternateMenu, etc. but to look at it, all we do is check that it's the right interface, add a component, and connect interfaces. I think we could make it generic no problem.

mtschoen-unity · 2017-11-21T07:05:33Z

+
+        readonly BindingDictionary m_Controls = new BindingDictionary();
+
+        public Transform rayOrigin { private get; set; }


Yep. We should kill it. See comment below.

mtschoen-unity · 2017-11-21T07:06:20Z

+            m_UndoMenuUI.alternateMenuOrigin = alternateMenuOrigin;
+            m_UndoMenuUI.actions = menuActions;
+            this.ConnectInterfaces(m_UndoMenuUI); // Connect interfaces before performing setup on the UI
+            m_UndoMenuUI.Setup();


Yep! Though we use them in quite a few places, our goal is to avoid Setup methods where possible.

mtschoen-unity · 2017-11-21T07:16:51Z

+            var currentDuration = 0f;
+            while (transitionAmount < 1f)
+            {
+                m_UndoButtonMaterial.SetColor(k_MaterialColorProperty, Color.Lerp(undoStartingColor, targetColor, transitionAmount));


Orrrr maybe AnimateProperty, once that lands? I'm inclined to punt for that reason.

mtschoen-unity · 2017-11-21T07:17:11Z

+            {
+                if (m_Engaged == value)
+                    return;
+                m_Engaged = value;


Blank line after return;

mtschoen-unity · 2017-11-21T07:21:42Z

+                    if (visible)
+                        ShowFeedback();
+                    else
+                        this.ClearFeedbackRequests();


So... this doesn't allocate, but it does show up when I'm deep profiling (iterates over all feedback). I don't think it's a major worry but we should avoid spamming this method every frame. Feel free to punt until after this PR.

mtschoen-unity · 2017-11-21T07:22:57Z

+            }
+        }
+
+        List<ActionMenuData> m_Actions;


Do we do anything with actions? Is this just a vestige of RadialMenu? If so, should be removed as part of the not-alternate-menu refactor.

mtschoen-unity · 2017-11-21T07:23:26Z

+
+                m_Visible = value;
+
+                gameObject.SetActive(value);


Careful with this. We might want to kill coroutines before setting active to false.

AndrewTHEManeri

Nothing really to add except the note about using DateTime.

Looks solid, good work

AndrewTHEManeri · 2017-11-21T21:16:17Z

        float m_LastRotationInput;
        MenuHideFlags m_MenuHideFlags = MenuHideFlags.Hidden;
+        float m_RotationInputStartValue;
+        DateTime m_RotationInputStartTime;


Pretty unusual for us to use DateTime, normally we just use float since that is what Unity's Time class gives us

…into features/jono/undo-redo-flick # Conflicts: # Prefabs/UI/Tooltip.prefab

…not get updated

mtschoen-unity · 2017-11-22T01:50:06Z

I did the IAlternateMenu refactor after merging with dev. Dev merge brought up a bug where a tooltip, if it is already visible, will not update its text if it changes and is shown again. Fixed that.

…AssetGridItem, and Delete Action

…th CCU-wrapped call to that fix to fix "Driving RectTransform" undo issue

…ing on the thumb: Undo & AnnotationTool.

…ity-Technologies/EditorVR into features/jono/undo-redo-flick

…objects, not gameObject (EXR root).

…s from IAlternateMenu to IActionsMenu.

…e alternate menus can overlap the Tools Menu; Remove left/right to undo/redo in AnnotationTool; Consume stick press input in LocomotionTool while two-handed-scaling

…ditorVR into features/jono/undo-redo-flick # Conflicts: # Menus/MainMenu/MainMenu.cs # Menus/RadialMenu/RadialMenu.cs # Scripts/Core/EditorVR.Menus.cs # Scripts/Core/EditorVR.cs # Tools/AnnotationTool/AnnotationTool.cs # Tools/AnnotationTool/UserInterface/ColorPickerActivator.cs # Tools/CreatePrimitiveTool/CreatePrimitiveTool.cs # Tools/SelectionTool/SelectionTool.cs

…ding the radial menu

…-Technologies/EditorVR into features/jono/undo-redo-flick # Conflicts: # Scripts/Core/EditorVR.cs

…raw the next one; Remove undo/redo buttons from AnnotationUI

…ditorVR into features/jono/undo-redo-flick # Conflicts: # Menus/MainMenu/MainMenu.cs # Scripts/Core/EditorVR.cs

mtschoen-unity · 2017-12-09T06:57:25Z

Conflicts resolved locally. Performing Github merge.

jono-unity added 13 commits November 14, 2017 18:08

Flick left or right on a hidden radial menu to undo/redo.

e87f0b8

Add UndoMenu as a new menu (no longer in the RadialMenu).

1dd81ec

Merge branch 'development' of https://github.com/Unity-Technologies/E…

101d865

…ditorVR into features/jono/undo-redo-flick

Click joystick in to engage undo, with a delay after releasing, becau…

d4470cd

…se it's hard to keep it pressed in while flicking.

No longer consume stick input in MainMenu, which allows it and UndoMe…

ad56a32

…nu to work together as expected.

Animate undo button alpha on dis/engage.

8316e94

Add animation when undo/redo is performed.

5b40717

Show feedback for hint & undo/redo performed.

14eec1d

Add initial Vive undo/redo control mapping.

a394d0f

Fix undo icon material render queue.

ada2704

Hide the undo menu if radial menu is open, and always show it otherwise.

4d4724e

Vertically flip redo arrow.

a2240eb

Change Vive MainMenu interaction to swipe, rather than click, to rota…

ad16463

…te face.

jono-unity self-assigned this Nov 21, 2017

jono-unity requested review from AndrewTHEManeri, dunity and mtschoen-unity November 21, 2017 01:05

dunity suggested changes Nov 21, 2017

View reviewed changes

mtschoen-unity requested changes Nov 21, 2017

View reviewed changes

Address PR cleanup feedback.

70b0c71

AndrewTHEManeri suggested changes Nov 21, 2017

View reviewed changes

mtschoen-unity added 6 commits November 21, 2017 19:31

Merge branch 'development' of github.com:Unity-Technologies/EditorVR …

a89f134

…into features/jono/undo-redo-flick # Conflicts: # Prefabs/UI/Tooltip.prefab

Replace DateTime.Now with Time.time

1ed8073

Refactor UndoMenu to be an IMenu, not an IAlternateMenu

58f0959

Fix bug in TooltipModule where existing tooltips with new text would …

71ea2f4

…not get updated

Remove unused Actions code from UndoMenu and UndoMenuUI

08236e2

Use this.StopCoroutine instead of regular StopCoroutine in UndoMenu

1c33214

jono-unity and others added 2 commits December 1, 2017 16:24

Add updated Material undo/redo icon.

8d6c18a

Register undo on object creation/destruction in CreatePrimitiveTool, …

f21a1c6

…AssetGridItem, and Delete Action

mtschoen-unity and others added 21 commits December 5, 2017 01:25

Include a patched UnityEngine.CoreModule.dll for 2017.2.0p1, along wi…

b998b4e

…th CCU-wrapped call to that fix to fix "Driving RectTransform" undo issue

Add ICustomAlternateMenu to manage display of additional menus appear…

6ca7437

…ing on the thumb: Undo & AnnotationTool.

Rename DeviceData.alternateMenus to customAlternateMenus.

a848508

Don't process Undo input if menu is hidden.

984e599

change how we add to the menuhide dict in devicedata

db6f51e

Merge branch 'features/jono/undo-redo-flick' of https://github.com/Un…

ba394cf

…ity-Technologies/EditorVR into features/jono/undo-redo-flick

Fix EXR lockup on hiding AnnotationTool, by hiding AnnotationTool UI …

c4a3d97

…objects, not gameObject (EXR root).

Fix AnnotationTool menuHideFlag logic.

29e2de5

Add namespaces to AnnotationTool scripts.

334db27

WIP ICustomAlternateMenu -> IAlternateMenu refactor, and split action…

f055f52

…s from IAlternateMenu to IActionsMenu.

Fix issue where undo menu isn't hidden by BrushSizeUI; Fix issue wher…

d8d18a8

…e alternate menus can overlap the Tools Menu; Remove left/right to undo/redo in AnnotationTool; Consume stick press input in LocomotionTool while two-handed-scaling

Fix line endings; Remove IActionsMenu from IMainMenu

b5538da

Fix issue where tools menu didn't return to its initial state when hi…

9e0fa05

…ding the radial menu

Merge branch 'bugfixes/schoen/undo-patch' of https://github.com/Unity…

f09bc95

…-Technologies/EditorVR into features/jono/undo-redo-flick # Conflicts: # Scripts/Core/EditorVR.cs

Fix null reference exception when undoing annotations and trying to d…

99378d5

…raw the next one; Remove undo/redo buttons from AnnotationUI

Fix issue where two handed scaling broke alternate menu hiding

60edbee

Fix line endings

89084cd

Code cleanup and style changes

68eea66

Rename customAlternateMenu to alternateMenu

566c516

Clean up unused code and fix style; Fix UndoMenu visibility

f71e815

mtschoen-unity approved these changes Dec 9, 2017

View reviewed changes

Merge branch 'development' of https://github.com/Unity-Technologies/E…

0d8c9f6

…ditorVR into features/jono/undo-redo-flick # Conflicts: # Menus/MainMenu/MainMenu.cs # Scripts/Core/EditorVR.cs

mtschoen-unity force-pushed the features/jono/undo-redo-flick branch from fcbba41 to 0d8c9f6 Compare December 9, 2017 06:33

dunity approved these changes Dec 9, 2017

View reviewed changes

mtschoen-unity merged commit d43614e into development Dec 9, 2017

mtschoen-unity deleted the features/jono/undo-redo-flick branch December 9, 2017 06:57

mtschoen-unity added this to the 0.1.1 milestone Dec 12, 2017

mtschoen-unity mentioned this pull request Feb 22, 2019

Undo/redo only appear if object is selected #204

Closed


		public Node node { get; set; }

		public event Action<Transform> itemWasSelected;


		readonly BindingDictionary m_Controls = new BindingDictionary();

		public Transform rayOrigin { private get; set; }

Uh oh!

Conversation

jono-unity commented Nov 21, 2017

Uh oh!

dunity left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtschoen-unity left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndrewTHEManeri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtschoen-unity commented Nov 22, 2017

Uh oh!

mtschoen-unity commented Dec 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dunity left a comment •

edited

Loading