Input system/cache properties#2370
Conversation
![u-pr[bot]](https://avatars.githubusercontent.com/in/1201017?s=60&v=4){kind=small}
There was a problem hiding this comment.
This pull request makes improvements to the PlayerInputManagerEditor. While the changes are generally sound, there are a few issues regarding unnecessary serialized property writes that could impact performance or the undo system, as well as some minor inconsistencies in API usage and lifecycle management.
🤖 Helpful? 👍/👎
| EditorGUILayout.PropertyField(m_MaxPlayerCountProperty); | ||
| --EditorGUI.indentLevel; | ||
| } | ||
| else |
There was a problem hiding this comment.
Assigning a value to m_MaxPlayerCountProperty.intValue inside the else block of OnInspectorGUI will result in an unconditional write to the serialized property on every GUI event (Layout, Repaint, etc.) whenever the toggle is disabled. This can cause the SerializedObject to be marked dirty unnecessarily and may interfere with the Undo system or performance in large scenes. Consider wrapping this assignment in a check to see if the value is already -1, or perform the assignment only when m_MaxPlayerCountEnabled changes.
🤖 Helpful? 👍/👎
| m_FixedNumberOfSplitScreensLabel = EditorGUIUtility.TrTextContent("Number of Screens", | ||
| fixedNumberProperty.tooltip); | ||
| EditorGUILayout.PropertyField(fixedNumberProperty, m_FixedNumberOfSplitScreensLabel); | ||
| m_FixedNumberOfSplitScreensProperty.tooltip); |
There was a problem hiding this comment.
There is an inconsistency in how tooltips are accessed. On line 199, GetTooltip() is used, while here .tooltip is accessed directly. It is generally recommended to stick to one method (likely GetTooltip() if it handles fallbacks or version compatibility) to ensure consistent behavior across the editor.
🤖 Helpful? 👍/👎
| else | ||
| { | ||
| fixedNumberProperty.intValue = -1; | ||
| m_FixedNumberOfSplitScreensProperty.intValue = -1; |
There was a problem hiding this comment.
Similar to the m_MaxPlayerCountProperty assignment, this unconditionally sets the property value to -1 on every repaint when m_FixedNumberOfSplitScreensEnabled is false. This should ideally be guarded by a change check or a value comparison to avoid unnecessary dirtiness of the serialized object.
🤖 Helpful? 👍/👎
| [NonSerialized] private SerializedProperty m_NotificationBehaviorProperty; | ||
| [NonSerialized] private SerializedProperty m_PlayerJoinedEventProperty; | ||
| [NonSerialized] private SerializedProperty m_PlayerLeftEventProperty; | ||
| [NonSerialized] private SerializedProperty m_JoinBehaviorProperty; | ||
| [NonSerialized] private SerializedProperty m_JoinActionProperty; | ||
| [NonSerialized] private SerializedProperty m_PlayerPrefabProperty; | ||
| [NonSerialized] private SerializedProperty m_AllowJoiningProperty; | ||
| [NonSerialized] private SerializedProperty m_MaxPlayerCountProperty; | ||
| [NonSerialized] private SerializedProperty m_SplitScreenProperty; | ||
| [NonSerialized] private SerializedProperty m_MaintainAspectRatioProperty; | ||
| [NonSerialized] private SerializedProperty m_FixedNumberOfSplitScreensProperty; | ||
| [NonSerialized] private SerializedProperty m_SplitScreenRectProperty; |
There was a problem hiding this comment.
The [NonSerialized] fields for SerializedProperty are correctly added to avoid serialization overhead. However, since OnEnable caches these, ensure that OnDisable or the class doesn't leak references if the SerializedObject is disposed. While typically handled by Unity's garbage collection for Editor objects, it's a good practice to null them out in OnDisable if the editor lifecycle is complex.
🤖 Helpful? 👍/👎
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## develop #2370 +/- ##
===========================================
- Coverage 77.90% 77.90% -0.01%
===========================================
Files 476 476
Lines 97613 97649 +36
===========================================
+ Hits 76048 76072 +24
- Misses 21565 21577 +12 Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
1 participant
Description
Please fill this section with a description what the pull request is trying to address and what changes were made.
Testing status & QA
Please describe the testing already done by you and what testing you request/recommend QA to execute. If you used or created any testing project please link them here too for QA.
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.