-
Notifications
You must be signed in to change notification settings - Fork 330
CHANGE: Remove the UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS compile-time define #2277
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
Packages/com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/InputActionsEditorWindow.cs
Show resolved
Hide resolved
c33d149 to
cf2729d
Compare
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr-agent |
| // Redirect to IMGUI editor | ||
| InputActionEditorWindow.OpenEditor(asset); | ||
| #endif | ||
| InputActionsEditorWindow.OpenEditor(asset); |
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.
Suggestion: The call to InputActionsEditorWindow.OpenEditor(asset) unconditionally opens the UI Toolkit editor, ignoring the InputSystem.settings.useIMGUIEditorForAssets setting. This setting is still used in other parts of the codebase to allow using the IMGUI editor. The logic to choose the editor based on this setting should be restored to avoid breaking user configurations that rely on the IMGUI editor. [possible issue, importance: 8]
| InputActionsEditorWindow.OpenEditor(asset); | |
| if (!InputSystem.settings.useIMGUIEditorForAssets) | |
| InputActionsEditorWindow.OpenEditor(asset); | |
| else | |
| InputActionEditorWindow.OpenEditor(asset); |
🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr-agent
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 agree that the useIMGUIEditorForAssets setting should not be used anymore and be marked deprecated?
Packages/com.unity.inputsystem/InputSystem/Editor/AssetImporter/InputActionImporterEditor.cs
Show resolved
Hide resolved
josepmariapujol-unity
left a comment
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! Approved
ekcoh
left a comment
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.
Great to see this clean-up happening. Could approve already now but want to sort out why we cannot at least deprecate and stop using the imgui setting?
Packages/com.unity.inputsystem/InputSystem/Actions/Composites/AxisComposite.cs
Show resolved
Hide resolved
| // Redirect to IMGUI editor | ||
| InputActionEditorWindow.OpenEditor(asset); | ||
| #endif | ||
| InputActionsEditorWindow.OpenEditor(asset); |
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 agree that the useIMGUIEditorForAssets setting should not be used anymore and be marked deprecated?
Packages/com.unity.inputsystem/InputSystem/Editor/AssetImporter/InputActionImporterEditor.cs
Show resolved
Hide resolved
Packages/com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/InputActionsEditorWindow.cs
Show resolved
Hide resolved
jfreire-unity
left a comment
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 doing this. Great to do some house cleaning! I share the sentiment of removing old IMGUI inspector code. Maybe we could also figure out how many inspectors there are and plan work to remove them in 6.7?
My only nitpick is to the PR title, because I got confused at first.
We're actually only removing the conditional compilation for the project-wide actions feature AND removing unused IMGUI code.
But changing it to to "Remove project-wide input action feature conditional compilation" or "Project-wide input actions feature is always compiled" would be slightly better, but it's just an opinion.
|
CC @Pauliusd01, here is a PR where we remove some more defines. Do you want to be added as a reviewer, or is it fine to just have you mentioned for viz? |
|
Going back to removing InputSystem.settings.useIMGUIEditorForAssets, all the spots I checked actually have a UITK path that is currently enabled by default.
Check out this image, it's typical. There are 14 files like that. I'd propose this:
How does that sound @ekcoh @jfreire-unity ? |
Pauliusd01
left a comment
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, tested:
- Input binding controls in the inspector
- Input Actions window and ProjectWideActions window functionality, saving, controls, focus, etc
- Upgrading from 1.7 Input system to this PR
Sounds like a good plan to me 👍 |
|
Failures overview: InputSystem-MobileFunctionalTests - 6000.5 - IOS This is a known problem that is unrelated to this branch, Wind is working on it at the moment. Test Sample Projects trunk It's caused by a surprising instability that is a compilation issue caused by some Timeline script upgrader shenanigans. Do we even depend on timeline though? Library/PackageCache/com.unity.timeline@6b9e48457ddb/Editor/TimelineHelpers.cs(31,20): error CS0234: The type or namespace name 'GUID' does not exist in the namespace 'UnityEditor' (are you missing an assembly reference?) Nothing of that is related to my changes so I'm going to force-merge now. |

Description
Hereby we propose to remove support for the case where we run without the project-wide input actions feature via the corresponding compile-time define (UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS).
It was necessary back in the day for two main reasons:
In this PR, alongside a myriad of trivial changes where we remove the define set/check, we also removed the old imgui-based Input Actions Editor Window that we don't show any more via normal means. It's been possible to enable it using features, but we no longer want to have that variability. With it, we're cutting
InputActionEditorToolbar. We would have loved to cut more imgui-related stuff, in particularInputActionPropertiesViewbut that proved to be impossible -- since it's still used in custom property drawers.Testing status & QA
Local testing / compilation.
Overall Product Risks
Comments to reviewers
Needs to land after #2274
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: