NEW: Icon to InputSystem settings asset#2400
NEW: Icon to InputSystem settings asset#2400josepmariapujol-unity wants to merge 8 commits intodevelopfrom
Conversation
josepmariapujol-unity
changed the title
josepmariapujol-unity
changed the title
![u-pr[bot]](https://avatars.githubusercontent.com/in/1201017?s=60&v=4){kind=small}
There was a problem hiding this comment.
The pull request introduces improvements to the Input System's editor settings, specifically regarding icon management. However, there are a few architectural and performance issues identified, such as setting icons in OnEnable (which can cause VCS noise) and potential icon persistence issues when creating new assets.
🤖 Helpful? 👍/👎
| private void OnEnable() | ||
| { | ||
| var icon = InputActionAssetIconLoader.LoadSettingsIcon(); | ||
| if (icon != null) | ||
| EditorGUIUtility.SetIconForObject(target, icon); | ||
| } |
There was a problem hiding this comment.
Calling EditorGUIUtility.SetIconForObject inside OnEnable of an Editor is an anti-pattern that can lead to several issues:
- Metadata Pollution: For assets, this method modifies the associated
.metafile. Calling it inOnEnableensures the metadata is potentially modified every time the asset is selected or the inspector is refreshed, causing unnecessary noise in Version Control Systems (e.g., Git, Perforce). - Dirtying Assets: It may mark the asset as dirty, triggering "Save Changes" prompts or automatic saves when the user merely viewed the asset.
- Import Loops: In some Unity versions, modifying metadata can trigger an asset re-import, which might re-trigger
OnEnable, potentially leading to a refresh loop.
It is recommended to set the icon once during asset creation or via an AssetPostprocessor rather than forcing it every time the inspector is enabled.
🤖 Helpful? 👍/👎
| var settings = ScriptableObject.CreateInstance<InputSettings>(); | ||
| var icon = InputActionAssetIconLoader.LoadSettingsIcon(); | ||
| if (icon != null) | ||
| EditorGUIUtility.SetIconForObject(settings, icon); | ||
| InputSystem.settings = InputAssetEditorUtils.CreateAsset(settings, relativePath); |
There was a problem hiding this comment.
Have you considered calling EditorGUIUtility.SetIconForObject after the asset is created via InputAssetEditorUtils.CreateAsset?
When SetIconForObject is called on a memory-only instance (created via ScriptableObject.CreateInstance), the icon association is often lost or not correctly persisted to the asset's metadata (.meta file) when it is subsequently saved to disk. To ensure the icon persists, it should be applied to the object once it has been registered as an asset in the AssetDatabase.
| var settings = ScriptableObject.CreateInstance<InputSettings>(); | |
| var icon = InputActionAssetIconLoader.LoadSettingsIcon(); | |
| if (icon != null) | |
| EditorGUIUtility.SetIconForObject(settings, icon); | |
| InputSystem.settings = InputAssetEditorUtils.CreateAsset(settings, relativePath); | |
| var settings = ScriptableObject.CreateInstance<InputSettings>(); | |
| var createdAsset = InputAssetEditorUtils.CreateAsset(settings, relativePath); | |
| if (createdAsset != null) | |
| { | |
| var icon = InputActionAssetIconLoader.LoadSettingsIcon(); | |
| if (icon != null) | |
| EditorGUIUtility.SetIconForObject(createdAsset, icon); | |
| InputSystem.settings = createdAsset; | |
| } |
🤖 Helpful? 👍/👎
| /// <returns>Icon resource reference or <code>null</code> if the resource could not be loaded.</returns> | ||
| internal static Texture2D LoadSettingsIcon() | ||
| { | ||
| return (Texture2D)EditorGUIUtility.Load(kSettingsIcon); |
There was a problem hiding this comment.
Have you considered caching the loaded Texture2D in a static field? While EditorGUIUtility.Load performs some internal caching, maintaining a static reference in this loader class avoids the overhead of repeated lookups, which is beneficial since this method is called within OnEnable of the InputSettingsEditor.
🤖 Helpful? 👍/👎
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## develop #2400 +/- ##
===========================================
+ Coverage 77.92% 78.08% +0.16%
===========================================
Files 482 483 +1
Lines 97755 98772 +1017
===========================================
+ Hits 76175 77131 +956
- Misses 21580 21641 +61 Flags with carried forward coverage won't be shown. Click here to find out more.
|
1 participant
Description
Adding an icon to InputSystem settings asset
InputSystem.inputsettings.Icon library:
Dark:
Light:
Testing status & QA
N/A
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
Please describe any additional information such as what to focus on, or historical info for the reviewers.
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.