[FEATURE]: Manage VFX function #520
Conversation
* Update the .Bat file to include runtime folder * Fix the inconsistent EditorPrefs variable so the GUI change on Script Validation could cause real change.
String to Int for consistency
Allows users to generate/compile codes during Playmode
Upload the unity_claude_skill that can be uploaded to Claude for a combo of unity-mcp-skill.
1. Fix Original Roslyn Compilation Custom Tool to fit the V8 standard 2. Add a new panel in the GUI to see and toggle/untoggle the tools. The toggle feature will be implemented in the future, right now its implemented here to discuss with the team if this is a good feature to add; 3. Add few missing summary in certain tools
This reverts commit ae8cfe5.
This reverts commit f423c2f.
Merge and use the previous PR's solution to solve the windows issue.
Fix the layout problem of manage_script in the panel
Tested object generation/modification with batch and it works perfectly! We should push and let users test for a while and see PS: I tried both VS Copilot and Claude Desktop. Claude Desktop works but VS Copilot does not due to the nested structure of batch. Will look into it more.
Update on Batch
* Enable Camera Capture through both play and editor mode Notes: Because the standard ScreenCapture.CaptureScreenshot does not work in editor mode, so we use ScreenCapture.CaptureScreenshotIntoRenderTexture to enable it during play mode. * user can access the camera access through the tool menu, or through direct LLM calling. Both tested on Windows with Claude Desktop.
nitpicking changes
Similar to deploy.bat, but sideload it to MCP For Unity for easier deployment inside Unity menu.
Add ManageVFX to the current function, support: 1. Modify LineRender, TrailRender properties 2. Modify particle system (legacy) properties 3. Modify VisualEffectGraph and use templates (does not support edit graph yet)
Reviewer's GuideIntroduce a unified manage_vfx tool pair (Unity editor + server) to control ParticleSystem, VisualEffect Graph, LineRenderer, and TrailRenderer components via structured actions, plus a generic float coercion helper on the server for safer numeric parsing. Sequence diagram for a manage_vfx particle_set_main callsequenceDiagram
actor caller
participant manage_vfx_tool
participant send_with_unity_instance
participant async_send_command_with_retry
participant unity_editor as Unity_Editor
participant manage_vfx_unity as ManageVFX
participant particle_system as ParticleSystem
caller->>manage_vfx_tool: manage_vfx(action=particle_set_main, target, duration, start_speed,...)
manage_vfx_tool->>manage_vfx_tool: validate action in ALL_ACTIONS
manage_vfx_tool->>manage_vfx_tool: build params_dict
manage_vfx_tool->>send_with_unity_instance: tool manage_vfx, params_dict
send_with_unity_instance->>async_send_command_with_retry: manage_vfx, params_dict
async_send_command_with_retry->>unity_editor: send command manage_vfx
unity_editor->>manage_vfx_unity: HandleCommand(JObject params)
manage_vfx_unity->>manage_vfx_unity: route to HandleParticleSystemAction(set_main)
manage_vfx_unity->>particle_system: ParticleSetMain
particle_system-->>manage_vfx_unity: updated main module
manage_vfx_unity-->>async_send_command_with_retry: {success=true,...}
async_send_command_with_retry-->>send_with_unity_instance: result
send_with_unity_instance-->>manage_vfx_tool: result dict
manage_vfx_tool-->>caller: result dict
Class diagram for Unity ManageVFX editor tool and related handlersclassDiagram
class ManageVFX {
<<static>>
+HandleCommand(params JObject) object
-ParseColor(token JToken) Color
-ParseVector3(token JToken) Vector3
-ParseVector4(token JToken) Vector4
-ParseGradient(token JToken) Gradient
-ParseAnimationCurve(token JToken, defaultValue float) AnimationCurve
-FindTargetGameObject(params JObject) GameObject
}
class ParticleSystemHandlers {
-HandleParticleSystemAction(params JObject, action string) object
-FindParticleSystem(params JObject) ParticleSystem
-ParseMinMaxCurve(token JToken, defaultValue float) MinMaxCurve
-ParseMinMaxGradient(token JToken) MinMaxGradient
-ParticleGetInfo(params JObject) object
-ParticleSetMain(params JObject) object
-ParticleSetEmission(params JObject) object
-ParticleSetShape(params JObject) object
-ParticleSetColorOverLifetime(params JObject) object
-ParticleSetSizeOverLifetime(params JObject) object
-ParticleSetVelocityOverLifetime(params JObject) object
-ParticleSetNoise(params JObject) object
-ParticleSetRenderer(params JObject) object
-ParticleEnableModule(params JObject) object
-ParticleControl(params JObject, action string) object
-ParticleAddBurst(params JObject) object
-ParticleClearBursts(params JObject) object
}
class VFXGraphHandlers {
-HandleVFXGraphAction(params JObject, action string) object
-FindVisualEffect(params JObject) VisualEffect
-VFXCreateAsset(params JObject) object
-VFXAssignAsset(params JObject) object
-VFXListTemplates(params JObject) object
-VFXListAssets(params JObject) object
-VFXGetInfo(params JObject) object
-VFXSetParameter(params JObject, setter Action) object
-VFXSetVector(params JObject, dims int) object
-VFXSetColor(params JObject) object
-VFXSetGradient(params JObject) object
-VFXSetTexture(params JObject) object
-VFXSetMesh(params JObject) object
-VFXSetCurve(params JObject) object
-VFXSendEvent(params JObject) object
-VFXControl(params JObject, action string) object
-VFXSetPlaybackSpeed(params JObject) object
-VFXSetSeed(params JObject) object
}
class LineRendererHandlers {
-HandleLineRendererAction(params JObject, action string) object
-FindLineRenderer(params JObject) LineRenderer
-LineGetInfo(params JObject) object
-LineSetPositions(params JObject) object
-LineAddPosition(params JObject) object
-LineSetPosition(params JObject) object
-LineSetWidth(params JObject) object
-LineSetColor(params JObject) object
-LineSetMaterial(params JObject) object
-LineSetProperties(params JObject) object
-LineClear(params JObject) object
-LineCreateLine(params JObject) object
-LineCreateCircle(params JObject) object
-LineCreateArc(params JObject) object
-LineCreateBezier(params JObject) object
}
class TrailRendererHandlers {
-HandleTrailRendererAction(params JObject, action string) object
-FindTrailRenderer(params JObject) TrailRenderer
-TrailGetInfo(params JObject) object
-TrailSetTime(params JObject) object
-TrailSetWidth(params JObject) object
-TrailSetColor(params JObject) object
-TrailSetMaterial(params JObject) object
-TrailSetProperties(params JObject) object
-TrailClear(params JObject) object
-TrailEmit(params JObject) object
}
class manage_vfx_tool_py {
<<tool function>>
+manage_vfx(ctx Context, action str, ...) dict
}
class TransportLayer {
+send_with_unity_instance(fn, unity_instance, tool_name str, params_dict dict) Any
+async_send_command_with_retry(unity_instance, tool_name str, params_dict dict) Any
}
ManageVFX ..> ParticleSystemHandlers
ManageVFX ..> VFXGraphHandlers
ManageVFX ..> LineRendererHandlers
ManageVFX ..> TrailRendererHandlers
manage_vfx_tool_py --> TransportLayer
TransportLayer --> ManageVFX
ParticleSystemHandlers --> ParticleSystem
VFXGraphHandlers --> VisualEffect
LineRendererHandlers --> LineRenderer
TrailRendererHandlers --> TrailRenderer
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded@Scriptwonder has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 8 minutes and 43 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a new Unity Editor tool (MCPForUnity.Editor.Tools.ManageVFX) with a central HandleCommand(JObject) API and matching server-side async Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server as manage_vfx.py
participant Unity as ManageVFX.cs
participant Component as VFX/Particle/Line/Trail
Client->>Server: manage_vfx(action, target, params...)
activate Server
rect rgb(235,244,255)
note over Server: Validate action & build params\n(filter out None)
Server->>Server: Validate action vs ALL_ACTIONS
Server->>Server: Populate params
end
Server->>Unity: send_command_with_retry → HandleCommand(params)
activate Unity
rect rgb(255,250,235)
note over Unity: Route by action prefix\n(particle_*, vfx_*, line_*, trail_*)
alt particle_*
Unity->>Component: ParticleSystem ops (get/set, modules, bursts, play/stop)
else vfx_*
Unity->>Component: VisualEffect/VFX Graph ops (assets, params, events)
else line_*
Unity->>Component: LineRenderer ops (positions, width, color, create)
else trail_*
Unity->>Component: TrailRenderer ops (properties, emit, clear)
end
end
Component-->>Unity: result/state
Unity-->>Server: result
deactivate Unity
Server->>Server: Normalize response (dict)
Server-->>Client: result
deactivate Server
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom Pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The
manage_vfxtool function has an extremely large parameter list and a very long body; consider splitting it into smaller, component-specific helpers (e.g., particle, vfx, line, trail) to improve readability and maintainability and to reduce the chance of drift between the Python and C# parameter mappings. - In
ManageVFX.VFXControl, thepauseaction togglesvfx.pauseinstead of setting it explicitly, which can be surprising if the caller expects idempotent behavior; consider using a separateresumeaction or an explicitisPausedparameter rather than toggling. - There is quite a bit of duplicated logic for applying common renderer properties across ParticleSystemRenderer, LineRenderer, and TrailRenderer; consider extracting shared helper methods (e.g., for shadows, probes, sorting) to reduce duplication and keep behavior consistent across renderer types.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `manage_vfx` tool function has an extremely large parameter list and a very long body; consider splitting it into smaller, component-specific helpers (e.g., particle, vfx, line, trail) to improve readability and maintainability and to reduce the chance of drift between the Python and C# parameter mappings.
- In `ManageVFX.VFXControl`, the `pause` action toggles `vfx.pause` instead of setting it explicitly, which can be surprising if the caller expects idempotent behavior; consider using a separate `resume` action or an explicit `isPaused` parameter rather than toggling.
- There is quite a bit of duplicated logic for applying common renderer properties across ParticleSystemRenderer, LineRenderer, and TrailRenderer; consider extracting shared helper methods (e.g., for shadows, probes, sorting) to reduce duplication and keep behavior consistent across renderer types.
## Individual Comments
### Comment 1
<location> `Server/src/services/tools/utils.py:80-94` </location>
<code_context>
return default
+def coerce_float(value: Any, default: float | None = None) -> float | None:
+ """Attempt to coerce a loosely-typed value to a float."""
+ if value is None:
+ return default
+ try:
+ if isinstance(value, bool):
+ return default
+ if isinstance(value, (int, float)):
+ return float(value)
+ s = str(value).strip()
+ if s.lower() in ("", "none", "null"):
+ return default
+ return float(s)
+ except Exception:
+ return default
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Narrow the exception type and reconsider special-casing bools in `coerce_float`.
Catching `Exception` here will also hide unexpected programmer errors (e.g., failures in `__str__`) and silently return `default`, which makes debugging harder. Prefer catching only the conversion-related exceptions (`ValueError`, `TypeError`) that `float()`/`str()` actually raise.
The special-casing of `bool` also changes Python’s default behavior (`float(True) == 1.0`, `float(False) == 0.0`). If the goal is to treat booleans as invalid numeric input, consider a short comment or a more explicit function name to document that choice; otherwise you could remove this branch to match standard numeric coercion semantics.
```suggestion
def coerce_float(value: Any, default: float | None = None) -> float | None:
"""Attempt to coerce a loosely-typed value to a float-like number."""
if value is None:
return default
try:
# Treat booleans as invalid numeric input instead of coercing to 0/1.
if isinstance(value, bool):
return default
if isinstance(value, (int, float)):
return float(value)
s = str(value).strip()
if s.lower() in ("", "none", "null"):
return default
return float(s)
except (TypeError, ValueError):
return default
```
</issue_to_address>
### Comment 2
<location> `MCPForUnity/Editor/Tools/ManageVFX.cs:1061-1070` </location>
<code_context>
+ private static object VFXListTemplates(JObject @params)
</code_context>
<issue_to_address>
**issue (bug_risk):** Template path comparisons mix absolute and asset-relative paths, so duplicate filtering may not work.
In `VFXListTemplates`, `relativePath = file.Replace("\\", "/")` is still an absolute filesystem path, while `AssetDatabase.GUIDToAssetPath(guid)` returns project-relative paths (`Assets/...`). The duplicate check:
```csharp
if (!templates.Any(t => ((dynamic)t).path == path))
```
compares these mismatched formats, so the same asset can be added twice.
Normalize paths before storing/comparing (e.g., convert filesystem paths to project-relative by stripping the project root, or convert all asset paths to absolute paths) so deduplication works reliably and `path` has a consistent format for consumers.
</issue_to_address>
### Comment 3
<location> `Server/src/services/tools/manage_vfx.py:216-217` </location>
<code_context>
+ reset_seed_on_play: Annotated[Any, "[VFX] Reset seed on play (bool or string)"] = None,
+
+ # === LINE/TRAIL RENDERER PARAMETERS ===
+ positions: Annotated[Any, "[Line] Positions [[x,y,z], ...] (array or JSON string)"] = None,
+ position: Annotated[Any, "[Line/Trail] Single position [x,y,z] (array or JSON string)"] = None,
+ index: Annotated[Any, "[Line] Position index (integer or string)"] = None,
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Python-side docs allow JSON strings for arrays/vectors, but the C# side only handles arrays/objects.
Several parameters are documented as accepting "array or JSON string" (e.g. `positions`, `position`, `color`, `gradient`), but the C# handlers expect `JArray`/`JObject` (e.g. `@params["positions"] as JArray`, checks in `ParseColor`/`ParseVector3`/`ParseGradient`). If a client passes a JSON string, it will be treated as a string value in the `JObject`, those casts/checks will fail, and you’ll hit errors like "Positions array required".
I’d recommend either parsing string values into JSON on the Python side before sending, or updating the Python annotations/docs to require structured JSON (arrays/objects) only, so the documented shapes match what the C# side actually accepts.
</issue_to_address>
### Comment 4
<location> `Server/src/services/tools/manage_vfx.py:296-318` </location>
<code_context>
+ """Unified VFX management tool."""
+
+ # Validate action against known actions
+ if action not in ALL_ACTIONS:
+ # Provide helpful error with closest matches by prefix
+ prefix = action.split("_")[0] + "_" if "_" in action else ""
+ available_by_prefix = {
+ "particle_": PARTICLE_ACTIONS,
+ "vfx_": VFX_ACTIONS,
+ "line_": LINE_ACTIONS,
+ "trail_": TRAIL_ACTIONS,
+ }
+ suggestions = available_by_prefix.get(prefix, [])
+ if suggestions:
+ return {"success": False, "message": f"Unknown action '{action}'. Available {prefix}* actions: {', '.join(suggestions)}"}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Action validation is case-sensitive in Python while the Unity side normalizes actions to lowercase.
On the C# side (`HandleCommand`), the action is lowercased (`actionLower = action.ToLowerInvariant()`) before matching, but here you validate the raw `action` against `ALL_ACTIONS`. That means inputs like `"Particle_Play"` or `"VFX_Set_Float"` would fail validation in Python even though Unity would accept them.
To align behavior, normalize to lowercase before validation and use that value going forward:
```python
action_normalized = action.lower()
if action_normalized not in ALL_ACTIONS:
...
params_dict = {"action": action_normalized}
```
Since `ALL_ACTIONS` is already lowercase, this should be a minimal change that keeps both sides consistent.
```suggestion
) -> dict[str, Any]:
"""Unified VFX management tool."""
# Normalize action to lowercase to match Unity-side behavior
action_normalized = action.lower()
# Validate action against known actions using normalized value
if action_normalized not in ALL_ACTIONS:
# Provide helpful error with closest matches by prefix
prefix = action_normalized.split("_")[0] + "_" if "_" in action_normalized else ""
available_by_prefix = {
"particle_": PARTICLE_ACTIONS,
"vfx_": VFX_ACTIONS,
"line_": LINE_ACTIONS,
"trail_": TRAIL_ACTIONS,
}
suggestions = available_by_prefix.get(prefix, [])
if suggestions:
return {
"success": False,
"message": f"Unknown action '{action}'. Available {prefix}* actions: {', '.join(suggestions)}",
}
else:
return {
"success": False,
"message": (
f"Unknown action '{action}'. Use prefixes: "
"particle_*, vfx_*, line_*, trail_*. Run with action='ping' to test connection."
),
}
unity_instance = get_unity_instance_from_context(ctx)
# Build parameters dict with normalized action to stay consistent with Unity
params_dict: dict[str, Any] = {"action": action_normalized}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| positions: Annotated[Any, "[Line] Positions [[x,y,z], ...] (array or JSON string)"] = None, | ||
| position: Annotated[Any, "[Line/Trail] Single position [x,y,z] (array or JSON string)"] = None, |
There was a problem hiding this comment.
issue (bug_risk): Python-side docs allow JSON strings for arrays/vectors, but the C# side only handles arrays/objects.
Several parameters are documented as accepting "array or JSON string" (e.g. positions, position, color, gradient), but the C# handlers expect JArray/JObject (e.g. @params["positions"] as JArray, checks in ParseColor/ParseVector3/ParseGradient). If a client passes a JSON string, it will be treated as a string value in the JObject, those casts/checks will fail, and you’ll hit errors like "Positions array required".
I’d recommend either parsing string values into JSON on the Python side before sending, or updating the Python annotations/docs to require structured JSON (arrays/objects) only, so the documented shapes match what the C# side actually accepts.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @MCPForUnity/Editor/Tools/ManageVFX.cs:
- Around line 1909-1916: TrailClear currently calls FindTrailRenderer and
tr.Clear() without recording an undo; update TrailClear to call
UnityEditor.Undo.RecordObject (or Undo.RegisterCompleteObjectUndo) on the
TrailRenderer instance (use a label like "Clear Trail") prior to invoking
TrailRenderer.Clear() so the action is undoable in the Editor; keep the existing
success/failure return shape and consider updating the success message to
mention that runtime trail data may not be fully restorable via Undo because
TrailRenderer.Clear() affects non-serialized runtime state.
In @Server/src/services/tools/manage_vfx.py:
- Around line 449-450: The params_dict currently uses the same key "size" for
two different inputs (size and size_over_lifetime), causing size_over_lifetime
to silently overwrite size; update both assignments to use distinct keys (e.g.,
params_dict["size"] for the event-level size and params_dict["sizeOverLifetime"]
or params_dict["size_over_lifetime"] for the particle-system lifetime size)
wherever size_over_lifetime is set (the second occurrence around the later
block) and adjust any downstream consumers to read the new key (or preserve the
original "size" when appropriate to maintain backward compatibility), ensuring
both params_dict["size"] and the new params_dict["sizeOverLifetime"] are used
consistently.
🧹 Nitpick comments (4)
Server/src/services/tools/manage_vfx.py (1)
604-615: Nil-filtering is redundant but harmless.Line 605 filters out
Nonevalues, but all values are already guarded byif ... is not Nonechecks before being added toparams_dict. The filter is defensive and doesn't hurt, but could be removed for minor cleanup.MCPForUnity/Editor/Tools/ManageVFX.cs (3)
751-755: Array size mismatch in ParticleAddBurst.
GetBurstsis called with an array of sizeidx + 1, but onlyidxbursts exist at that point. Unity'sGetBurstswill only fill the firstidxelements, leavingbursts[idx]uninitialized until you assign it.While this works because you immediately assign
bursts[idx] = burst, the intent would be clearer if the array is sized toidxfirst:🔎 Suggested clearer approach
int idx = emission.burstCount; - var bursts = new ParticleSystem.Burst[idx + 1]; - emission.GetBursts(bursts); + var existingBursts = new ParticleSystem.Burst[idx]; + emission.GetBursts(existingBursts); + var bursts = new ParticleSystem.Burst[idx + 1]; + existingBursts.CopyTo(bursts, 0); bursts[idx] = burst; emission.SetBursts(bursts);Or simply append after retrieving:
var bursts = new List<ParticleSystem.Burst>(); var temp = new ParticleSystem.Burst[idx]; emission.GetBursts(temp); bursts.AddRange(temp); bursts.Add(burst); emission.SetBursts(bursts.ToArray());
985-995: Empty catch block silently swallows exceptions.The empty
catch { }at line 994 hides any errors that occur during template file searching. This could make debugging difficult if template discovery fails unexpectedly.🔎 Suggested fix: Log or handle the exception
try { string[] allVfxFiles = System.IO.Directory.GetFiles(basePath, "*.vfx", System.IO.SearchOption.AllDirectories); foreach (string file in allVfxFiles) { if (System.IO.Path.GetFileNameWithoutExtension(file).ToLower().Contains(templateName.ToLower())) return file; } } - catch { } + catch (Exception ex) + { + // Log but continue searching other paths + Debug.LogWarning($"Error searching VFX templates in {basePath}: {ex.Message}"); + }
1085-1097: Empty catch block and potential runtime exception with dynamic.Two concerns:
- Line 1096: Empty
catch { }silently swallows exceptions during template enumeration.- Line 1104: Using
dynamicfor the path comparison((dynamic)t).pathcan throwRuntimeBinderExceptionif the anonymous type doesn't have apathproperty.🔎 Suggested fixes
For the empty catch:
catch { } + catch (Exception ex) + { + Debug.LogWarning($"Error enumerating VFX files in {basePath}: {ex.Message}"); + }For the dynamic usage, consider storing templates as a typed structure:
// Define a simple record or class record VFXTemplateInfo(string Name, string Path, string Source); // Use typed list var templates = new List<VFXTemplateInfo>(); // ... templates.Add(new VFXTemplateInfo(name, relativePath, isPackage ? "package" : "project")); // Then comparison becomes safe if (!templates.Any(t => t.Path == path))
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
MCPForUnity/Editor/Tools/ManageVFX.csMCPForUnity/Editor/Tools/ManageVFX.cs.metaServer/src/services/tools/manage_vfx.pyServer/src/services/tools/utils.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-03T16:00:55.839Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 0
File: :0-0
Timestamp: 2025-09-03T16:00:55.839Z
Learning: ComponentResolver in UnityMcpBridge/Editor/Tools/ManageGameObject.cs is a nested static class within ManageGameObject, not a sibling type. The `using static MCPForUnity.Editor.Tools.ManageGameObject;` import is required to access ComponentResolver methods directly without the outer class qualifier.
Applied to files:
MCPForUnity/Editor/Tools/ManageVFX.cs
📚 Learning: 2025-12-29T15:23:11.613Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:11.613Z
Learning: In MCPForUnity, prefer relying on the established testing process to catch UI initialization issues instead of adding defensive null checks for UI elements in editor windows. This means during reviews, verify that tests cover UI initialization paths and that code avoids repetitive null-check boilerplate in Editor Windows. If a UI element can be null, ensure there is a well-tested fallback or that its initialization is guaranteed by design, rather than sprinkling null checks throughout editor code.
Applied to files:
MCPForUnity/Editor/Tools/ManageVFX.cs
🧬 Code graph analysis (2)
Server/src/services/tools/manage_vfx.py (2)
Server/src/transport/unity_transport.py (1)
send_with_unity_instance(79-113)Server/src/transport/legacy/unity_connection.py (1)
async_send_command_with_retry(838-870)
MCPForUnity/Editor/Tools/ManageVFX.cs (1)
MCPForUnity/Editor/Helpers/ObjectResolver.cs (2)
Material(133-140)Texture(145-152)
🪛 Ruff (0.14.10)
Server/src/services/tools/utils.py
93-93: Do not catch blind exception: Exception
(BLE001)
Server/src/services/tools/manage_vfx.py
42-42: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
🔇 Additional comments (9)
MCPForUnity/Editor/Tools/ManageVFX.cs.meta (1)
1-13: LGTM!Standard Unity .meta file with appropriate MonoImporter settings. The GUID appears manually crafted but is valid and will function correctly.
Server/src/services/tools/utils.py (1)
80-95: LGTM!The implementation correctly mirrors
coerce_intand handles all expected input types robustly. The broadexcept Exceptionis appropriate here since this is a tolerant coercion utility designed to fail gracefully to the default value on any parsing error.Server/src/services/tools/manage_vfx.py (1)
126-296: LGTM - Well-structured tool with comprehensive parameter coverage.The function signature is large but necessarily so for exposing a unified VFX API. The action validation with prefix-based suggestions provides good UX for users who mistype actions.
MCPForUnity/Editor/Tools/ManageVFX.cs (6)
78-281: LGTM - Robust parsing helpers.The parsing methods handle multiple input formats (arrays, objects, scalars) with sensible defaults. The consistent pattern across
ParseColor,ParseVector3,ParseVector4,ParseGradient, andParseAnimationCurvemakes the code predictable and maintainable.
286-310: LGTM - Clean action routing for ParticleSystem.The switch statement cleanly maps action names to handler methods. The error message at line 308 lists all valid actions, which is helpful for debugging.
1188-1227: LGTM - VFX parameter setters are well-structured.The generic
VFXSetParameter<T>pattern is clean and allows type-safe parameter setting. TheVFXSetVectormethod elegantly handles different vector dimensions with a single switch.
1384-1733: LGTM - Comprehensive LineRenderer implementation.The shape creation methods (
LineCreateCircle,LineCreateArc,LineCreateBezier) are mathematically correct. The cross-product calculation at lines 1640-1643 correctly handles the edge case where the normal is parallel to forward vector.
1736-1907: LGTM - TrailRenderer handlers are well-implemented.Good use of conditional compilation for
TrailEmit(Unity 2021.1+ API). The property setters follow the same consistent pattern as LineRenderer.
15-76: LGTM - Well-structured command dispatcher.The
HandleCommandmethod cleanly routes actions based on prefixes. The try-catch at lines 36-75 ensures exceptions are caught and returned with stack traces for debugging. Good use ofToLowerInvariant()for case-insensitive matching.
| if size is not None: | ||
| params_dict["size"] = size |
There was a problem hiding this comment.
Potential parameter collision: size and size_over_lifetime both map to the same key.
Both parameters map to params_dict["size"]:
- Line 450:
sizeparameter →params_dict["size"] = size - Line 596:
size_over_lifetimeparameter →params_dict["size"] = size_over_lifetime
If both are provided, size_over_lifetime will overwrite size since it's processed later. This may be intentional (different contexts), but could cause confusion.
🔎 Suggested fix: Use distinct keys
Consider using sizeOverLifetime as a separate key if these serve different purposes:
if size_over_lifetime is not None:
- params_dict["size"] = size_over_lifetime
+ params_dict["sizeOverLifetime"] = size_over_lifetimeOr document that size is for VFX events while size_over_lifetime is for particle systems.
Also applies to: 595-596
🤖 Prompt for AI Agents
In @Server/src/services/tools/manage_vfx.py around lines 449 - 450, The
params_dict currently uses the same key "size" for two different inputs (size
and size_over_lifetime), causing size_over_lifetime to silently overwrite size;
update both assignments to use distinct keys (e.g., params_dict["size"] for the
event-level size and params_dict["sizeOverLifetime"] or
params_dict["size_over_lifetime"] for the particle-system lifetime size)
wherever size_over_lifetime is set (the second occurrence around the later
block) and adjust any downstream consumers to read the new key (or preserve the
original "size" when appropriate to maintain backward compatibility), ensuring
both params_dict["size"] and the new params_dict["sizeOverLifetime"] are used
consistently.
|
@Scriptwonder this looks awesome, it'll be amazing to have a vfx tool. I haven't looked too close at the code but for recent PRs I've been trying to do two things:
And
That way we can not only avoid duplication but keep making the utilities and helpers stronger too. Just some thoughts as we go! The models are getting so good that we can get a lot of this code hygiene basically for free just by asking them while we develop. |
Maybe a benchmark or a .rule/.skill file revolving around these? I have tried to avoid redundancy (even worse is the repetitive generation), but it still takes some labor and careful checking to do so. |
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Yeah that's a good way to do it so it's basically automatic and agent-enforced. So not only do our coding agents refer to our architecture rules and existing utilities while we develop the repo, but they also have them for code review after they do a first draft, which I find super helpful (well before coderabbit feedback). |
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @Server/src/services/tools/manage_vfx.py:
- Line 357: The issue is that start_color and start_color_line both write to
params_dict["startColor"] so the latter silently overwrites the former; fix by
using distinct keys or conditional assignment: either write start_color_line
into a separate key (e.g., params_dict["startColorLine"] or
params_dict["lineStartColor"]) or only set params_dict["startColor"] for the
appropriate renderer type by checking the component/type before assigning (use
the same condition that differentiates Particle/VFX vs Line/Trail), and update
any downstream consumers to read the new key or respect the conditional so
values don't collide.
🧹 Nitpick comments (1)
Server/src/services/tools/manage_vfx.py (1)
42-42: Optional: Use iterable unpacking for cleaner list construction.The current list concatenation is correct, but using iterable unpacking
[..., *PARTICLE_ACTIONS, ...]can be slightly more idiomatic.🔎 Proposed refactor
-ALL_ACTIONS = ["ping"] + PARTICLE_ACTIONS + VFX_ACTIONS + LINE_ACTIONS + TRAIL_ACTIONS +ALL_ACTIONS = ["ping", *PARTICLE_ACTIONS, *VFX_ACTIONS, *LINE_ACTIONS, *TRAIL_ACTIONS]
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Server/src/services/tools/manage_vfx.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: JohanHoltby
Repo: CoplayDev/unity-mcp PR: 309
File: MCPForUnity/Editor/Helpers/ServerInstaller.cs:478-508
Timestamp: 2025-10-13T13:41:00.086Z
Learning: In the MCPForUnityTools feature (MCPForUnity/Editor/Helpers/ServerInstaller.cs), the design intentionally forces users to have only one .py file per MCPForUnityTools folder to keep file tracking simple. Package-style tools (subdirectories with __init__.py) are not supported.
🪛 Ruff (0.14.10)
Server/src/services/tools/manage_vfx.py
42-42: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
🔇 Additional comments (3)
Server/src/services/tools/manage_vfx.py (3)
45-125: Excellent documentation.The comprehensive docstring clearly explains prerequisites, targeting options, and action groups. This will greatly help users understand component requirements before attempting VFX operations.
299-326: Action validation correctly normalizes to lowercase.The implementation properly normalizes the action to lowercase before validation, ensuring consistency with Unity's case-insensitive action handling. The prefix-based error suggestions are helpful.
617-627: Clean parameter filtering and error handling.The None-value filtering before sending and result normalization ensure clean data transfer and consistent return types.
| if start_rotation is not None: | ||
| params_dict["startRotation"] = start_rotation | ||
| if start_color is not None: | ||
| params_dict["startColor"] = start_color |
There was a problem hiding this comment.
Parameter collision: start_color_line overwrites start_color.
Both start_color (line 357, for Particle/VFX) and start_color_line (line 496, for Line/Trail) map to params_dict["startColor"]. If both are provided, start_color_line will silently overwrite start_color since it's processed later.
🔎 Suggested fix: Use distinct keys or document mutual exclusivity
Option 1: Use different keys for line/trail renderers:
if start_color_line is not None:
- params_dict["startColor"] = start_color_line
+ params_dict["lineStartColor"] = start_color_lineOption 2: Document that these parameters are mutually exclusive based on component type (Particle/VFX vs Line/Trail).
Also applies to: 357-357
Also applies to: 496-496
🤖 Prompt for AI Agents
In @Server/src/services/tools/manage_vfx.py at line 357, The issue is that
start_color and start_color_line both write to params_dict["startColor"] so the
latter silently overwrites the former; fix by using distinct keys or conditional
assignment: either write start_color_line into a separate key (e.g.,
params_dict["startColorLine"] or params_dict["lineStartColor"]) or only set
params_dict["startColor"] for the appropriate renderer type by checking the
component/type before assigning (use the same condition that differentiates
Particle/VFX vs Line/Trail), and update any downstream consumers to read the new
key or respect the conditional so values don't collide.
Added 6 helper methods : ApplyCommonRendererProperties - Handles shadows, lighting, probes, sorting, rendering layer (used by ParticleSetRenderer, LineSetProperties, TrailSetProperties) GetCommonRendererInfo - Returns common renderer info for GetInfo methods ApplyWidthProperties - Generic width handling (used by LineSetWidth, TrailSetWidth) ApplyColorProperties - Generic color handling with fadeEndAlpha option (used by LineSetColor, TrailSetColor) SetRendererMaterial - Generic material assignment (used by LineSetMaterial, TrailSetMaterial) ApplyLineTrailProperties - Shared Line/Trail properties like alignment, textureMode, numCornerVertices (used by LineSetProperties, TrailSetProperties)
✅ Actions performedReview triggered.
|
|
@Scriptwonder I just updated main, primarily to help with asset store uploads. I made some changes to the README, you might want to pull in the latest main before adding this tool to the README. Also, can you split the ManageVFX logic into a few different files? If it's possible, it would be easier to read and update! |
Declutter the redundant files by adding RendererHelpers and VectorParsing.cs
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In @MCPForUnity/Editor/Helpers/RendererHelpers.cs:
- Around line 157-164: The code invokes several Action delegates without null
checks leading to possible NullReferenceExceptions; update the block that reads
@params keys so each delegate is null-checked before invocation (mirror the
existing checks for setLoop and setUseWorldSpace): ensure setNumCornerVertices,
setNumCapVertices, setAlignment, setTextureMode, and setGenerateLightingData are
each tested for null (e.g., "&& setNumCornerVertices != null") before calling
them and before adding their corresponding string to changes, and keep the
existing Enum.TryParse logic for setAlignment and setTextureMode.
In @MCPForUnity/Editor/Helpers/VectorParsing.cs:
- Around line 274-277: Multiple parsing methods use Debug.LogWarning instead of
the project logging helper; replace Debug.LogWarning calls in the catch blocks
of ParseVector4 (the catch with ex and message "[VectorParsing] Failed to parse
Vector4 from '{token}': {ex.Message}") and similarly for ParseGradient and
ParseAnimationCurve with McpLog.Warn, passing the same formatted message and the
exception (or include ex.Message) to maintain the same content and behavior;
locate the catch blocks inside the methods ParseVector4, ParseGradient, and
ParseAnimationCurve and swap Debug.LogWarning(...) to McpLog.Warn(...) so
logging is consistent with ParseVector3/ParseColor/ParseRect.
- Around line 232-236: The ParseColorOrDefault method incorrectly checks
defaultValue == default(Color) which treats transparent black as "unspecified";
change the API to accept a nullable Color (Nullable<Color> or Color?) or provide
an overload so callers can omit the default, then inside ParseColorOrDefault
check if defaultValue.HasValue and use defaultValue.Value otherwise use
Color.white; finally return ParseColor(token) ?? chosenDefault (or equivalent).
Ensure you update the method signature (ParseColorOrDefault) and usage sites to
pass a nullable Color or call the new overload, and keep ParseColor(token) as
the source of parsed value.
In @MCPForUnity/Editor/Tools/ManageVFX.cs:
- Around line 1646-1653: TrailClear does not record an Undo before mutating the
TrailRenderer; update the TrailClear method to call Undo.RecordObject(tr, "Clear
Trail") (or Undo.RegisterCompleteObjectUndo) on the TrailRenderer instance found
via FindTrailRenderer before calling tr.Clear(), then proceed to clear and
return the same result; also add a brief comment in the method noting that
TrailRenderer.Clear affects runtime-only trail data and may not be fully
restorable via Undo.
- Around line 889-903: The code builds a mix of absolute filesystem paths
(relativePath from file) and project-relative asset paths
(AssetDatabase.GUIDToAssetPath(guid)), causing duplicate-check mismatches in
VFXListTemplates; fix by normalizing the path stored in templates to a
project-relative form before adding and before comparing: convert file (the
absolute path) to a path that starts with "Assets/..." (e.g., use
UnityEditor.FileUtil.GetProjectRelativePath(file) or trim Application.dataPath
and prepend "Assets", then replace backslashes) and apply the same replacement
when checking packageInfo.resolvedPath so the isPackage test and the
templates.Any(t => ((dynamic)t).path == path) comparison use the same path
format.
- Around line 1412-1420: The LineRenderer is set to loop=true but you allocate
segments+1 positions and write both position 0 and position segments as the same
point, creating a duplicate vertex; fix by allocating exactly segments positions
(set lr.positionCount = segments) and change the loop to iterate i from 0 to i <
segments (compute angle as (float)i / segments * Mathf.PI * 2f) so you call
lr.SetPosition for indices 0..segments-1, avoiding the duplicate while keeping
lr.loop = true (references: lr, segments, positionCount, loop, SetPosition).
- Around line 687-718: Reflection calls to
UnityEditor.VFX.VisualEffectAssetEditorUtility.CreateNewAsset and
UnityEditor.VFX.VisualEffectResource.CreateNewAsset are fragile across VFX
Graph/Unity versions; update the code to (1) add a brief comment/guard
documenting supported VFX Graph package versions, (2) add explicit logs that
record which creation route succeeded (e.g.,
"VisualEffectAssetEditorUtility.CreateNewAsset succeeded" or
"VisualEffectResource.CreateNewAsset succeeded"), (3) replace the generic
failure message with specific diagnostics describing which reflection lookups
failed (e.g., "VisualEffectAssetEditorUtility type not found" / "CreateNewAsset
method not found on VisualEffectResource"), and (4) implement a stable fallback
using AssetDatabase.CreateAsset on a ScriptableObject-based VisualEffectAsset
placeholder if both reflection attempts fail, so code locates the logic in the
block referencing VisualEffectAssetEditorUtility, VisualEffectResource,
CreateNewAsset and AssetDatabase.LoadAssetAtPath.
🧹 Nitpick comments (2)
MCPForUnity/Editor/Tools/ManageVFX.cs (2)
550-554: Burst array initialization creates uninitialized slot before setting.The array is created with
idx + 1slots, butGetBurstsonly fillsidxslots (the existing bursts). The code then overwritesbursts[idx]with the new burst. This works correctly but is slightly confusing.A clearer approach would be to create an array of exactly
idxsize forGetBursts, then create the final array:🔎 Optional clarification
int idx = emission.burstCount; - var bursts = new ParticleSystem.Burst[idx + 1]; - emission.GetBursts(bursts); + var existingBursts = new ParticleSystem.Burst[idx]; + emission.GetBursts(existingBursts); + var bursts = new ParticleSystem.Burst[idx + 1]; + Array.Copy(existingBursts, bursts, idx); bursts[idx] = burst; emission.SetBursts(bursts);
1-26: Consider splitting this file for maintainability.As noted in PR comments by msanatan, this file is quite large (~1670 lines). Consider splitting into separate files per component type (e.g.,
ManageVFX.ParticleSystem.cs,ManageVFX.LineRenderer.cs) using partial classes.This would improve readability and make it easier for multiple contributors to work on different components simultaneously.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
MCPForUnity/Editor/Helpers/RendererHelpers.csMCPForUnity/Editor/Helpers/RendererHelpers.cs.metaMCPForUnity/Editor/Helpers/VectorParsing.csMCPForUnity/Editor/Tools/ManageVFX.cs
✅ Files skipped from review due to trivial changes (1)
- MCPForUnity/Editor/Helpers/RendererHelpers.cs.meta
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: JohanHoltby
Repo: CoplayDev/unity-mcp PR: 309
File: MCPForUnity/Editor/Helpers/ServerInstaller.cs:478-508
Timestamp: 2025-10-13T13:41:00.086Z
Learning: In the MCPForUnityTools feature (MCPForUnity/Editor/Helpers/ServerInstaller.cs), the design intentionally forces users to have only one .py file per MCPForUnityTools folder to keep file tracking simple. Package-style tools (subdirectories with __init__.py) are not supported.
📚 Learning: 2025-12-29T15:23:11.613Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:11.613Z
Learning: In MCPForUnity, prefer relying on the established testing process to catch UI initialization issues instead of adding defensive null checks for UI elements in editor windows. This means during reviews, verify that tests cover UI initialization paths and that code avoids repetitive null-check boilerplate in Editor Windows. If a UI element can be null, ensure there is a well-tested fallback or that its initialization is guaranteed by design, rather than sprinkling null checks throughout editor code.
Applied to files:
MCPForUnity/Editor/Tools/ManageVFX.csMCPForUnity/Editor/Helpers/VectorParsing.csMCPForUnity/Editor/Helpers/RendererHelpers.cs
🧬 Code graph analysis (3)
MCPForUnity/Editor/Tools/ManageVFX.cs (3)
MCPForUnity/Editor/Helpers/VectorParsing.cs (12)
Color(178-226)Color(232-236)VectorParsing(12-531)Vector3(19-52)Vector3(57-60)Vector4(245-280)Vector4(286-289)Gradient(300-373)Gradient(379-391)AnimationCurve(403-451)AnimationCurve(459-462)Vector2(67-98)MCPForUnity/Editor/Helpers/ObjectResolver.cs (2)
ObjectResolver(13-200)Texture(146-153)MCPForUnity/Editor/Helpers/RendererHelpers.cs (4)
RendererHelpers(13-166)ApplyCommonRendererProperties(20-45)SetRendererMaterial(130-145)ApplyLineTrailProperties(151-164)
MCPForUnity/Editor/Helpers/VectorParsing.cs (1)
MCPForUnity/Editor/Tools/ManageVFX.cs (4)
Color(81-81)Vector4(83-83)Gradient(84-84)AnimationCurve(85-86)
MCPForUnity/Editor/Helpers/RendererHelpers.cs (2)
MCPForUnity/Editor/Helpers/VectorParsing.cs (6)
AnimationCurve(403-451)AnimationCurve(459-462)Color(178-226)Color(232-236)Gradient(300-373)Gradient(379-391)MCPForUnity/Editor/Tools/ManageVFX.cs (4)
AnimationCurve(85-86)Color(81-81)Gradient(84-84)Material(91-92)
🔇 Additional comments (7)
MCPForUnity/Editor/Tools/ManageVFX.cs (2)
28-76: Well-structured command routing with proper error handling.The
HandleCommandmethod provides clean action routing with prefix-based dispatching. The try-catch wrapper and informative error messages are good practices for a tool API.
78-93: Good use of delegate wrappers for parsing utilities.The parsing delegates cleanly delegate to
VectorParsingandObjectResolver, keeping the tool focused on VFX logic while reusing shared helpers. This aligns with the PR's goal of consolidating utilities.MCPForUnity/Editor/Helpers/VectorParsing.cs (2)
300-373: Gradient parsing handles edge cases well.The
ParseGradientmethod supports both simple (startColor/endColor) and full (colorKeys/alphaKeys) formats, with sensible defaults when keys are missing. The alpha override viastartAlpha/endAlphais a nice touch for flexibility.
403-451: AnimationCurve parsing is comprehensive.Good support for multiple input formats: constant values, simple start/end, and full keyframe arrays with tangent control. The alias handling (
start/startValue,end/endValue) improves API usability.MCPForUnity/Editor/Helpers/RendererHelpers.cs (3)
20-45: Clean extraction of common renderer property handling.This consolidates shadow, lighting, probe, motion vector, and sorting layer logic that would otherwise be duplicated across ParticleSystemRenderer, LineRenderer, and TrailRenderer handlers. Good use of
Enum.TryParsefor safe enum handling.
75-91: Width property application is well-designed.The callback-based approach allows both LineRenderer and TrailRenderer to share this logic while handling their specific property setters. The support for both unified
widthand separatestartWidth/endWidthis user-friendly.
130-145: SetRendererMaterial properly handles Undo and dirty marking.Good defensive checks for null renderer and missing path. The Undo recording before modification ensures the operation is reversible.
| // Create empty VFX asset using reflection to access internal API | ||
| try | ||
| { | ||
| // Try to use VisualEffectAssetEditorUtility.CreateNewAsset if available | ||
| var utilityType = System.Type.GetType("UnityEditor.VFX.VisualEffectAssetEditorUtility, Unity.VisualEffectGraph.Editor"); | ||
| if (utilityType != null) | ||
| { | ||
| var createMethod = utilityType.GetMethod("CreateNewAsset", BindingFlags.Public | BindingFlags.Static); | ||
| if (createMethod != null) | ||
| { | ||
| createMethod.Invoke(null, new object[] { assetPath }); | ||
| AssetDatabase.Refresh(); | ||
| newAsset = AssetDatabase.LoadAssetAtPath<UnityEngine.VFX.VisualEffectAsset>(assetPath); | ||
| } | ||
| } | ||
|
|
||
| // Fallback: Create a ScriptableObject-based asset | ||
| if (newAsset == null) | ||
| { | ||
| // Try direct creation via internal constructor | ||
| var resourceType = System.Type.GetType("UnityEditor.VFX.VisualEffectResource, Unity.VisualEffectGraph.Editor"); | ||
| if (resourceType != null) | ||
| { | ||
| var createMethod = resourceType.GetMethod("CreateNewAsset", BindingFlags.Public | BindingFlags.Static | BindingFlags.NonPublic); | ||
| if (createMethod != null) | ||
| { | ||
| var resource = createMethod.Invoke(null, new object[] { assetPath }); | ||
| AssetDatabase.Refresh(); | ||
| newAsset = AssetDatabase.LoadAssetAtPath<UnityEngine.VFX.VisualEffectAsset>(assetPath); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the full context around the code in question
cat -n MCPForUnity/Editor/Tools/ManageVFX.cs | sed -n '660,740p'Repository: CoplayDev/unity-mcp
Length of output: 4732
🏁 Script executed:
# Check if there are any version guards or conditional compilation around this code
cat -n MCPForUnity/Editor/Tools/ManageVFX.cs | sed -n '680,720p'Repository: CoplayDev/unity-mcp
Length of output: 2660
🏁 Script executed:
# Look for any existing version checking patterns in the file
rg "UNITY_.*_OR_NEWER|EditorUserBuildSettingsUtils|Version" MCPForUnity/Editor/Tools/ManageVFX.cs -A 2 -B 2Repository: CoplayDev/unity-mcp
Length of output: 280
🌐 Web query:
Unity VFX Graph API stability VisualEffectAsset CreateNewAsset breaking changes between versions
💡 Result:
Short answer: there is no stable, documented public API named VisualEffectAsset.CreateNewAsset in Unity’s supported VFX Graph API. Unity exposes VisualEffectAsset (constructor and read-only members) and the Visual Effect component runtime API; creating assets is done via UnityEditor APIs (AssetDatabase.CreateAsset) or the VFX package/editor utilities, and the VFX package has had multiple internal/editor changes across versions — so relying on an undocumented CreateNewAsset (or internal editor method) is likely to break between package/Editor versions. [1][2][3]
Actionable recommendations:
- Use the supported APIs: create/save assets with UnityEditor.AssetDatabase.CreateAsset (or the ScriptableObject constructor + CreateAsset) and use the public VisualEffectAsset/VisualEffect component APIs at runtime. [1][2]
- When upgrading Unity or the com.unity.visualeffectgraph package, read that package’s CHANGELOG for breaking or editor-side changes (every release documents notable fixes/changes). Check the changelog for the exact package version you’re moving to. [3][4]
Sources:
- VisualEffectAsset (scripting API docs). [1]
- Visual Effect component / Component API (VFX Graph manual). [2]
- Visual Effect Graph package changelogs (example pages). [3][4]
If you want, tell me which Unity Editor version and Visual Effect Graph package version you’re targeting and I’ll check the changelogs and suggest the exact code pattern to create/save a VFX asset and notes about any breaking changes.
Reflection-based asset creation relies on undocumented internal APIs that change across Unity/VFX package versions.
The code at lines 687-718 uses reflection to access UnityEditor.VFX.VisualEffectAssetEditorUtility.CreateNewAsset and UnityEditor.VFX.VisualEffectResource.CreateNewAsset, neither of which are documented public APIs. The VFX Graph package has a history of internal/editor-side breaking changes across versions, making this approach fragile.
Consider:
- Document supported VFX Graph package versions and add a version guard or warning in comments
- Log which creation method succeeds (first or fallback) for debugging failed asset creation
- Improve error messages at line 728: replace the generic message with specific details about which reflection attempts failed (e.g., "VisualEffectAssetEditorUtility not found" vs "CreateNewAsset method not available")
- Consider using
AssetDatabase.CreateAssetwith a ScriptableObject subclass as a more stable fallback, since the internal APIs are undocumented and subject to change
🤖 Prompt for AI Agents
In @MCPForUnity/Editor/Tools/ManageVFX.cs around lines 687 - 718, Reflection
calls to UnityEditor.VFX.VisualEffectAssetEditorUtility.CreateNewAsset and
UnityEditor.VFX.VisualEffectResource.CreateNewAsset are fragile across VFX
Graph/Unity versions; update the code to (1) add a brief comment/guard
documenting supported VFX Graph package versions, (2) add explicit logs that
record which creation route succeeded (e.g.,
"VisualEffectAssetEditorUtility.CreateNewAsset succeeded" or
"VisualEffectResource.CreateNewAsset succeeded"), (3) replace the generic
failure message with specific diagnostics describing which reflection lookups
failed (e.g., "VisualEffectAssetEditorUtility type not found" / "CreateNewAsset
method not found on VisualEffectResource"), and (4) implement a stable fallback
using AssetDatabase.CreateAsset on a ScriptableObject-based VisualEffectAsset
placeholder if both reflection attempts fail, so code locates the logic in the
block referencing VisualEffectAssetEditorUtility, VisualEffectResource,
CreateNewAsset and AssetDatabase.LoadAssetAtPath.
Minor Fixes based on AI-feedback
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @MCPForUnity/Editor/Helpers/VectorParsing.cs:
- Around line 445-448: The ParseAnimationCurve method is using Debug.LogWarning
instead of the project logging utility; replace the Debug.LogWarning call in the
catch block of ParseAnimationCurve with McpLog.Warn, passing the same formatted
message and exception details (matching the pattern used by
ParseVector3/ParseColor) so parsing failures are logged consistently via
McpLog.Warn including the token and ex.Message.
- Around line 367-370: Replace the inconsistent Debug.LogWarning call in
ParseGradient with the project-standard McpLog.Warn so the method uses the same
logging facility as ParseVector3 and ParseColor; locate the catch block in the
ParseGradient method that currently calls Debug.LogWarning($"[VectorParsing]
Failed to parse Gradient from '{token}': {ex.Message}") and change it to call
McpLog.Warn with the same message and error details.
- Around line 232-236: The method ParseColorOrDefault misdetects an omitted
default because comparing defaultValue to default compares against
Color(0,0,0,0) and will override any caller-supplied transparent black; change
the parameter to a nullable Color (e.g., Color? defaultValue = null) and then
treat null as "no default provided" by setting defaultValue = Color.white when
null, and return ParseColor(token) ?? defaultValue.Value; update the
ParseColorOrDefault signature and its null-check accordingly.
In @MCPForUnity/Editor/Tools/ManageVFX.cs:
- Around line 1440-1449: The circle creation sets LineRenderer.loop = true but
also writes segments + 1 positions, duplicating the start point; in the method
where lr is configured (uses lr.positionCount, lr.loop and the for loop calling
lr.SetPosition), change to use lr.positionCount = segments and iterate for (int
i = 0; i < segments; i++) computing angle = (float)i / segments * Mathf.PI * 2f
and SetPosition(i, point) so the renderer has exactly one vertex per segment
with loop=true (alternatively, keep segments+1 and set loop=false, but prefer
reducing to segments to remove the redundant duplicate).
- Around line 687-742: The reflection calls to
UnityEditor.VFX.VisualEffectAssetEditorUtility.CreateNewAsset and
UnityEditor.VFX.VisualEffectResource.CreateNewAsset rely on internal VFX Graph
APIs and need safer handling: add a short comment above the block listing the
supported VFX Graph package version(s) you tested, instrument each reflection
step with Debug.Log/Debug.LogWarning/Debug.LogError so you log which attempt ran
and whether it succeeded (log when utilityType is null, when createMethod is
null, when resourceType is null, etc.), and improve the final failure return
(and the catch return) to include which reflection attempts failed or the caught
exception message (e.g., indicate "VisualEffectAssetEditorUtility not found;
VisualEffectResource.CreateNewAsset failed: <error>"). Update the return paths
that currently return simple messages to include these specific diagnostics
while keeping the existing assetPath/assetName/template outputs on success.
🧹 Nitpick comments (2)
MCPForUnity/Editor/Tools/ManageVFX.cs (2)
467-478: Inconsistent material resolution pattern.Lines 467-478 use
ManageGameObject.FindObjectByInstructionto resolve materials, while the class defines aFindMaterialByPathhelper (line 91-92) that delegates toObjectResolver.ResolveMaterial. For consistency, consider using the same resolution pattern throughout the class.🔎 Suggested refactor
// Material if (@params["materialPath"] != null) { - var findInst = new JObject { ["find"] = @params["materialPath"].ToString() }; - Material mat = ManageGameObject.FindObjectByInstruction(findInst, typeof(Material)) as Material; + Material mat = FindMaterialByPath(@params["materialPath"].ToString()); if (mat != null) { renderer.sharedMaterial = mat; changes.Add("material"); } } if (@params["trailMaterialPath"] != null) { - var findInst = new JObject { ["find"] = @params["trailMaterialPath"].ToString() }; - Material mat = ManageGameObject.FindObjectByInstruction(findInst, typeof(Material)) as Material; + Material mat = FindMaterialByPath(@params["trailMaterialPath"].ToString()); if (mat != null) { renderer.trailMaterial = mat; changes.Add("trailMaterial"); } }
1098-1100: Inconsistent asset resolution pattern in VFX methods.VFX methods use
ManageGameObject.FindObjectByInstructionto resolve textures and meshes, creating temporary JObject wrappers. Consider defining helper methods similar toFindMaterialByPathfor consistency across the codebase.🔎 Suggested refactor
Add helper methods near the existing resolution helpers (around lines 88-92):
private static Material FindMaterialByPath(string path) => ObjectResolver.ResolveMaterial(path); + private static Texture FindTextureByPath(string path) + => ObjectResolver.ResolveTexture(path);Then update the VFX methods:
private static object VFXSetTexture(JObject @params) { // ... - var findInst = new JObject { ["find"] = path }; - Texture tex = ManageGameObject.FindObjectByInstruction(findInst, typeof(Texture)) as Texture; + Texture tex = FindTextureByPath(path); // ... }Apply similar changes for mesh resolution.
Also applies to: 1118-1120
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
MCPForUnity/Editor/Helpers/RendererHelpers.csMCPForUnity/Editor/Helpers/VectorParsing.csMCPForUnity/Editor/Tools/ManageVFX.cs
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: JohanHoltby
Repo: CoplayDev/unity-mcp PR: 309
File: MCPForUnity/Editor/Helpers/ServerInstaller.cs:478-508
Timestamp: 2025-10-13T13:41:00.086Z
Learning: In the MCPForUnityTools feature (MCPForUnity/Editor/Helpers/ServerInstaller.cs), the design intentionally forces users to have only one .py file per MCPForUnityTools folder to keep file tracking simple. Package-style tools (subdirectories with __init__.py) are not supported.
📚 Learning: 2025-12-29T15:23:11.613Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:11.613Z
Learning: In MCPForUnity, prefer relying on the established testing process to catch UI initialization issues instead of adding defensive null checks for UI elements in editor windows. This means during reviews, verify that tests cover UI initialization paths and that code avoids repetitive null-check boilerplate in Editor Windows. If a UI element can be null, ensure there is a well-tested fallback or that its initialization is guaranteed by design, rather than sprinkling null checks throughout editor code.
Applied to files:
MCPForUnity/Editor/Helpers/VectorParsing.csMCPForUnity/Editor/Helpers/RendererHelpers.csMCPForUnity/Editor/Tools/ManageVFX.cs
🧬 Code graph analysis (3)
MCPForUnity/Editor/Helpers/VectorParsing.cs (2)
MCPForUnity/Editor/Tools/ManageGameObject.cs (1)
Color(1840-1853)MCPForUnity/Editor/Tools/ManageVFX.cs (4)
Color(81-81)Vector4(83-83)Gradient(84-84)AnimationCurve(85-86)
MCPForUnity/Editor/Helpers/RendererHelpers.cs (2)
MCPForUnity/Editor/Helpers/VectorParsing.cs (6)
AnimationCurve(403-451)AnimationCurve(459-462)Color(178-226)Color(232-236)Gradient(300-373)Gradient(379-391)MCPForUnity/Editor/Tools/ManageVFX.cs (4)
AnimationCurve(85-86)Color(81-81)Gradient(84-84)Material(91-92)
MCPForUnity/Editor/Tools/ManageVFX.cs (5)
MCPForUnity/Editor/Helpers/VectorParsing.cs (12)
Color(178-226)Color(232-236)VectorParsing(12-531)Vector3(19-52)Vector3(57-60)Vector4(245-280)Vector4(286-289)Gradient(300-373)Gradient(379-391)AnimationCurve(403-451)AnimationCurve(459-462)Vector2(67-98)MCPForUnity/Editor/Helpers/ObjectResolver.cs (2)
ObjectResolver(13-200)Texture(146-153)MCPForUnity/Editor/Tools/BatchExecute.cs (1)
JArray(153-161)MCPForUnity/Editor/Helpers/RendererHelpers.cs (4)
RendererHelpers(13-166)ApplyCommonRendererProperties(20-45)SetRendererMaterial(130-145)ApplyLineTrailProperties(151-164)MCPForUnity/Editor/Tools/ManageAsset.cs (2)
CreateFolder(279-332)DeleteAsset(495-523)
🔇 Additional comments (8)
MCPForUnity/Editor/Helpers/RendererHelpers.cs (6)
20-45: LGTM!The
ApplyCommonRendererPropertiesmethod correctly handles renderer properties with proper enum parsing, null checks, and change tracking.
50-62: LGTM!The
GetCommonRendererInfomethod provides a clean snapshot of common renderer properties, consistent with the properties managed byApplyCommonRendererProperties.
75-91: LGTM!The
ApplyWidthPropertiesmethod provides a clean abstraction for setting width-related properties. The required delegate parameters follow a consistent pattern throughout the helper class.
104-120: LGTM!The
ApplyColorPropertiesmethod provides flexible color configuration with thefadeEndAlphaparameter, which is useful for trail fade effects.
130-145: LGTM!The
SetRendererMaterialmethod handles material assignment with proper error handling, Undo support, and dirty marking.
151-164: LGTM!The
ApplyLineTrailPropertiesmethod correctly handles optional delegates with null checks, addressing the previously flagged issue. This allows the same helper to work for both LineRenderer (with loop/useWorldSpace) and TrailRenderer (without those properties).MCPForUnity/Editor/Tools/ManageVFX.cs (2)
28-76: LGTM!The
HandleCommandmethod provides clean action routing with proper error handling. The prefix-based routing (particle_, vfx_, line_, trail_) keeps the code organized and extensible.
80-92: LGTM!The helper delegates provide clean wrappers around VectorParsing and ObjectResolver, centralizing parsing and resolution logic for the entire tool.
| public static Color ParseColorOrDefault(JToken token, Color defaultValue = default) | ||
| { | ||
| if (defaultValue == default) defaultValue = Color.white; | ||
| return ParseColor(token) ?? defaultValue; | ||
| } |
There was a problem hiding this comment.
Unusual default value handling pattern.
The comparison if (defaultValue == default) for a Color struct compares against default(Color) which is (0,0,0,0) (transparent black), not Color.white. This means passing Color.black or any color with all zeros would unexpectedly be overridden to Color.white.
This was previously flagged and remains unresolved.
🔎 Suggested fix
- public static Color ParseColorOrDefault(JToken token, Color defaultValue = default)
- {
- if (defaultValue == default) defaultValue = Color.white;
- return ParseColor(token) ?? defaultValue;
- }
+ /// <summary>
+ /// Parses a JToken into a Color, returning Color.white if parsing fails and no default is specified.
+ /// </summary>
+ public static Color ParseColorOrDefault(JToken token) => ParseColor(token) ?? Color.white;
+
+ /// <summary>
+ /// Parses a JToken into a Color, returning the specified default if parsing fails.
+ /// </summary>
+ public static Color ParseColorOrDefault(JToken token, Color defaultValue) => ParseColor(token) ?? defaultValue;🤖 Prompt for AI Agents
In @MCPForUnity/Editor/Helpers/VectorParsing.cs around lines 232 - 236, The
method ParseColorOrDefault misdetects an omitted default because comparing
defaultValue to default compares against Color(0,0,0,0) and will override any
caller-supplied transparent black; change the parameter to a nullable Color
(e.g., Color? defaultValue = null) and then treat null as "no default provided"
by setting defaultValue = Color.white when null, and return ParseColor(token) ??
defaultValue.Value; update the ParseColorOrDefault signature and its null-check
accordingly.
| // Create empty VFX asset using reflection to access internal API | ||
| try | ||
| { | ||
| // Try to use VisualEffectAssetEditorUtility.CreateNewAsset if available | ||
| var utilityType = System.Type.GetType("UnityEditor.VFX.VisualEffectAssetEditorUtility, Unity.VisualEffectGraph.Editor"); | ||
| if (utilityType != null) | ||
| { | ||
| var createMethod = utilityType.GetMethod("CreateNewAsset", BindingFlags.Public | BindingFlags.Static); | ||
| if (createMethod != null) | ||
| { | ||
| createMethod.Invoke(null, new object[] { assetPath }); | ||
| AssetDatabase.Refresh(); | ||
| newAsset = AssetDatabase.LoadAssetAtPath<UnityEngine.VFX.VisualEffectAsset>(assetPath); | ||
| } | ||
| } | ||
|
|
||
| // Fallback: Create a ScriptableObject-based asset | ||
| if (newAsset == null) | ||
| { | ||
| // Try direct creation via internal constructor | ||
| var resourceType = System.Type.GetType("UnityEditor.VFX.VisualEffectResource, Unity.VisualEffectGraph.Editor"); | ||
| if (resourceType != null) | ||
| { | ||
| var createMethod = resourceType.GetMethod("CreateNewAsset", BindingFlags.Public | BindingFlags.Static | BindingFlags.NonPublic); | ||
| if (createMethod != null) | ||
| { | ||
| var resource = createMethod.Invoke(null, new object[] { assetPath }); | ||
| AssetDatabase.Refresh(); | ||
| newAsset = AssetDatabase.LoadAssetAtPath<UnityEngine.VFX.VisualEffectAsset>(assetPath); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| return new { success = false, message = $"Failed to create VFX asset: {ex.Message}" }; | ||
| } | ||
| } | ||
|
|
||
| if (newAsset == null) | ||
| { | ||
| return new { success = false, message = "Failed to create VFX asset. Try using a template from list_templates." }; | ||
| } | ||
|
|
||
| return new | ||
| { | ||
| success = true, | ||
| message = $"Created VFX asset: {assetPath}", | ||
| data = new | ||
| { | ||
| assetPath = assetPath, | ||
| assetName = newAsset.name, | ||
| template = template | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
Reflection-based asset creation relies on undocumented internal APIs.
The code at lines 687-718 uses reflection to access UnityEditor.VFX.VisualEffectAssetEditorUtility.CreateNewAsset and UnityEditor.VFX.VisualEffectResource.CreateNewAsset, neither of which are documented public APIs. The VFX Graph package has a history of internal changes across versions.
This was previously flagged and remains unresolved. Consider:
- Document supported VFX Graph package versions in code comments
- Add logging when each creation method succeeds/fails
- Improve the error message at line 728 with specific details about which reflection attempts failed
🤖 Prompt for AI Agents
In @MCPForUnity/Editor/Tools/ManageVFX.cs around lines 687 - 742, The reflection
calls to UnityEditor.VFX.VisualEffectAssetEditorUtility.CreateNewAsset and
UnityEditor.VFX.VisualEffectResource.CreateNewAsset rely on internal VFX Graph
APIs and need safer handling: add a short comment above the block listing the
supported VFX Graph package version(s) you tested, instrument each reflection
step with Debug.Log/Debug.LogWarning/Debug.LogError so you log which attempt ran
and whether it succeeded (log when utilityType is null, when createMethod is
null, when resourceType is null, etc.), and improve the final failure return
(and the catch return) to include which reflection attempts failed or the caught
exception message (e.g., indicate "VisualEffectAssetEditorUtility not found;
VisualEffectResource.CreateNewAsset failed: <error>"). Update the return paths
that currently return simple messages to include these specific diagnostics
while keeping the existing assetPath/assetName/template outputs on success.
Merged! Tested on both Windows/Mac on my end, looking good for the release. Tried to optimize the PR and will work on it more possibly later👍 |
上游變更 (v7.0.0 → v9.0.3)： - GameObject 工具集重構 (CoplayDev#518) - VFX 管理功能 (CoplayDev#520) - 批次執行錯誤處理改進 (CoplayDev#531) - Windows 長路徑問題修復 (CoplayDev#534) - HTTP/Stdio 傳輸 UX 改進 (CoplayDev#530) - LLM 工具註解功能 (CoplayDev#480) 衝突解決： - modify/delete：接受刪除（架構已重構） - content：接受 v9.0.3 版本（2020 相容性修正將另行處理）
3 participants
Add ManageVFX to the current function, support:
Tested on Unity 6.2 with Mac. Want to inform the team on this @msanatan @dsarno before merging, since this is just a building ground for VFX-type features (I am only touching the basics of visualEffectGraph!)
PS: Will rebase my main after this:)
Summary by Sourcery
Introduce a unified Manage VFX toolchain spanning Unity editor and server, enabling scripted control and configuration of particle systems, VFX Graph, line renderers, and trail renderers via a single MCP tool.
New Features:
Enhancements:
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.