-
Notifications
You must be signed in to change notification settings - Fork 330
FIX: DefaultActionMap dropdown set to <None> by default (ISXB-1559) #2194
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
Conversation
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## develop #2194 +/- ##
===========================================
- Coverage 67.80% 67.79% -0.01%
===========================================
Files 367 367
Lines 53526 53528 +2
===========================================
Hits 36291 36291
- Misses 17235 17237 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
Can new automated tests be added for this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, correct map is picked and kept between play modes when adding fresh assets to player input, switching them out, modifying the maps on an already present one, etc.
The only odd thing I noticed was that when a PWA asset exists and you create a player input component it pre-picks the PWA asset to it. Which is known and probably getting changed but the weird part is that it still pre-picks the None map. Not like it matters as that setting isn't respected with PWA assets at the moment but that was the only odd one out scenario
…ap-dropdown-none # Conflicts: # Packages/com.unity.inputsystem/CHANGELOG.md # Packages/com.unity.inputsystem/InputSystem/Plugins/PlayerInput/PlayerInputEditor.cs
I think that should be changed anyway since using the PWA asset for PlayerInput is not recommended - do we have a ticket to remove the automated prefill with the PWA asset? |
Description
Like described in this ticket, the default ActionMap dropdown was set to by default, this PR fixes this by setting the first ActionMap as a default option on changing/first adding the Action Asset.
Testing status & QA
Manual testing done.
Overall Product Risks
Please rate the potential complexity and halo effect from low to high for the reviewers. Note down potential risks to specific Editor branches if any.
Comments to reviewers
Reviewers:
I renamed the OnActionAssetChange() method since this also happens as soon as the component window needs to be re-initialised after navigating away from the view, since the dependent properties are not static.
I edited the CheckIfActionAssetChanged() method, since it's not changing if the property just lost the reference when navigating away from the window.
For testing:
Checklist
Before review:
Changed,Fixed,Addedsections.Area_CanDoX,Area_CanDoX_EvenIfYIsTheCase,Area_WhenIDoX_AndYHappens_ThisIsTheResult.During merge:
NEW: ___.FIX: ___.DOCS: ___.CHANGE: ___.RELEASE: 1.1.0-preview.3.After merge: