fix(niagara): emitter resolution, honor emitterName, real accel/size/color modules, stack-module readback#480
Conversation
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughFour Niagara authoring handler files are updated: emitter resolution gains a single-emitter fallback and richer error messages; ChangesNiagara Authoring Improvements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/NiagaraAuthoring/McpAutomationBridge_NiagaraAuthoringHandlersParticleModules.cpp`:
- Around line 76-93: The parsed Acceleration values are never actually applied
to the Niagara module that was created. After the AddModuleAndVerify call
succeeds and the System reference is populated, you must author the parsed
Acceleration vector values (Acceleration.X, Acceleration.Y, Acceleration.Z) to
the actual module node in the Niagara stack. Currently, the code only copies
these values into Context.Result for the response, but the module itself remains
unconfigured. Add the necessary calls to apply these input parameters to the
created module node after the AddModuleAndVerify call and before setting
Context.Result values.
- Around line 82-93: The AddModuleAndVerify function call is not properly
checking whether the module was actually added before proceeding to set result
fields and call SendSuccess. When AddModuleAndVerify returns false (indicating
the module was not successfully added), the handler should treat this as an
error and return early or call an error response instead of continuing to set
the result fields and calling SendSuccess. Add a proper check after the
AddModuleAndVerify call to verify the module was actually added, and if it was
not, handle it as an error case by returning false or calling an appropriate
error response method before reaching the SendSuccess call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5bc8f4d2-3f01-499f-8d1e-e4f0b394b6da
📒 Files selected for processing (4)
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/NiagaraAuthoring/McpAutomationBridge_NiagaraAuthoringHandlersContext.cppplugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/NiagaraAuthoring/McpAutomationBridge_NiagaraAuthoringHandlersInfoValidation.cppplugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/NiagaraAuthoring/McpAutomationBridge_NiagaraAuthoringHandlersParticleModules.cppplugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/NiagaraAuthoring/McpAutomationBridge_NiagaraAuthoringHandlersSystems.cpp
| const TSharedPtr<FJsonObject>* AccelObj; | ||
| FVector Acceleration = FVector(0, 0, -980); | ||
| if (Context.Payload->TryGetObjectField(TEXT("acceleration"), AccelObj)) | ||
| { | ||
| Acceleration = GetVectorFromJson(*AccelObj); | ||
| } | ||
| UNiagaraSystem* System = LoadSystemOrError(Context); | ||
| if (!System) | ||
| UNiagaraSystem* System = nullptr; | ||
| if (!AddModuleAndVerify(Context, TEXT("/Niagara/Modules/Update/Forces/AccelerationForce.AccelerationForce"), ENiagaraScriptUsage::ParticleUpdateScript, TEXT("Acceleration Force"), System)) | ||
| { | ||
| return true; | ||
| } | ||
| const bool bParameterAdded = AddOrSetVectorUserParameter(System, TEXT("MCP_Acceleration"), Acceleration); | ||
| MarkDirtyAndVerify(Context, System); | ||
| Context.Result->SetStringField(TEXT("moduleName"), TEXT("Acceleration")); | ||
| Context.Result->SetBoolField(TEXT("parameterAdded"), bParameterAdded); | ||
| Context.Result->SetStringField(TEXT("parameterName"), TEXT("MCP_Acceleration")); | ||
| Context.Result->SetStringField(TEXT("message"), TEXT("Configured acceleration module.")); | ||
| Context.SendSuccess(true, TEXT("Acceleration module configured.")); | ||
| Context.Result->SetNumberField(TEXT("accelerationX"), Acceleration.X); | ||
| Context.Result->SetNumberField(TEXT("accelerationY"), Acceleration.Y); | ||
| Context.Result->SetNumberField(TEXT("accelerationZ"), Acceleration.Z); | ||
| Context.Result->SetStringField(TEXT("message"), TEXT("Added acceleration force module.")); | ||
| Context.SendSuccess(true, TEXT("Acceleration module added.")); | ||
| return true; |
There was a problem hiding this comment.
Parsed module inputs are never authored to the Niagara stack module.
Line 76/98/121 parse caller inputs, but Lines 88-90/107/129-132 only mirror those values into Context.Result; nothing writes them into the created module node. This makes the action claim configured values that were not actually applied.
Also applies to: 98-110, 115-135
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/NiagaraAuthoring/McpAutomationBridge_NiagaraAuthoringHandlersParticleModules.cpp`
around lines 76 - 93, The parsed Acceleration values are never actually applied
to the Niagara module that was created. After the AddModuleAndVerify call
succeeds and the System reference is populated, you must author the parsed
Acceleration vector values (Acceleration.X, Acceleration.Y, Acceleration.Z) to
the actual module node in the Niagara stack. Currently, the code only copies
these values into Context.Result for the response, but the module itself remains
unconfigured. Add the necessary calls to apply these input parameters to the
created module node after the AddModuleAndVerify call and before setting
Context.Result values.
…color modules, stack-module readback manage_effect Niagara module actions (add_spawn_rate_module, etc.) always failed EMITTER_NOT_FOUND: the dispatch layer defaults emitterName to "DefaultEmitter" when omitted, but the C++ names the emitter handle after the source emitter asset, so the exact-match lookup never hits. add_emitter_to_system also ignored a caller-supplied emitterName, and add_acceleration/size/color_module added User.MCP_* parameters instead of real stack modules; get_niagara_info reported no stack modules. - LoadSystemAndEmitter: single-emitter fallback when the requested name isn't found and the system has exactly one emitter (surfaces emitterResolvedBy/resolvedEmitterName); still errors for 0 or >1. - AddEmitterToSystem: honor caller emitterName via FNiagaraEmitterHandle::SetName. - AddAcceleration/Size/Color: real modules via AddModuleToEmitterStack (AccelerationForce / ScaleSpriteSize / Color) instead of User.MCP_* parameter shims. - AddModuleAndVerify: return CREATE_FAILED + false when the stack insertion yields a null module node, instead of a success response with moduleAdded:false. - get_niagara_info: enumerate per-emitter stack modules (UNiagaraNodeFunctionCall::GetCalledUsage() == Module; GetOrderedModuleNodes is not DLL-exported). Verified live in UE 5.8. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
SoloGorilla
force-pushed
the
fix/niagara-emitter-and-modules
branch
from
7178c3a to
b0cc8e5
Compare
|
Thanks for the review. Addressed the success-on-failure finding: On "parsed inputs are never authored to the module": that is intentional for now. It matches the existing |
|
Status of both review findings:
@coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
fd -t f "McpAutomationBridge_NiagaraAuthoringHandlersParticleModules.cpp" | head
FILE=$(fd -t f "McpAutomationBridge_NiagaraAuthoringHandlersParticleModules.cpp")
ast-grep run --pattern 'AddModuleAndVerify($$$) {
$$$
}' --lang cpp "$FILE" 2>/dev/null | head -100
echo "---"
rg -n "CREATE_FAILED|AddModuleToEmitterStack|moduleAdded" "$FILE" -C2Length of output: 1250 Confirmed — the fix is in place: On the input-authoring scope decision, that's a reasonable call — keeping the accel/size/color handlers consistent with the existing Kicking off a fresh review now to confirm everything looks good end-to-end. ✏️ Learnings added
✅ Action performedReview finished.
|
2 participants
Problem
manage_effectNiagara module actions were unusable end-to-end:add_*_moduleactions failedEMITTER_NOT_FOUND. The dispatch layer defaultsemitterNameto"DefaultEmitter"when the caller omits it, butAddEmitterToSystemnames the emitter handle after the source emitter asset, so the exact-match lookup inLoadSystemAndEmitternever matches.add_emitter_to_systemignored a caller-suppliedemitterNameentirely.add_acceleration_module/add_size_module/add_color_moduleaddedUser.MCP_*parameters instead of real Niagara stack modules — particles never rendered, thoughsuccess:truewas returned.get_niagara_inforeported emitters + user parameters but no stack modules, so a real module was indistinguishable from a parameter shim.Fix (
Domains/NiagaraAuthoring/)LoadSystemAndEmitter— single-emitter fallback: if the requested name isn't found and the system has exactly one emitter, resolve to it and surfaceemitterResolvedBy/requestedEmitterName/resolvedEmitterName. Still errors for 0 or >1 emitters.AddEmitterToSystem— honor a caller-suppliedemitterNameviaFNiagaraEmitterHandle::SetName(also makes the handle name consistent with what module actions resolve against).AddAcceleration/Size/Color— replace theUser.MCP_*shims with realAddModuleToEmitterStackcalls (Update/Forces/AccelerationForce,Update/Size/ScaleSpriteSize,Update/Color/Color), mirroring the existingAddForceModulepattern.get_niagara_info— enumerate per-emitter stack modules by walking the script graph'sUNiagaraNodeFunctionCallnodes filtered onGetCalledUsage()==Module. (FNiagaraStackGraphUtilities::GetOrderedModuleNodesis declared but notNIAGARAEDITOR_API-exported, so it can't be linked from the plugin module.)Validation (UE 5.8, live)
add_emitter_to_system emitterName=FireEmitter→ handle namedFireEmitter;add_spawn_rate_module(no emitterName) →moduleAdded:truevia single-emitter fallback; accel/size/color → real modules;get_niagara_info→moduleCount:12incl.AccelerationForce/ScaleSpriteSize/Color,userParameterCount:0.🤖 Generated with Claude Code