[UUM-131032] Coreclr - fast enter playmode#666
Conversation
lopezt-unity
requested review from
antoine-unity,
modrimkus-unity and
xianenli30
![u-pr[bot]](https://avatars.githubusercontent.com/in/1201017?s=60&v=4){kind=small}
There was a problem hiding this comment.
This review identifies several issues related to static state management and potential memory leaks. Key concerns include a race condition in ActionSettings, incorrect initialization timing in ShowHoveredInfo, and potential material leaks. Several minor optimizations and consistency improvements are also recommended.
🤖 Helpful? 👍/👎
| bool m_IsHovering; | ||
|
|
||
| #if UNITY_EDITOR | ||
| [RuntimeInitializeOnLoadMethod] |
There was a problem hiding this comment.
The default load type for RuntimeInitializeOnLoadMethod is AfterSceneLoad, which executes after the scene has loaded and Awake has been called. If an instance of ShowHoveredInfo exists in the scene, its Start method may run before this reset, causing it to initialize m_Material only for it to be immediately nulled here. This can lead to NullReferenceExceptions if the material is used later and also results in a resource leak.
Using BeforeSceneLoad ensures the static reference is cleared before any instance logic runs.
| [RuntimeInitializeOnLoadMethod] | |
| [RuntimeInitializeOnLoadMethod(RuntimeInitializeLoadType.BeforeSceneLoad)] |
🤖 Helpful? 👍/👎
| [RuntimeInitializeOnLoadMethod] | ||
| static void ResetStaticsOnLoad() | ||
| { | ||
| m_Material = null; |
There was a problem hiding this comment.
When 'Enter Play Mode' is used with Domain Reload disabled, static material instances created via new Material() will leak if the reference is nulled without the material being destroyed. It is recommended to explicitly destroy the material before clearing the reference to prevent memory accumulation in the Editor.
| m_Material = null; | |
| if (m_Material != null) UnityEngine.Object.DestroyImmediate(m_Material); | |
| m_Material = null; |
🤖 Helpful? 👍/👎
| [InitializeOnEnterPlayMode] | ||
| internal static void ResetPreviewActionManagerStatics() | ||
| { | ||
| Cancel(); |
There was a problem hiding this comment.
There is a race condition here because the execution order of [InitializeOnEnterPlayMode] methods is undefined across different classes. If UndoUtility.ResetStaticsOnLoad (in UndoUtility.cs) executes before this method, the preview group index will be reset to -1 before Cancel() is called. This prevents UndoUtility.CancelPreview() from reverting the preview geometry, potentially leaving 'ghost' objects in the scene when Play Mode starts.
Consider using EditorApplication.playModeStateChanged with PlayModeStateChange.ExitingEditMode to ensure the cancellation happens before any static states are reset.
🤖 Helpful? 👍/👎
| { | ||
| // Window may be destroyed when entering Play Mode without domain reload. | ||
| try { s_NotificationWindow.RemoveNotification(); } | ||
| catch {} |
There was a problem hiding this comment.
Empty catch blocks can swallow unexpected exceptions and make troubleshooting difficult. If the exception is expected because the window might be destroyed, consider checking for null first or catching a specific exception type (e.g., MissingReferenceException).
🤖 Helpful? 👍/👎
| [RuntimeInitializeOnLoadMethod(RuntimeInitializeLoadType.BeforeSceneLoad)] | ||
| static void ResetStaticsOnLoad() | ||
| { | ||
| PI_2 = 2.0 * Math.PI; |
There was a problem hiding this comment.
This reset appears to be redundant because PI_2 is a private static variable assigned a constant value and does not seem to be modified anywhere else in the class.
🤖 Helpful? 👍/👎
| // Reset the static variables to their initial values, these 3 static variables will be re-initialized when accessed | ||
| s_DefaultMaterial = null; | ||
| s_UnityDefaultDiffuse = null; | ||
| s_ShapePreviewMaterial = null; |
There was a problem hiding this comment.
If these materials were instantiated using new Material() (which can occur in Init() if the resource load fails), they will leak in memory when the reference is nulled across Play Mode sessions. It is safer to check for null and call DestroyImmediate before resetting the reference.
🤖 Helpful? 👍/👎
| s_SmoothAvg = new Vector3[Smoothing.smoothRangeMax]; | ||
| s_SmoothAvgCount = new float[Smoothing.smoothRangeMax]; | ||
| s_CachedIntArray = new int[ushort.MaxValue]; |
There was a problem hiding this comment.
Re-allocating these large arrays every time Play Mode is entered increases GC pressure. Since these are static caches, consider using Array.Clear to reuse the existing arrays if they are already allocated.
🤖 Helpful? 👍/👎
|
|
||
| #if UNITY_EDITOR | ||
| [RuntimeInitializeOnLoadMethod(RuntimeInitializeLoadType.BeforeSceneLoad)] | ||
| static void ResetStaticsOnLoad_UvAutoManualConversion() |
There was a problem hiding this comment.
The method name ResetStaticsOnLoad_UvAutoManualConversion is inconsistent with the ResetStaticsOnLoad naming pattern used throughout this PR.
🤖 Helpful? 👍/👎
Codecov ReportAttention: Patch coverage is @@ Coverage Diff @@
## master #666 +/- ##
==========================================
+ Coverage 36.05% 37.79% +1.73%
==========================================
Files 277 277
Lines 34909 38874 +3965
==========================================
+ Hits 12588 14691 +2103
- Misses 22321 24183 +1862
Flags with carried forward coverage won't be shown. Click here to find out more.
|
1 participant
Purpose of this PR
Resetting static states on enter playmode :
Using [InitializeOnEnterPlayMode] attribute for editor only scripts and [RuntimeInitializeOnLoadMethod] for runtime ones.
Also fixing the state of tools when entering / exiting playmode as there were desync when in used while entering playmode.
Links
Jira: https://jira.unity3d.com/browse/UUM-131032
Comments to Reviewers
[List known issues, planned work, provide any extra context for your code.]