Skip to content

Conversation

@jfreire-unity
Copy link
Collaborator

@jfreire-unity jfreire-unity commented Dec 15, 2023

Description

ISX-1528

Changes made

  • Change InputSystemProvider for Input For UI module to work with the Project Wide Actions instead of DefaultInputActions. This makes UI TK use project-wide action bindings that can be modified by users.

  • Added some warnings when the asset is saved with changes to the "UI" action maps. By doing this only in save, it makes sure that other parts of the code are decoupled from Project Wide Actions.

  • Should there be a way to reset specific action maps? Besides having just "Reset", also have "Reset UI map" and "Reset Player map" as well?
  • Some might prefer to have an Editor UI visual warning (e.g. like a warning icon) when the UI action map is modified. But that would take more time than expected, as it involves UI TK UI changes. Please let me know if there should be any extra effort to change the UI.

Notes

ℹ️ The project to be used for QA is specified in the JIRA ticket.

I'll add doc changes in another PR after talking with Ben Pitt.

Checklist

Before review:

  • Changelog entry added.
    • Explains the change in Changed, Fixed, Added sections.
    • For API change contains an example snippet and/or migration example.
    • FogBugz ticket attached, example ([case %number%](https://issuetracker.unity3d.com/issues/...)).
    • FogBugz is marked as "Resolved" with next release version correctly set.
  • Tests added/changed, if applicable.
    • Functional tests Area_CanDoX, Area_CanDoX_EvenIfYIsTheCase, Area_WhenIDoX_AndYHappens_ThisIsTheResult.
    • Performance tests.
    • Integration tests.
  • Docs for new/changed API's.
    • Xmldoc cross references are set correctly.
    • Added explanation how the API works.
    • Usage code examples added.
    • The manual is updated, if needed.

During merge:

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

@jfreire-unity jfreire-unity force-pushed the isx-1528-inputforui-with-project-wide-actions branch from 0856332 to 0bdd4a6 Compare December 15, 2023 12:54
Copy link
Collaborator

@ritamerkl ritamerkl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Collaborator

@ekcoh ekcoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good in general. I think we can skip the caching or was it taking a long time that drived the decision to cache in s_DefaultUIMap?

I also believe it would be good to add a test to cover CheckFirDefaultUIActionMapChanges.

Copy link
Collaborator

@ekcoh ekcoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another one, CHANGELOG.md entry seems to be missing.

@jamesmcgill
Copy link
Collaborator

I think for the first pass, it is enough that InputForUI just uses InputSystem.actions.

UI improvements like blocking editing the UI action names, warnings, or resetUI map, would take more time.

@jfreire-unity
Copy link
Collaborator Author

jfreire-unity commented Dec 21, 2023

@Pauliusd01 regarding testing, you can try to play around with removing the UI action map, UI actions, or changing the bindings like we discussed in the daily sync.
image

You should still see the events in the right column the Visualizer test project scene. And should get appropriate warning and error messages. There are already some warning messages once the asset is saved and you rename the UI action map and its actions.

@jfreire-unity
Copy link
Collaborator Author

@Pauliusd01 I put the wrong image before, I updated it now

@jfreire-unity jfreire-unity marked this pull request as ready for review December 21, 2023 16:37
@jfreire-unity jfreire-unity requested review from ekcoh and removed request for ekcoh January 10, 2024 11:09
Copy link
Collaborator

@ekcoh ekcoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general looks good to me, happy to see some test coverage added. Two minor comments that I believe can improve the PR slightly.

## [Unreleased]

### Changed
- UI toolkit now uses the "UI" action map of project-wide actions as their default input actions. Removing bindings or renaming actions will break UI input for UI toolkit.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is expected given the solution we have previously discussed but its unfortunate since the action is basically required (driven by the implementation). In the long run I believe we should consider design changes that would allow the integration to own the action while the editor would allow reconfiguring it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the CHANGED message I believe it could be more clear to users if you added the delta to the description, e.g. "UI toolkit now uses the "UI" action map of project-wide actions as their default input actions instead of..." basically highlighting the switch from hard-coded to customisable.

@jfreire-unity jfreire-unity force-pushed the isx-1528-inputforui-with-project-wide-actions branch from 3f15852 to a79a613 Compare January 10, 2024 13:23
@jfreire-unity jfreire-unity merged commit 3f96d05 into develop Jan 10, 2024
@jfreire-unity jfreire-unity deleted the isx-1528-inputforui-with-project-wide-actions branch January 10, 2024 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants