Token Optimization for VFX #626
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Reviewer's GuideRefactors the manage_vfx interface to drastically reduce exposed parameters by moving most per-action arguments into a generic properties payload handled on the C# side, adds robust parameter normalization and aliases, introduces auto-assigned default materials for VFX-related renderers across pipelines, and updates CLI commands and VFX helper code accordingly. Sequence diagram for manage_vfx request flow with normalized propertiessequenceDiagram
actor User
participant CLI_vfx as cli_vfx_command
participant PyTool as manage_vfx_tool_py
participant Unity as Unity_MCP_connection
participant ManageVFX as ManageVFX_cs
participant VfxHandler as VFX_or_Particle_Line_Trail_handler
participant RendererHelpers as RendererHelpers
participant RenderPipelineUtil as RenderPipelineUtility
User->>CLI_vfx: run vfx subcommand
CLI_vfx->>CLI_vfx: build params dict
CLI_vfx->>CLI_vfx: _normalize_vfx_params
CLI_vfx->>PyTool: run_command manage_vfx(normalized params)
PyTool->>PyTool: normalize_action
PyTool->>PyTool: params_dict = {action, target, searchMethod}
PyTool->>PyTool: attach properties payload
PyTool->>Unity: send manage_vfx request
Unity->>ManageVFX: HandleCommand(JObject params)
ManageVFX->>ManageVFX: NormalizeParams
ManageVFX->>ManageVFX: ExtractProperties(properties)
ManageVFX->>ManageVFX: NormalizeKey and NormalizeToken
ManageVFX->>VfxHandler: HandleParticleSystemAction or HandleVFXGraphAction or HandleLineRendererAction or HandleTrailRendererAction
VfxHandler->>RendererHelpers: EnsureMaterial(renderer)
RendererHelpers->>RenderPipelineUtil: GetOrCreateDefaultVFXMaterial(VFXComponentType)
RenderPipelineUtil-->>RendererHelpers: default Material
RendererHelpers-->>VfxHandler: renderer.sharedMaterial assigned
VfxHandler-->>ManageVFX: result payload
ManageVFX-->>Unity: result
Unity-->>PyTool: result
PyTool-->>CLI_vfx: result
CLI_vfx-->>User: formatted output
Class diagram for updated VFX management and helpersclassDiagram
class ManageVFX {
+static Dictionary~string,string~ ParamAliases
+static object HandleCommand(JObject params)
-static JObject NormalizeParams(JObject source)
-static JObject ExtractProperties(JObject source)
-static string NormalizeKey(string key, bool allowAliases)
-static JToken NormalizeToken(JToken token)
-static string ToCamelCase(string key)
}
class RendererHelpers {
+static void EnsureMaterial(Renderer renderer)
+static void ApplyCommonRendererProperties(JObject params, List~string~ changes, Action~bool~ setReceiveShadows, Action~UnityEngine.Rendering.ShadowCastingMode~ setShadowCastingMode, Action~UnityEngine.Rendering.LightProbeUsage~ setLightProbeUsage, Action~UnityEngine.Rendering.ReflectionProbeUsage~ setReflectionProbeUsage, Action~UnityEngine.Rendering.MotionVectorGenerationMode~ setMotionVectorGenerationMode, Action~int~ setSortingLayerID, Action~string~ setSortingLayerName, Action~int~ setSortingOrder, Action~int~ setRenderingLayerMask)
+static void ApplyColorProperties(JObject params, List~string~ changes, Action~Color~ setColor, Action~Gradient~ setGradient, Action~Color~ setStartColor, Action~Color~ setEndColor)
+static object SetRendererMaterial(Renderer renderer, JObject params, string undoName, Func~string,Material~ findMaterial, bool autoAssignDefault)
}
class RenderPipelineUtility {
<<static>>
enum PipelineKind
enum VFXComponentType
+static PipelineKind GetActivePipeline()
+static Material GetOrCreateDefaultVFXMaterial(VFXComponentType componentType)
-static Shader ResolveDefaultUnlitShader(PipelineKind pipeline)
-static void WarnIfPipelineMismatch(string shaderName, PipelineKind active, PipelineKind expected)
}
class LineCreate {
+static object CreateLine(JObject params)
+static object CreateCircle(JObject params)
+static object CreateArc(JObject params)
+static object CreateBezier(JObject params)
}
class LineWrite {
+static object SetPositions(JObject params)
+static object AddPosition(JObject params)
+static object SetPosition(JObject params)
+static object SetWidth(JObject params)
+static object SetColor(JObject params)
+static object SetProperties(JObject params)
}
class TrailWrite {
+static object SetTime(JObject params)
+static object SetWidth(JObject params)
+static object SetColor(JObject params)
+static object SetProperties(JObject params)
}
class ParticleWrite {
+static object SetMain(JObject params)
+static object SetEmission(JObject params)
+static object SetShape(JObject params)
+static object SetColorOverLifetime(JObject params)
+static object SetSizeOverLifetime(JObject params)
+static object SetVelocityOverLifetime(JObject params)
+static object SetNoise(JObject params)
+static object SetRenderer(JObject params)
}
class ParticleControl {
+static object Control(JObject params, string action)
+static object AddBurst(JObject params)
}
class TrailControl {
+static object Emit(JObject params)
}
ManageVFX ..> RendererHelpers : uses EnsureMaterial via handlers
ManageVFX ..> RenderPipelineUtility : uses through RendererHelpers
LineCreate ..> RendererHelpers : EnsureMaterial
LineWrite ..> RendererHelpers : EnsureMaterial
TrailWrite ..> RendererHelpers : EnsureMaterial
ParticleWrite ..> RendererHelpers : EnsureMaterial
ParticleControl ..> RendererHelpers : EnsureMaterial
TrailControl ..> RendererHelpers : EnsureMaterial
RendererHelpers ..> RenderPipelineUtility : GetOrCreateDefaultVFXMaterial
RenderPipelineUtility o-- RenderPipelineUtility.VFXComponentType
RenderPipelineUtility o-- RenderPipelineUtility.PipelineKind
Flow diagram for VFX parameter normalization and material auto-assignmentflowchart TD
A[CLI builds params dict] --> B[_normalize_vfx_params]
B -->|Move non top-level keys| C[properties dict]
B --> D[params with action,target,searchMethod,properties]
C --> D
D --> E[manage_vfx Python tool]
E -->|wraps properties if provided| F[Unity ManageVFX.HandleCommand]
F --> G[NormalizeParams]
G --> H[ExtractProperties from properties key]
H --> I[NormalizeKey and NormalizeToken]
I --> J[normalizedParams JObject]
J --> K{action prefix}
K -->|particle_*| L[HandleParticleSystemAction]
K -->|vfx_*| M[HandleVFXGraphAction]
K -->|line_*| N[HandleLineRendererAction]
K -->|trail_*| O[HandleTrailRendererAction]
L --> P[ParticleWrite or ParticleControl]
N --> Q[LineCreate or LineWrite]
O --> R[TrailWrite or TrailControl]
P --> S[RendererHelpers.EnsureMaterial]
Q --> S
R --> S
S --> T[RenderPipelineUtility.GetOrCreateDefaultVFXMaterial]
T --> U[default Material cached per pipeline and VFXComponentType]
U --> V[renderer.sharedMaterial assigned]
V --> W[Apply action-specific changes]
W --> X[result success,message]
X --> Y[return to CLI and user]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds pipeline-aware default VFX material creation/caching and automatic material assignment across Particle/Line/Trail handlers; introduces parameter-normalization for VFX commands and consolidates server manage_vfx to accept a single properties payload. Changes
Sequence Diagram(s)sequenceDiagram
participant User as CLI User
participant CLI as Server CLI (commands/vfx.py)
participant Server as Server/manage_vfx
participant Unity as Unity Editor (ManageVFX)
participant RenderUtil as RenderPipelineUtility / RendererHelpers
User->>CLI: send VFX command
CLI->>Server: run_command("manage_vfx", _normalize_vfx_params(params))
Server->>Unity: run_command("manage_vfx", {action, target, properties})
Unity->>Unity: NormalizeParams -> determine action and target
Unity->>RenderUtil: EnsureMaterial / GetOrCreateDefaultVFXMaterial (if renderer missing material)
RenderUtil-->>Unity: return Material (cached or created)
Unity-->>Server: return result (success/warnings/errors)
Server-->>CLI: forward response
CLI-->>User: display result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
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. ✨ Finishing touches
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}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new
NormalizeParamsinManageVFX.csmergespropertiesinto the top-level params with top-level keys taking precedence; if that’s intentional, consider adding a brief comment, otherwise you may want to invert the precedence or explicitly resolve collisions to avoid surprising overrides. - In
vfx.py,_normalize_vfx_paramshas its own hard-coded set of top-level keys; to avoid drift with the C# side (e.g., if more top-level keys are added later), consider centralizing or deriving this list from a single source or at least adding a TODO to keep it aligned withManageVFX.NormalizeParams. - The param alias map in
ManageVFX.ParamAliasescurrently only covers a handful of legacy keys; if you expect more existing callers to rely on old parameter names, it may be worth expanding this map or documenting the breaking changes so consumers know which keys are still auto-normalized versus which now must go throughproperties.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `NormalizeParams` in `ManageVFX.cs` merges `properties` into the top-level params with top-level keys taking precedence; if that’s intentional, consider adding a brief comment, otherwise you may want to invert the precedence or explicitly resolve collisions to avoid surprising overrides.
- In `vfx.py`, `_normalize_vfx_params` has its own hard-coded set of top-level keys; to avoid drift with the C# side (e.g., if more top-level keys are added later), consider centralizing or deriving this list from a single source or at least adding a TODO to keep it aligned with `ManageVFX.NormalizeParams`.
- The param alias map in `ManageVFX.ParamAliases` currently only covers a handful of legacy keys; if you expect more existing callers to rely on old parameter names, it may be worth expanding this map or documenting the breaking changes so consumers know which keys are still auto-normalized versus which now must go through `properties`.
## Individual Comments
### Comment 1
<location> `Server/src/cli/commands/vfx.py:26-27` </location>
<code_context>
+_VFX_TOP_LEVEL_KEYS = {"action", "target", "searchMethod", "properties"}
+
+
+def _normalize_vfx_params(params: dict[str, Any]) -> dict[str, Any]:
+ params = dict(params)
+ properties: dict[str, Any] = {}
+ for key in list(params.keys()):
+ if key in _VFX_TOP_LEVEL_KEYS:
+ continue
+ properties[key] = params.pop(key)
+
+ if properties:
+ existing = params.get("properties")
+ if isinstance(existing, dict):
+ params["properties"] = {**properties, **existing}
+ else:
</code_context>
<issue_to_address>
**suggestion (bug_risk):** CLI flag precedence vs JSON `properties` may be inverted in merge logic
In `_normalize_vfx_params`, when both top-level keys and an existing `properties` dict are present, `params["properties"] = {**properties, **existing}` causes existing `properties` values to override CLI flags for overlapping keys. If CLI flags are meant to take precedence over the JSON payload (which is typical), the merge order should be `{**existing, **properties}` instead. Please confirm and adjust to match the intended precedence before this becomes relied-on behavior.
```suggestion
if isinstance(existing, dict):
# Existing JSON `properties` are the base; CLI/top-level flags take precedence.
params["properties"] = {**existing, **properties}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+26
to
+27
| if isinstance(existing, dict): | ||
| params["properties"] = {**properties, **existing} |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): CLI flag precedence vs JSON properties may be inverted in merge logic
In _normalize_vfx_params, when both top-level keys and an existing properties dict are present, params["properties"] = {**properties, **existing} causes existing properties values to override CLI flags for overlapping keys. If CLI flags are meant to take precedence over the JSON payload (which is typical), the merge order should be {**existing, **properties} instead. Please confirm and adjust to match the intended precedence before this becomes relied-on behavior.
Suggested change
| if isinstance(existing, dict): | |
| params["properties"] = {**properties, **existing} | |
| if isinstance(existing, dict): | |
| # Existing JSON `properties` are the base; CLI/top-level flags take precedence. | |
| params["properties"] = {**existing, **properties} |
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors the VFX tool interface to reduce token usage by moving most action-specific parameters into a single properties payload, and shifts more parameter normalization/material handling to the Unity (C#) side.
Changes:
- Simplified
manage_vfxtool schema to{action, target, searchMethod, properties}and updated CLI to pack extra args intoproperties. - Added Unity-side parameter normalization (snake_case → camelCase, aliasing) and expanded VFX tool documentation/comments in
ManageVFX.cs. - Introduced automatic default material assignment for Particle/Line/Trail renderers, plus additional property handling in several write/control paths.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| Server/src/services/tools/manage_vfx.py | Collapses many parameters into a single properties payload for reduced token usage. |
| Server/src/services/tools/manage_texture.py | Minor description string formatting tweak. |
| Server/src/cli/commands/vfx.py | Adds _normalize_vfx_params and routes CLI requests through properties. |
| MCPForUnity/Editor/Tools/Vfx/TrailWrite.cs | Ensures materials and expands trail_set_properties handling (material/time/width). |
| MCPForUnity/Editor/Tools/Vfx/TrailControl.cs | Ensures material before trail_emit. |
| MCPForUnity/Editor/Tools/Vfx/ParticleWrite.cs | Ensures material in multiple particle write actions; improves renderer material logging and duration-change behavior. |
| MCPForUnity/Editor/Tools/Vfx/ParticleControl.cs | Ensures material before particle play/restart and burst operations. |
| MCPForUnity/Editor/Tools/Vfx/ManageVFX.cs | Adds parameter normalization + aliases and expands inline documentation. |
| MCPForUnity/Editor/Tools/Vfx/LineWrite.cs | Ensures material and expands line_set_properties handling (material/positions/positionCount). |
| MCPForUnity/Editor/Tools/Vfx/LineCreate.cs | Ensures material and applies optional width/color for created shapes. |
| MCPForUnity/Editor/Tools/ManageScript.cs | Uses ToLowerInvariant() for action normalization. |
| MCPForUnity/Editor/Helpers/RendererHelpers.cs | Adds EnsureMaterial and extends SetRendererMaterial to support default assignment. |
| MCPForUnity/Editor/Helpers/RenderPipelineUtility.cs | Adds default VFX material creation/caching per render pipeline + component type. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| Material defaultMat = RenderPipelineUtility.GetOrCreateDefaultVFXMaterial(componentType.Value); | ||
| if (defaultMat != null) | ||
| { |
There was a problem hiding this comment.
EnsureMaterial mutates renderer.sharedMaterial without recording an Undo step or marking the object dirty. Since many call sites invoke EnsureMaterial(...) before Undo.RecordObject(...), the auto-assigned material change won’t be undoable and may not persist reliably in editor workflows. Consider either (a) moving EnsureMaterial calls to after Undo.RecordObject, or (b) updating EnsureMaterial to handle Undo/SetDirty (e.g., accept an optional undo label and call Undo.RecordObject / EditorUtility.SetDirty when it assigns a material).
Suggested change
| { | |
| { | |
| Undo.RecordObject(renderer, "Assign default VFX material"); | |
| EditorUtility.SetDirty(renderer); |
Comment on lines
189
to
191
| catch (JsonException) | ||
| { | ||
| return null; |
There was a problem hiding this comment.
ExtractProperties silently swallows JsonException when properties is provided as a JSON string. This makes invalid JSON inputs fail “quietly” (properties are ignored) and can be very hard to debug from the caller side. Consider surfacing a clear error (e.g., throw with a descriptive message so HandleCommand returns it), or at least logging a warning that properties JSON could not be parsed.
Suggested change
| catch (JsonException) | |
| { | |
| return null; | |
| catch (JsonException ex) | |
| { | |
| throw new JsonException( | |
| $"Failed to parse 'properties' JSON string. Raw value: {token}", | |
| ex); |
Comment on lines
+16
to
+31
| def _normalize_vfx_params(params: dict[str, Any]) -> dict[str, Any]: | ||
| params = dict(params) | ||
| properties: dict[str, Any] = {} | ||
| for key in list(params.keys()): | ||
| if key in _VFX_TOP_LEVEL_KEYS: | ||
| continue | ||
| properties[key] = params.pop(key) | ||
|
|
||
| if properties: | ||
| existing = params.get("properties") | ||
| if isinstance(existing, dict): | ||
| params["properties"] = {**properties, **existing} | ||
| else: | ||
| params["properties"] = properties | ||
|
|
||
| return {k: v for k, v in params.items() if v is not None} |
There was a problem hiding this comment.
_normalize_vfx_params is new behavior that changes the wire format (moves non-top-level keys into properties). Existing CLI tests exercise VFX commands, but none assert the request payload shape, so regressions here could go unnoticed. Add targeted unit tests that verify keys like time, positions, etc. end up under properties while action/target/searchMethod stay top-level, including the merge behavior when properties is already supplied.
| EditorUtility.SetDirty(ps); | ||
|
|
||
| // Restart particle system if it was playing | ||
| if (needsStop && wasPlaying) |
There was a problem hiding this comment.
Condition is always true because of access to local variable needsStop.
Suggested change
| if (needsStop && wasPlaying) | |
| if (needsStop) |
| TrailRenderer | ||
| } | ||
|
|
||
| private static Dictionary<string, Material> s_DefaultVFXMaterials = new Dictionary<string, Material>(); |
There was a problem hiding this comment.
Field 's_DefaultVFXMaterials' can be 'readonly'.
Suggested change
| private static Dictionary<string, Material> s_DefaultVFXMaterials = new Dictionary<string, Material>(); | |
| private static readonly Dictionary<string, Material> s_DefaultVFXMaterials = new Dictionary<string, Material>(); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@MCPForUnity/Editor/Tools/Vfx/LineWrite.cs`:
- Around line 16-20: Move the positions validation before any renderer mutation:
check `@params`["positions"] into posArr and return early if null before calling
RendererHelpers.EnsureMaterial(lr); in the LineWrite handling (referencing
posArr, `@params`["positions"], RendererHelpers.EnsureMaterial and lr), so
EnsureMaterial is only invoked when positions are present.
- Around line 126-163: The code currently treats an empty JArray for
`@params`["positions"] as "do nothing" and also prevents the positionCount
fallback; update the positions handling in LineWrite.cs so that when
`@params`["positions"] is present you always handle it: cast to JArray (posArr)
and if posArr.Count > 0 parse and SetPositions as before, otherwise clear the
line by setting lr.positionCount = 0 (and/or lr.SetPositions(new Vector3[0]))
and record changes.Add("positions(0)"); also change the subsequent positionCount
check so it runs only when `@params`["positions"] is null (i.e., keep it as an
else branch based on presence of `@params`["positions"] rather than posArr.Count).
♻️ Duplicate comments (1)
MCPForUnity/Editor/Tools/Vfx/LineWrite.cs (1)
58-61: Duplicate: validate before mutating renderer state.Same concern as above for invalid indices in
SetPosition.
🧹 Nitpick comments (8)
Server/src/services/tools/manage_vfx.py (1)
42-42: Consider iterable unpacking for cleaner list construction.Per static analysis (RUF005), iterable unpacking is preferred over concatenation for combining lists.
Suggested refactor
-ALL_ACTIONS = ["ping"] + PARTICLE_ACTIONS + VFX_ACTIONS + LINE_ACTIONS + TRAIL_ACTIONS +ALL_ACTIONS = ["ping", *PARTICLE_ACTIONS, *VFX_ACTIONS, *LINE_ACTIONS, *TRAIL_ACTIONS]MCPForUnity/Editor/Tools/Vfx/TrailWrite.cs (2)
100-112: Consider usingRendererHelpers.ApplyWidthPropertiesinstead of duplicating width logic.The
SetWidthmethod (lines 38-41) already usesRendererHelpers.ApplyWidthProperties, which also handleswidthCurveandwidthMultiplier. This inline implementation misses those parameters and duplicates logic.♻️ Suggested refactor
- // Handle width properties if provided - if (`@params`["width"] != null || `@params`["startWidth"] != null || `@params`["endWidth"] != null) - { - if (`@params`["width"] != null) - { - float w = `@params`["width"].ToObject<float>(); - tr.startWidth = w; - tr.endWidth = w; - changes.Add("width"); - } - if (`@params`["startWidth"] != null) { tr.startWidth = `@params`["startWidth"].ToObject<float>(); changes.Add("startWidth"); } - if (`@params`["endWidth"] != null) { tr.endWidth = `@params`["endWidth"].ToObject<float>(); changes.Add("endWidth"); } - } + // Handle width properties using shared helper + RendererHelpers.ApplyWidthProperties(`@params`, changes, + v => tr.startWidth = v, v => tr.endWidth = v, + v => tr.widthCurve = v, v => tr.widthMultiplier = v, + ManageVfxCommon.ParseAnimationCurve);
82-95: Material assignment doesn't record undo.The material assignment at line 88 modifies
tr.sharedMaterialbutUndo.RecordObjectwas called at line 79 before this block. However, if only the material is changed (and no other properties), the undo will still capture it. This is fine, but consider addingUndo.RecordObjectspecifically for material changes for consistency withSetMaterialwhich usesRendererHelpers.SetRendererMaterial.MCPForUnity/Editor/Tools/Vfx/ParticleWrite.cs (3)
53-58: Redundant condition check.The condition
needsStop && wasPlayingis redundant sinceneedsStopis already defined as@params["duration"] != null && wasPlayingon line 26. IfneedsStopis true,wasPlayingis guaranteed to be true.♻️ Simplify condition
- if (needsStop && wasPlaying) + if (needsStop)
68-73: Repetitive material initialization pattern across multiple methods.The same 5-line block for getting the renderer and ensuring material is repeated in SetEmission, SetShape, SetColorOverLifetime, SetSizeOverLifetime, SetVelocityOverLifetime, and SetNoise. Consider extracting to a helper method if this pattern continues to expand.
♻️ Example helper extraction
private static void EnsureParticleSystemMaterial(ParticleSystem ps) { var renderer = ps.GetComponent<ParticleSystemRenderer>(); if (renderer != null) { RendererHelpers.EnsureMaterial(renderer); } }Then each method would simply call:
EnsureParticleSystemMaterial(ps);
284-289: Missing warning log fortrailMaterialPathnot found.For consistency with the
materialPathhandling above (lines 278-281), consider adding a warning whentrailMaterialPathcannot be resolved.♻️ Add warning for consistency
if (`@params`["trailMaterialPath"] != null) { - var findInst = new JObject { ["find"] = `@params`["trailMaterialPath"].ToString() }; + string trailMatPath = `@params`["trailMaterialPath"].ToString(); + var findInst = new JObject { ["find"] = trailMatPath }; Material mat = ObjectResolver.Resolve(findInst, typeof(Material)) as Material; - if (mat != null) { renderer.trailMaterial = mat; changes.Add("trailMaterial"); } + if (mat != null) + { + renderer.trailMaterial = mat; + changes.Add($"trailMaterial={mat.name}"); + } + else + { + McpLog.Warn($"Trail material not found at path: {trailMatPath}. Keeping existing trail material."); + } }MCPForUnity/Editor/Tools/Vfx/ManageVFX.cs (2)
183-193: Consider logging JSON parse failures.When
propertiesis a string that fails to parse as JSON, the error is silently swallowed. This could make debugging difficult for API consumers who accidentally pass malformed JSON.♻️ Add warning log
if (token.Type == JTokenType.String) { try { return JToken.Parse(token.ToString()) as JObject; } catch (JsonException) { + McpLog.Warn($"Failed to parse 'properties' string as JSON: {token.ToString().Substring(0, Math.Min(100, token.ToString().Length))}..."); return null; } }
313-316: Consider whether to expose stack traces in error responses.Including
stackTracein the error response is helpful for debugging but may expose internal implementation details. Since this is a Unity Editor tool (not production runtime), this is likely acceptable, but worth considering for security-conscious deployments.
Comment on lines
+16
to
20
| RendererHelpers.EnsureMaterial(lr); | ||
|
|
||
| JArray posArr = @params["positions"] as JArray; | ||
| if (posArr == null) return new { success = false, message = "Positions array required" }; | ||
|
|
Contributor
There was a problem hiding this comment.
Avoid side effects when the positions array is missing.
EnsureMaterial runs before validating positions, so an invalid request still mutates the renderer. Consider validating first and only then mutating.
🔧 Proposed fix
- RendererHelpers.EnsureMaterial(lr);
-
- JArray posArr = `@params`["positions"] as JArray;
- if (posArr == null) return new { success = false, message = "Positions array required" };
+ JArray posArr = `@params`["positions"] as JArray;
+ if (posArr == null) return new { success = false, message = "Positions array required" };
+
+ RendererHelpers.EnsureMaterial(lr);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| RendererHelpers.EnsureMaterial(lr); | |
| JArray posArr = @params["positions"] as JArray; | |
| if (posArr == null) return new { success = false, message = "Positions array required" }; | |
| JArray posArr = `@params`["positions"] as JArray; | |
| if (posArr == null) return new { success = false, message = "Positions array required" }; | |
| RendererHelpers.EnsureMaterial(lr); |
🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/Tools/Vfx/LineWrite.cs` around lines 16 - 20, Move the
positions validation before any renderer mutation: check `@params`["positions"]
into posArr and return early if null before calling
RendererHelpers.EnsureMaterial(lr); in the LineWrite handling (referencing
posArr, `@params`["positions"], RendererHelpers.EnsureMaterial and lr), so
EnsureMaterial is only invoked when positions are present.
Comment on lines
+126
to
+163
| // Handle material if provided | ||
| if (@params["materialPath"] != null) | ||
| { | ||
| Material mat = ManageVfxCommon.FindMaterialByPath(@params["materialPath"].ToString()); | ||
| if (mat != null) | ||
| { | ||
| lr.sharedMaterial = mat; | ||
| changes.Add($"material={mat.name}"); | ||
| } | ||
| else | ||
| { | ||
| McpLog.Warn($"Material not found: {@params["materialPath"]}"); | ||
| } | ||
| } | ||
|
|
||
| // Handle positions if provided | ||
| if (@params["positions"] != null) | ||
| { | ||
| JArray posArr = @params["positions"] as JArray; | ||
| if (posArr != null && posArr.Count > 0) | ||
| { | ||
| var positions = new Vector3[posArr.Count]; | ||
| for (int i = 0; i < posArr.Count; i++) | ||
| { | ||
| positions[i] = ManageVfxCommon.ParseVector3(posArr[i]); | ||
| } | ||
| lr.positionCount = positions.Length; | ||
| lr.SetPositions(positions); | ||
| changes.Add($"positions({positions.Length})"); | ||
| } | ||
| } | ||
| else if (@params["positionCount"] != null) | ||
| { | ||
| int count = @params["positionCount"].ToObject<int>(); | ||
| lr.positionCount = count; | ||
| changes.Add("positionCount"); | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
Empty positions arrays are ignored.
When positions is provided but empty, the branch does nothing and also blocks the positionCount fallback. This makes positions: [] unable to clear the line. Consider treating empty arrays as a valid clear.
🔧 Proposed fix
- if (posArr != null && posArr.Count > 0)
+ if (posArr != null)
{
var positions = new Vector3[posArr.Count];
for (int i = 0; i < posArr.Count; i++)
{
positions[i] = ManageVfxCommon.ParseVector3(posArr[i]);
}
lr.positionCount = positions.Length;
lr.SetPositions(positions);
changes.Add($"positions({positions.Length})");
}🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/Tools/Vfx/LineWrite.cs` around lines 126 - 163, The code
currently treats an empty JArray for `@params`["positions"] as "do nothing" and
also prevents the positionCount fallback; update the positions handling in
LineWrite.cs so that when `@params`["positions"] is present you always handle it:
cast to JArray (posArr) and if posArr.Count > 0 parse and SetPositions as
before, otherwise clear the line by setting lr.positionCount = 0 (and/or
lr.SetPositions(new Vector3[0])) and record changes.Add("positions(0)"); also
change the subsequent positionCount check so it runs only when
`@params`["positions"] is null (i.e., keep it as an else branch based on presence
of `@params`["positions"] rather than posArr.Count).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@MCPForUnity/Editor/Tools/Vfx/ManageVFX.cs`:
- Around line 183-194: The catch in ExtractProperties references an undeclared
variable `ex`; update the catch clause to declare the exception (e.g., change
`catch (JsonException)` to `catch (JsonException ex)`) and pass that `ex` into
the thrown JsonException so the inner exception is available (keep the existing
throw that uses the message and `ex`). Locate this in the token.Type ==
JTokenType.String branch near the JObject parsing logic.
Comment on lines
183
to
194
| if (token.Type == JTokenType.String) | ||
| { | ||
| try | ||
| { | ||
| return JToken.Parse(token.ToString()) as JObject; | ||
| } | ||
| catch (JsonException) | ||
| { | ||
| throw new JsonException( | ||
| $"Failed to parse 'properties' JSON string. Raw value: {token}", | ||
| ex); | ||
| } |
Contributor
There was a problem hiding this comment.
Fix compile error in ExtractProperties catch.
ex is referenced but never declared, which breaks the build.
🐛 Proposed fix
- catch (JsonException)
+ catch (JsonException ex)
{
throw new JsonException(
$"Failed to parse 'properties' JSON string. Raw value: {token}",
ex);
}🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/Tools/Vfx/ManageVFX.cs` around lines 183 - 194, The catch
in ExtractProperties references an undeclared variable `ex`; update the catch
clause to declare the exception (e.g., change `catch (JsonException)` to `catch
(JsonException ex)`) and pass that `ex` into the thrown JsonException so the
inner exception is available (keep the existing throw that uses the message and
`ex`). Locate this in the token.Type == JTokenType.String branch near the
JObject parsing logic.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Optimize token usage for VFX component.
Type of Change
Save your change type
Changes Made
Shorten the description and cut the token usage by half, also manage to sideload a lot of parameter assignment to C# side.
Testing/Screenshots/Recordings
Before Change:
After Change:
Documentation Updates
tools/UPDATE_DOCS_PROMPT.md(recommended)tools/UPDATE_DOCS.mdRelated Issues
Additional Notes
Summary by Sourcery
Simplify the manage_vfx tool interface by moving most action-specific parameters into a generic properties payload handled on the C# side, while improving defaults and documentation for VFX components.
Enhancements:
Documentation:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.