-
Notifications
You must be signed in to change notification settings - Fork 693
fix: Filter EditorApplication.isCompiling false positives in Play mode #582
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| using System; | ||
| using System.Reflection; | ||
| using MCPForUnity.Editor.Helpers; | ||
| using Newtonsoft.Json; | ||
| using Newtonsoft.Json.Linq; | ||
|
|
@@ -265,7 +266,7 @@ private static void OnUpdate() | |
| if (now - _lastUpdateTimeSinceStartup < MinUpdateIntervalSeconds) | ||
| { | ||
| // Still update on compilation edge transitions to keep timestamps meaningful. | ||
| bool isCompiling = EditorApplication.isCompiling; | ||
| bool isCompiling = GetActualIsCompiling(); | ||
| if (isCompiling == _lastIsCompiling) | ||
| { | ||
| return; | ||
|
|
@@ -289,7 +290,7 @@ private static JObject BuildSnapshot(string reason) | |
| _sequence++; | ||
| _observedUnixMs = DateTimeOffset.UtcNow.ToUnixTimeMilliseconds(); | ||
|
|
||
| bool isCompiling = EditorApplication.isCompiling; | ||
| bool isCompiling = GetActualIsCompiling(); | ||
| if (isCompiling && !_lastIsCompiling) | ||
| { | ||
| _lastCompileStartedUnixMs = _observedUnixMs; | ||
|
|
@@ -427,6 +428,42 @@ public static JObject GetSnapshot() | |
| return (JObject)_cached.DeepClone(); | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Returns the actual compilation state, working around a known Unity quirk where | ||
| /// EditorApplication.isCompiling can return false positives in Play mode. | ||
| /// See: https://github.com/CoplayDev/unity-mcp/issues/549 | ||
| /// </summary> | ||
| private static bool GetActualIsCompiling() | ||
| { | ||
| // If EditorApplication.isCompiling is false, Unity is definitely not compiling | ||
| if (!EditorApplication.isCompiling) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| // In Play mode, EditorApplication.isCompiling can have false positives. | ||
| // Double-check with CompilationPipeline.isCompiling via reflection. | ||
| if (EditorApplication.isPlaying) | ||
| { | ||
| try | ||
| { | ||
| Type pipeline = Type.GetType("UnityEditor.Compilation.CompilationPipeline, UnityEditor"); | ||
| var prop = pipeline?.GetProperty("isCompiling", BindingFlags.Public | BindingFlags.Static); | ||
| if (prop != null) | ||
| { | ||
| return (bool)prop.GetValue(null); | ||
| } | ||
| } | ||
| catch | ||
| { | ||
| // If reflection fails, fall back to EditorApplication.isCompiling | ||
| } | ||
| } | ||
|
|
||
| // Outside Play mode or if reflection failed, trust EditorApplication.isCompiling | ||
| return true; | ||
|
Comment on lines
+464
to
+465
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick: Final return value relies on earlier guard, making the comment slightly misleading. The comment says we "trust EditorApplication.isCompiling", but this branch always returns |
||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
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 (bug_risk): Catching all exceptions here may hide useful diagnostics when the reflection path breaks.
This bare
catchwill silently hide reflection issues (e.g., type renames, removed APIs), making it hard to detect when this path stops working due to Unity changes. Please at least emit a one-time warning when reflection fails (e.g., guarded by a static flag) so such breakages are visible without flooding the console.Suggested implementation:
Add a static flag field to the containing class (likely
EditorStateCache):Place it with the other fields in
EditorStateCache.Ensure the file has the necessary
usingdirectives at the top if they are not already present:If your codebase uses a different logging abstraction (e.g., a custom logger instead of
UnityEngine.Debug), replaceUnityEngine.Debug.LogWarning(...)with the project-standard logging method.