Add UI Toolkit matching control bindings UI #1835

lyndon-unity · 2024-02-08T10:56:49Z

Description

Implemented UI TK version of the matching control bindings recently added to the IMGUI editor
#1672

Example of it working in the project wide input actions. This is the same UX used in the individual assets.

Changes made

The code to populate the UX is the same as the IMGUI code - but split out to generate a tree before then generating the UI specific tree.

Also fixed up some styling on the control scheme info which is in the binding UI to be more consistent with the IMGUI version. Bolded and fixed margins.

Notes

Note this version doesn't store the full tree state - just the expanded/not level.
This means I didn't need to add the static dictionary which was present in the IMGUI version.
I didn't feel this was critical and reduced static state.

I also had to make the top level expansion based on e checkbox rather than a 'fold out' / top of tree as the UI TK treeView doesn't have API to react to the expand/collapse state.
^ This comment is old - I've added a Foldout to replace the Toggle now

Checklist

Before review:

Changelog entry added.
No tests added/changed
No new doc

During merge:

Commit message for squash-merge is prefixed with one of the list:
- NEW: ___.
- FIX: ___.
- DOCS: ___.
- CHANGE: ___.
- RELEASE: 1.1.0-preview.3.

Tightened up layout of control scheme text and the matching paths tree view labels Removed the 'show matching paths' checkbox Attempted to save and restore expansion of the 'Derived Paths' but waiting on feedback on how to respond to expand/collapse from UI TK team.

Moved some more of the style to uss file Changed a label to a HelpBox to match UGUI version Switched to toggle/checkbox for the main "Show matching bindings" part as we can't detect expand/collapse in tree Removed an unused callback Renamed some functions to be clearer

ritamerkl

Good to see this working!
I just have one major point- I think it could be worth outsourcing the biggest part of the DrawMatchingControlPath and connected methods to an external script (or other related existing classes). Like that we could keep the PropertiesView clean and "logic free".
I also added one minor comment on a potential unused method to remove.

...ages/com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/Views/BindingPropertiesView.cs

lyndon-unity · 2024-02-08T14:32:53Z

Good to see this working! I just have one major point- I think it could be worth outsourcing the biggest part of the DrawMatchingControlPath and connected methods to an external script (or other related existing classes). Like that we could keep the PropertiesView clean and "logic free". I also added one minor comment on a potential unused method to remove.

Yes I agree I can take a look at spitting those out

ritamerkl

LGTM now

Pauliusd01 · 2024-02-09T11:37:59Z

Is the style used wrong? Why is the derived binding text selectable? It wasn't like that in the IMGUI version and I don't see why that should be changed

Pauliusd01 · 2024-02-09T11:48:21Z

Unity_2024-02-09_13-47-02.mp4

Since this is a toggle now I think its state should be saved, seems annoying to have to re-enable it between window restarts

Pauliusd01

Just minor notes from me. Functionality seems the same and works without issues

So like IMGUI version

lyndon-unity · 2024-02-09T17:49:03Z

Is the style used wrong? Why is the derived binding text selectable? It wasn't like that in the IMGUI version and I don't see why that should be changed

I've fixed that - it was just the default for the UI TK tree view.
I've set it to none to match IMGUI version

lyndon-unity · 2024-02-09T17:49:48Z

I think its state should be saved

I'd like to do that but I don't have a quick fix for that so might land the update so far and look at that later.

Pauliusd01 · 2024-02-12T12:21:02Z

I think its state should be saved

I'd like to do that but I don't have a quick fix for that so might land the update so far and look at that later.

Does it have to be a toggle then? If a foldout would solve this issue I'd prefer that, and later on we can switch it to a toggle once state saving is there

…eopen

lyndon-unity · 2024-02-12T15:32:04Z

I think its state should be saved

I'd like to do that but I don't have a quick fix for that so might land the update so far and look at that later.

Does it have to be a toggle then? If a foldout would solve this issue I'd prefer that, and later on we can switch it to a toggle once state saving is there

State is now maintained over open/close to match IMGUI.
I've just switched to a foldout now too.

ekcoh

LGTM, is all logic in MatchingControlPaths new? If it is then its fine as-is, otherwise it might be worthwhile avoiding duplicating logic with existing feature implementation for IMGUI? It seems like MatchingControlPaths is not tied to UI framework and would be possible to cover with unit tests?

lyndon-unity · 2024-02-12T21:11:25Z

LGTM, is all logic in MatchingControlPaths new? If it is then its fine as-is, otherwise it might be worthwhile avoiding duplicating logic with existing feature implementation for IMGUI? It seems like MatchingControlPaths is not tied to UI framework and would be possible to cover with unit tests?

The MatchingControlPaths isn't all new code - its adapted from the IMGUI version. It drops some of the code for retaining the expand/collapse state of individual parts of the hierarchy as that had a static dictionary that seemed lower priority for the memory overheads. To use in IMGUI directly we would have to add back in the static dictionary and store extra state in the returned data. I decided to duplicate as the IMGUI version would be deprecated in future.
^ but I have now removed duplicated code and refactored a little so it can be shared with IMGUI and UI TK.

Tests should be added but I felt given time constraints that part could be added later.

…d UITK editors And fixed the foldout callback to only trigger on the foldout and not its children

…ons)

… define

Pauliusd01

LGTM, checked that the matching bindings are correct for different bindings in both ProjectWideActions and custom action assets.

ekcoh · 2024-02-13T08:38:38Z

LGTM, is all logic in MatchingControlPaths new? If it is then its fine as-is, otherwise it might be worthwhile avoiding duplicating logic with existing feature implementation for IMGUI? It seems like MatchingControlPaths is not tied to UI framework and would be possible to cover with unit tests?

The MatchingControlPaths isn't all new code - its adapted from the IMGUI version. It drops some of the code for retaining the expand/collapse state of individual parts of the hierarchy as that had a static dictionary that seemed lower priority for the memory overheads. To use in IMGUI directly we would have to add back in the static dictionary and store extra state in the returned data. I decided to duplicate as the IMGUI version would be deprecated in future. ^ but I have now removed duplicated code and refactored a little so it can be shared with IMGUI and UI TK.

Tests should be added but I felt given time constraints that part could be added later.

I see, great to avoid duplicating too much. OK to postpone the test part temporarily but in that case please file a ticket on the remaining part from your perspective.

lyndon-unity added 4 commits February 7, 2024 16:27

Add 'matching controls' into tree view of new UI TK action asset UI

176ccd1

Tightened up layout

bd47800

Tightened up layout of control scheme text and the matching paths tree view labels Removed the 'show matching paths' checkbox Attempted to save and restore expansion of the 'Derived Paths' but waiting on feedback on how to respond to expand/collapse from UI TK team.

Updated change log and fixed formatting

fe2415b

lyndon-unity requested review from Billreyn, Pauliusd01, RogPodge and ritamerkl February 8, 2024 10:56

Pauliusd01 changed the title ~~Add UI Toolkit matching control bindings UX~~ Add UI Toolkit matching control bindings UI Feb 8, 2024

ritamerkl reviewed Feb 8, 2024

View reviewed changes

...ages/com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/Views/BindingPropertiesView.cs Outdated Show resolved Hide resolved

Removed unused function

2d503d2

Split out MatchingControlPaths to separate source file

f286523

lyndon-unity requested a review from ritamerkl February 8, 2024 14:58

ritamerkl approved these changes Feb 8, 2024

View reviewed changes

Pauliusd01 requested changes Feb 9, 2024

View reviewed changes

Can no longer select items in the matching controls tree

ea0c67c

So like IMGUI version

Pauliusd01 self-requested a review February 12, 2024 12:06

lyndon-unity added 2 commits February 12, 2024 15:21

Maintaining state for showing derived binding over editor close and r…

0724351

…eopen

Switched from checkbox to fold out

d339aa4

Formatting fix and moved one class internal;

a5c99a4

ekcoh approved these changes Feb 12, 2024

View reviewed changes

Removed some duplicated code and reused matching controls in IMGUI an…

0c112d4

…d UITK editors And fixed the foldout callback to only trigger on the foldout and not its children

lyndon-unity added 3 commits February 12, 2024 22:43

Merge branch 'develop' into add_ui_tk_matching_controls

50a1b37

Fixed issue when only IMGUI editor active (no project wide input acti…

fe2ab01

…ons)

UI TK specific elements for project wide input actions protected with…

5006f89

… define

Pauliusd01 approved these changes Feb 13, 2024

View reviewed changes

Formatting fix

417dfe3

Merge branch 'develop' into add_ui_tk_matching_controls

d2d7538

lyndon-unity merged commit d46ba1a into develop Feb 14, 2024

lyndon-unity deleted the add_ui_tk_matching_controls branch February 14, 2024 08:31

Add UI Toolkit matching control bindings UI #1835

Add UI Toolkit matching control bindings UI #1835

Uh oh!

Conversation

lyndon-unity commented Feb 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes made

Notes

Checklist

Uh oh!

ritamerkl 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!

Uh oh!

lyndon-unity commented Feb 8, 2024

Uh oh!

ritamerkl left a comment

Choose a reason for hiding this comment

Uh oh!

Pauliusd01 commented Feb 9, 2024

Uh oh!

Pauliusd01 commented Feb 9, 2024

Uh oh!

Pauliusd01 left a comment

Choose a reason for hiding this comment

Uh oh!

lyndon-unity commented Feb 9, 2024

Uh oh!

lyndon-unity commented Feb 9, 2024

Uh oh!

Pauliusd01 commented Feb 12, 2024

Uh oh!

lyndon-unity commented Feb 12, 2024

Uh oh!

ekcoh left a comment

Choose a reason for hiding this comment

Uh oh!

lyndon-unity commented Feb 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Pauliusd01 left a comment

Choose a reason for hiding this comment

Uh oh!

ekcoh commented Feb 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lyndon-unity commented Feb 8, 2024 •

edited

Loading

ritamerkl left a comment •

edited

Loading

lyndon-unity commented Feb 12, 2024 •

edited

Loading