-
Notifications
You must be signed in to change notification settings - Fork 329
FIX: InputActionMap warnings when the UI map is not setup correctly (ISXB-1560) #2229
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: InputActionMap warnings when the UI map is not setup correctly (ISXB-1560) #2229
Conversation
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.
Thanks for looking into this. Some comments and suggestions....
Packages/com.unity.inputsystem/InputSystem/Plugins/InputForUI/InputActionAssetVerifier.cs
Outdated
Show resolved
Hide resolved
Packages/com.unity.inputsystem/InputSystem/Plugins/InputForUI/InputActionAssetVerifier.cs
Outdated
Show resolved
Hide resolved
|
|
||
| // Check if the map (if any) exists | ||
| var noMapOrMapExists = true; | ||
| if (asset.actionMaps.Count == 0) |
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.
I do not understand why you are checking this? This code is trying to verify a single action (it's the intended purpose of the method). This method should not verify the asset, only the action within the asset. Additionally, based on the comments in the JIRA case, it seems like the desired outcome is to not generate all warnings when there is no "UI" map (relying on defaults), and not in the general case. If this is the goal with the fix is to disable all action verification when there is no UI map I suggest we remove this and instead add an early return to line #35 in this file. That early return I believe should also check for the presence of an "UI" action map and not check action map count in general. E.g.
if (asset.FindActionMap("UI", false) == null) return;
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.
Also consider adding a unit test (do not remember if tests for this class exist or not that may be extended) to avoid future regression.
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.
Ok got it, I think misunderstood this one, I went by what Paulius said but I didn't understand the first time round from reading the comment. I'll try to check with him to confirm.
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.
Yes, you are right! I understood from the ticket when there is nothing mapped in the input file, when it should have been no UI map there.
I made the change you suggested and now it seems to behave as expected fixing Paulius' issue below also.
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.
I have now added two unit tests also to test both that warnings are thrown when there are actions missing from the UI action map.
And another so that we do not have any warnings when no UI map exists.
|
I guess the tests checkbox and docs checkbox should not be checked since the PR isn't currently touching any of those areas? |
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## develop #2229 +/- ##
========================================
Coverage 68.14% 68.14%
========================================
Files 367 367
Lines 53661 53661
========================================
Hits 36567 36567
Misses 17094 17094
Flags with carried forward coverage won't be shown. Click here to find out more. see 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
This comment was marked as resolved.
This comment was marked as resolved.
I actually though initially I shouldn't need to add tests so checked it as I thought about it. But I saw you left a comment to add a test so I can look into that! |
|
@Pauliusd01, it wouldn't let me quote reply for some reason. Edit: Nevermind, it was due to my misunderstanding Hakan pointed out. Now everything should work. I pushed a fix. |
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. Checked that the warning is no longer shown when UI map is missing and that it is still shown when the map is partially incomplete. Also, double checked that game UI (UGUI and UIToolkit) still works properly without having a UI map in the project wide actions asset and it does, the warning really was unnecessary in this scenario.
…ctions are missing from the UI action map.
| LogAssert.Expect(LogType.Warning, new Regex($"^InputAction with path 'UI/ScrollWheel' in asset '{link}' could not be found.")); | ||
| } | ||
| // else: expect suppression of child errors | ||
| LogAssert.NoUnexpectedReceived(); |
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.
Had an issue here with Teardown logging the warnings again causing the logs to be there twice and fail.
If you think I should do this another way let me know.
| { | ||
| var asset = ProjectWideActionsAsset.CreateDefaultAssetAtPath(kAssetPath); | ||
| var uiActionMap = asset.FindActionMap("UI", true); | ||
| for (int i = uiActionMap.m_Actions.Length - 1; i >= 0; i--) |
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.
I tried using Clear here instead but it didn't work / caused exceptions.
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.
Not sure what you try to test here? Based on the name it sounds like it's a duplicate of my previous test ActionMapWithNonExistentRequiredAction_ShouldGenerateWarning? Maybe you are good with any additional tests? (Check test coverage)
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.
Maybe this should just go?
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.
Ah, in this case. It's similar to your old test but the difference is we leave the UI map in and delete the contents.
This should show warnings as me & Paulius talked about this, the default is not used in a case where the UI map is present but not containing necessary actions.
Which is what I test for here.
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 now. Good work!
|
One small thing, you need to rerun formatter on this PR to get CI green. |
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
|
@Darren-Kelly-Unity can you make the PR name more descriptive, please? Just so that the commit makes more sense. Thanks |
|
Suggested way forward:
|
Description
Fix the warning being shown when no InputActionMap is setup properly. There was a missing file name in the warning.
I also now only show the warning if there is an action map as we default when there isn't one at all, therefore we don't need the warning.
Ticket
https://jira.unity3d.com/browse/ISXB-1560
Testing status & QA
I have tested manually I recommend QA does the same. Following the repro steps in the ticket.
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
Nothing major to comment.
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: