Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX: Prefabs and missing default control scheme in the inspector view of playerinput (ISXB-818) #1932

Merged
merged 5 commits into from
May 27, 2024

Conversation

bmalrat
Copy link
Collaborator

@bmalrat bmalrat commented May 21, 2024

Description

Fixed Prefabs and missing default control scheme used by PlayerInput component are now correctly shown in the inspector ISXB-818.
Before the change previously defined PlayerInput's default control scheme that doesn't exist exist any more are not shown and produce some error when entering in playmode. Disabling/enabling component silently remove the lost control scheme.
Also Default Scheme, Auto-Switch,Default Map fields was not well displayed(no blue override color and no context actions) for prefabs.

Changes made

Changed the PlayerInput's inspector the show the missing default control scheme with a red background and a suffix.
Capture d'écran 2024-05-21 093655
The missing one is always in second position after and others schemes are now sorted in the popup.

Changed the ways that Default Scheme, Auto-Switch,Default Map are displayed to support prefabs.
Capture d'écran 2024-05-24 160723

Notes

Please write down any additional notes, remove the section if not applicable.

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.

@bmalrat bmalrat requested review from ekcoh and Pauliusd01 May 21, 2024 13:50
@bmalrat bmalrat changed the title FIX: Missing default control scheme in the inspector view of playerinput FIX: Missing default control scheme in the inspector view of playerinput (ISXB-818) May 21, 2024
@Pauliusd01
Copy link
Collaborator

This seems to mess with scheme order even when there are no missing schemes in the list (In the video I compare your PR with the current develop)
https://github.com/Unity-Technologies/InputSystem/assets/54306142/2b904247-bee1-44cc-a3ae-3e43cbd17c9e

@bmalrat
Copy link
Collaborator Author

bmalrat commented May 22, 2024

This seems to mess with scheme order even when there are no missing schemes in the list (In the video I compare your PR with the current develop) https://github.com/Unity-Technologies/InputSystem/assets/54306142/2b904247-bee1-44cc-a3ae-3e43cbd17c9e

I sorted them alphabetically like mentioned in that comment ////TODO: sort alphabetically
I can remove the sort and/or the sort the scheme editor windows

@Pauliusd01
Copy link
Collaborator

Pauliusd01 commented May 22, 2024

This seems to mess with scheme order even when there are no missing schemes in the list (In the video I compare your PR with the current develop) https://github.com/Unity-Technologies/InputSystem/assets/54306142/2b904247-bee1-44cc-a3ae-3e43cbd17c9e

I sorted them alphabetically like mentioned in that comment ////TODO: sort alphabetically I can remove the sort and/or the sort the scheme editor windows

In my opinion It should be the same list both between the player component and the actual actions window. So probably best to not do any extra sorting in the component at all, if a missing scheme is selected it will already be quite visible by the red highlight. But maybe Hakan thinks otherwise

@bmalrat
Copy link
Collaborator Author

bmalrat commented May 22, 2024

This seems to mess with scheme order even when there are no missing schemes in the list (In the video I compare your PR with the current develop) https://github.com/Unity-Technologies/InputSystem/assets/54306142/2b904247-bee1-44cc-a3ae-3e43cbd17c9e

I sorted them alphabetically like mentioned in that comment ////TODO: sort alphabetically I can remove the sort and/or the sort the scheme editor windows

In my opinion It should be the same list both between the player component and the actual actions window. So probably best to not do any extra sorting in the component at all, if a missing scheme is selected it will already be quite visible by the red highlight. But maybe Hakan thinks otherwise

I removed the sort. If @ekcoh prefer with it I will revert the last change

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.

LGTM, added some questions just to make sure I understand the change correctly.

@@ -15,6 +15,7 @@ however, it has to be formatted properly to pass verification tests.
- Fixed an issue where a composite binding would not be consecutively triggered after ResetDevice() has been called from the associated action handler [ISXB-746](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-746).
- Fixed resource designation for "d_InputControl" icon to address CI failure.
- Fixed an issue where a composite binding would not be consecutively triggered after disabling actions while there are action modifiers in progress [ISXB-505](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-505).
- Fixed Missing default control scheme used by PlayerInput component are now correctly shown in the inspector [ISXB-818](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-818)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Am I correct to assume the behavior with a missing control scheme is equivalent to having it set to Any (ignoring Inspector representation)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like this is mainly targeting representation of control scheme when opening a project/asset with a previously configured control scheme that no longer exist, or e.g. deleting a control scheme.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Am I correct to assume the behavior with a missing control scheme is equivalent to having it set to Any (ignoring Inspector representation)?

yes

@ekcoh
Copy link
Collaborator

ekcoh commented May 24, 2024

I think we can skip sort. Consistency between different places representing the same thing makes sense.

@Pauliusd01
Copy link
Collaborator

Pauliusd01 commented May 24, 2024

Sorry, had to change the status. I took a look at the original bug report and noticed that action map specific fields do not dirty the prefab is this not what the user had trouble with? Are we truly fixing the issue properly here?

@Pauliusd01 Pauliusd01 self-requested a review May 24, 2024 12:46
@Pauliusd01
Copy link
Collaborator

Pauliusd01 commented May 24, 2024

Notice how none of the action specific fields get the blue highlights next to them when they're changed, so if the user wants to override their prefab they can't. The only way is to go into prefab mode and change it there.

Unity_2024-05-24_15-47-23.mp4

@bmalrat
Copy link
Collaborator Author

bmalrat commented May 24, 2024

Sorry, had to change the status. I took a look at the original bug report and noticed that action map specific fields do not dirty the prefab is this not what the user had trouble with? Are we truly fixing the issue properly here?

thanks for the highlight

@bmalrat bmalrat changed the title FIX: Missing default control scheme in the inspector view of playerinput (ISXB-818) FIX: Prefabs and missing default control scheme in the inspector view of playerinput (ISXB-818) May 24, 2024
@bmalrat bmalrat force-pushed the isxb-818-fix-for-missing-default-control-scheme branch from 947c367 to 0b03772 Compare May 24, 2024 20:14
@bmalrat bmalrat requested a review from ekcoh May 27, 2024 12:28
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.

LGTM

@bmalrat bmalrat merged commit cbf4058 into develop May 27, 2024
77 of 79 checks passed
@bmalrat bmalrat deleted the isxb-818-fix-for-missing-default-control-scheme branch May 27, 2024 15:14
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.

3 participants