Create layout interactive elements #1165

brean · 2017-10-15T19:54:19Z

New, small Example for Dynamic Buttons, see #1137

StephenHodgson

looks good!

just a few comments

StephenHodgson · 2017-10-17T14:43:29Z

Assets/HoloToolkit-Examples/UX/Scripts/ExampleData/NoteDataProvider.cs

+                return;
+            }
+            int interactivePos = SourceSet.SelectedIndices[0];
+            string title = SourceSet.Interactives[interactivePos].gameObject.GetComponent<LabelTheme>().Default;


Anytime you use GetComponent<T> you should always check if it's null before using it's returned value.

StephenHodgson · 2017-10-17T14:44:35Z

Assets/HoloToolkit-Examples/UX/Scripts/InteractiveGroup.cs

+                );
+                interactive.gameObject.transform.localPosition = new Vector3(
+                    ((i / rows) - ((columns - 1) / 2f)) * Distance.x,
+                    -(j - (rows - 1) / 2f) * Distance.y);


Instad of value / 2 we can use value * 0.5f

I think value / 2 is better for (human) readability and I don't think the CPU will mind (its not like we are doing this loop for thousands of objects). Or is there a coding convention I am missing?

Idk, as a human, I've always hated fractions.

as for CPU perf, gotta remember we're also targeting HoloLens, and it's a mobile device and every little bit counts.

StephenHodgson · 2017-10-17T14:46:53Z

Assets/HoloToolkit-Examples/UX/Scripts/InteractiveSet.cs

@@ -22,7 +22,7 @@ public class InteractiveSet : MonoBehaviour
        public SelectionType Type = SelectionType.single;

        [Tooltip("Interactives that will be managed by this controller")]
-        public InteractiveToggle[] Interactives;
+        public List<InteractiveToggle> Interactives;


What's the advantage of changing this to a list?

It's easier to remove items in a list, see the "RemoveInteractive"-method in the same file. If the data changes I don't delete all Interactives and their prefabs, only the delta and update all items afterwards (recycling objects).

StephenHodgson · 2017-10-17T14:49:21Z

Assets/HoloToolkit-Examples/UX/Scripts/InteractiveGroup.cs

+            LabelTheme lblTheme = gameObject.GetComponent<LabelTheme>();
+            if (lblTheme == null)
+            {
+                Debug.LogError("No LabelTheme attached to this Interactive");


We could probably add a component requirement, and always get the component if null, and remove the debug.

brean · 2017-10-19T19:55:34Z

@StephenHodgson I implemented your feedback. Please review.

StephenHodgson · 2017-10-19T22:08:24Z

Assets/HoloToolkit-Examples/UX/Scripts/InteractiveGroup.cs

@@ -102,6 +99,10 @@ public void UpdateData()
                // layouting
                int j = i % rows;
                Collider collider = interactive.gameObject.GetComponent<Collider>();
+                if (collider == null)


Should we also add Collider to the list of required components?

That would be the Collider in InteractiveToggle. I can attach the InteractiveToggle-script to any GameObject and have the collider in some child of that GameObject, so I don't think having it as required component is a good idea... we can however ignore the collider boundary size for layouting if it is not present. If you don't have a collider attached directly the user can define the distance between the InteractiveToggles by setting the Offsets-values. Alternatively I could iterate over transform.children recursively and calculate the biggest bounding box, but that seems a bit overkill.

StephenHodgson · 2017-10-19T22:09:23Z

Assets/HoloToolkit-Examples/UX/Scripts/InteractiveGroup.cs

+                if (InterInst != null)
+                {
+                    Interactives.Add(InterInst);
+                }


Should we throw a warning if our prefab doesn't have the InteractiveToggle telling the dev to add one to their prefab?

StephenHodgson · 2017-10-19T22:10:29Z

Assets/HoloToolkit-Examples/UX/Scripts/ExampleData/NoteDataProvider.cs

+            {
+                TargetGroup.Titles = Data[label.Default];
+                TargetGroup.UpdateData();
+            }


Should we add a warning if our object doesn't have a LabelTheme telling our dev to add one to the object?

Yes. The Dev would most likely implement his own "DataProvider", where he gets some data from an external source and than acts on it accordingly, putting that data into InteractiveSets/InteractiveGroups as he likes. So I also added some documentation that descibes it a bit more.

…eToggle and add some more documentation

brean · 2017-10-24T11:16:38Z

@StephenHodgson I am done integrating your feedback, please review this.

StephenHodgson · 2017-10-24T15:29:23Z

Assets/HoloToolkit-Examples/UX/Scripts/ExampleData/NoteDataProvider.cs

+        // some test data - imagine this comming from a web-service 
+        // or some input menu
+        public Dictionary<string, List<string>> Data = 
+            new Dictionary<string, List<string>>


Let's keep all of this on one line, and updated the comments with summary tags

StephenHodgson · 2017-10-24T15:29:35Z

Assets/HoloToolkit-Examples/UX/Scripts/InteractiveGroup.cs

+
+        public Vector2 Offsets = new Vector2(0.00f, 0.00f);
+
+        void Start()


nit: private

StephenHodgson · 2017-10-24T15:29:48Z

Assets/HoloToolkit-Examples/UX/Scripts/InteractiveGroup.cs

+        /// remove unused Interactives from scene
+        /// </summary>
+        /// <param name="keep">number of Iteractives that will NOT be deleted</param>
+        void RemoveInteractives(int keep = 0)


nit: private

brean · 2017-10-24T18:35:28Z

@StephenHodgson done.

SimonDarksideJ

For the changes in this PR, everything is good.

However, it does highlight an error logging issue @StephenHodgson whereby, repeared ERROR logs are generated when speech recog isn't available. Will need fixing in another PR.

brean · 2017-11-02T12:29:35Z

I like to see this merged and it has been approved by @StephenHodgson and @DDReaper . Is there anything I can do to speed up the process? Should I take a deeper look at the Error messages @DDReaper mentioned?

SimonDarksideJ · 2017-11-02T13:08:15Z

The issue is a separate one, not related to this PR, so it doesn't need fixing here.

brean added 3 commits October 14, 2017 23:35

Create and Layout Interactive elements based on data

596eea0

Documentation and minor code cleanup

a1f7288

Merge branch 'master' into CreateLayoutInteractiveElements

5e52fb9

StephenHodgson reviewed Oct 17, 2017

View reviewed changes

brean added 3 commits October 19, 2017 21:27

Merge branch 'master' into CreateLayoutInteractiveElements

23fbfee

always check for null when using GetComponent

fc04bc9

avoid fractions (use '* 0.5' instead of '/ 2')

2e287e3

StephenHodgson reviewed Oct 19, 2017

View reviewed changes

brean added 5 commits October 20, 2017 10:44

Remove unused SetTitle

bad9cc1

Show warning if LabelTheme is not attached to the selected Interactiv…

16d41e3

…eToggle and add some more documentation

ignore collider bounds if collider is not present

2c6adba

add warning if prefab does not have an InteractiveToggle

d935d81

Merge branch 'master' into CreateLayoutInteractiveElements

d98360a

StephenHodgson reviewed Oct 24, 2017

View reviewed changes

brean added 2 commits October 24, 2017 20:19

Merge branch 'master' into CreateLayoutInteractiveElements

7993825

code readability: add summary tags and remove line breaks

023af11

SimonDarksideJ approved these changes Oct 26, 2017

View reviewed changes

StephenHodgson approved these changes Oct 29, 2017

View reviewed changes

StephenHodgson merged commit 7320519 into microsoft:master Nov 2, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create layout interactive elements #1165

Create layout interactive elements #1165

brean commented Oct 15, 2017

StephenHodgson left a comment

StephenHodgson Oct 17, 2017

StephenHodgson Oct 17, 2017

brean Oct 17, 2017

StephenHodgson Oct 17, 2017

StephenHodgson Oct 17, 2017

brean Oct 17, 2017

StephenHodgson Oct 17, 2017

brean Oct 17, 2017

brean commented Oct 19, 2017

StephenHodgson Oct 19, 2017

brean Oct 20, 2017

StephenHodgson Oct 19, 2017

brean Oct 20, 2017

StephenHodgson Oct 19, 2017 •

edited

brean Oct 20, 2017

brean commented Oct 24, 2017

StephenHodgson Oct 24, 2017

StephenHodgson Oct 24, 2017

StephenHodgson Oct 24, 2017

brean commented Oct 24, 2017

SimonDarksideJ left a comment

brean commented Nov 2, 2017

SimonDarksideJ commented Nov 2, 2017


		public Vector2 Offsets = new Vector2(0.00f, 0.00f);

		void Start()

Create layout interactive elements #1165

Create layout interactive elements #1165

Conversation

brean commented Oct 15, 2017

StephenHodgson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brean commented Oct 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephenHodgson Oct 19, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brean commented Oct 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brean commented Oct 24, 2017

SimonDarksideJ left a comment

Choose a reason for hiding this comment

brean commented Nov 2, 2017

SimonDarksideJ commented Nov 2, 2017

StephenHodgson Oct 19, 2017 •

edited