Skip to content

fix: screenshot, Niagara crash, hollow getters, and param aliases#359

Merged
ChiR24 merged 7 commits into
ChiR24:devfrom
spencer-zaid:fix/screenshot-niagara-hollow-handlers
Jun 2, 2026
Merged

fix: screenshot, Niagara crash, hollow getters, and param aliases#359
ChiR24 merged 7 commits into
ChiR24:devfrom
spencer-zaid:fix/screenshot-niagara-hollow-handlers

Conversation

@spencer-zaid

@spencer-zaid spencer-zaid commented Apr 16, 2026

Copy link
Copy Markdown
Contributor

Summary

Six fixes uncovered while exercising the full MCP tool surface against UE 5.7.4 with the McpAutomationBridge plugin + unreal-engine-mcp-server@0.5.21. All of them cause user-visible silent failures or hard editor crashes today; after this PR the plugin loads cleanly and every tool in the set I tested returns meaningful data.

Plugin-side fixes

1. take_screenshot reports success but never writes a file

HandleControlEditorScreenshot calls FScreenshotRequest::RequestScreenshot(), which is driven by the game-thread slate tick. In editor mode (no PIE running) that queue is often never drained, so the handler returns {""message"":""Screenshot request submitted""} but no PNG is ever saved.

Replaced with a synchronous capture:

Viewport->Draw();
FlushRenderingCommands();
Viewport->ReadPixels(Bitmap, FReadSurfaceDataFlags(RCM_UNorm));
FImageUtils::PNGCompressImageArray(...);
FFileHelper::SaveArrayToFile(...);

Also prefer GEditor->GetPIEViewport() when PIE is running, so take_screenshot during a play session captures the game view rather than the editor viewport behind it. Response now includes width, height, and fileSizeBytes.

2. create_niagara_system crashes the editor on UE 5.7

HandleCreateNiagaraSystem manually creates a default UNiagaraEmitter and writes EmitterData->GraphSource = EmitterSource on the struct returned by GetLatestEmitterData(). In UE 5.1+ this leaves the versioned emitter data in a default-version slot that isn't reachable the way the code expects, so NiagaraEmitter.cpp:804 fails its GraphSource != nullptr ensure on the next tick and the editor dies with a container-mutation assertion.

Reliable repro: every call takes the editor down ~130ms after the request.

The fix removes the default-emitter creation entirely (empty systems are valid — callers can use add_emitter_to_system to populate them) and mirrors UNiagaraSystemFactoryNew::InitializeSystem() for the system-level scripts, then calls RequestCompile(false).

3. Hollow getters: get_project_settings / get_editor_settings / get_world_settings

All three handlers returned {""success"": true} with no data. Now pull real values from UGeneralProjectSettings, ULevelEditorViewportSettings, and AWorldSettings (KillZ, gravity, time dilation, default game mode, package name, timeSeconds, hasBegunPlay, isPlayInEditor, …). get_world_settings also tries GEditor->PlayWorld first so PIE queries return the live world.

4. Hollow getters: get_performance_stats / get_memory_stats

Both literally returned ""placeholder - implement with actual metrics"". Now return real stats:

  • Performance: fps, frameTimeMs, gameThreadMs, renderThreadMs, rhiThreadMs, gpuMs, deltaSeconds (from GGameThreadTime/GRenderThreadTime/… via FPlatformTime::ToMilliseconds).
  • Memory: used/peak/available physical + virtual MB plus totals from FPlatformMemory::GetStats() and GetConstants().

5. get_game_framework_info empty outside PIE

In editor mode World->GetAuthGameMode() returns nullptr, so the handler emitted ""gameFrameworkInfo"": {}. Now resolves the game mode in order of:

  1. live PlayWorld->GetAuthGameMode() (if PIE is running)
  2. AWorldSettings::DefaultGameMode (per-level override)
  3. UGameMapsSettings::GetGlobalDefaultGameMode() (project default)

…and reports which source won via a source field. All CDO-level classes (DefaultPawn, PlayerController, GameState, PlayerState, HUD) are read off the resolved class's default object.

6. find_actors_by_class ignores the classPath param

The control_actor schema advertises classPath, but HandleControlActorFindByClass only reads className (with class as a fallback). Calls using the schema-declared name silently hit ""className or class is required"". Plugin now accepts classPath too.

Server-side fixes

7. create_niagara_system silently defaults name to NS_Custom

effect-handlers.ts only looked at argsTyped.name before falling through to the NS_Custom placeholder. The manage_effect tool schema exposes the parameter as systemName, so every call hit the default and the user-supplied name was lost. Same bug for create_niagara_emitter with emitterName. Both handlers now check the schema-declared key first.

8. find_by_class rejects schema-declared classPath

Server-side analogue of #6: actor-handlers.ts normalizeArgs only accepted className/class_name/class, so a client passing the schema's classPath got ""Missing required argument: className"". Added classPath to the alias list.

Test plan

All plugin-side fixes built and were verified live against UE 5.7.4 editor with the automation bridge loaded:

  • take_screenshot (editor mode): writes valid 2343×1190 PNG (~620KB).
  • take_screenshot (PIE): prefers the PIE viewport; writes valid PNG (~835KB).
  • create_niagara_system: asset is created, saves to disk, opens in the Niagara editor without corruption, no crash.
  • get_project_settings / get_editor_settings / get_world_settings: return populated JSON with the expected fields.
  • get_performance_stats / get_memory_stats: return real metric numbers.
  • get_game_framework_info: returns GameMode / PlayerController / GameState / PlayerState / HUD / DefaultPawn plus source for a TestMap whose AWorldSettings::DefaultGameMode is set.
  • find_actors_by_class with classPath (on the plugin side — still blocked by server-side chore(deps): bump body-parser from 2.2.0 to 2.2.1 in the npm_and_yarn group across 1 directory #8 without this PR).
  • Editor plugin build: clean, no warnings introduced.

The TypeScript server fixes are purely additive (new fallback branches in two handler functions). They were not rebuilt locally because the repo's lockfile currently refuses npm install (@typescript-eslint/scope-manager@8.58.1 is pinned but not published on the registry I can reach) — happy to fix that in a follow-up if you'd prefer. Worth noting this same lockfile problem blocks any local npm ci today.

🤖 Generated with Claude Code


Open with Devin

Six fixes uncovered while exercising the full MCP tool surface against
UE 5.7. All of them cause user-visible silent failures or editor crashes.

## Plugin-side fixes

### 1. Screenshot reports success but never writes a file

`HandleControlEditorScreenshot` calls `FScreenshotRequest::RequestScreenshot()`,
which is driven by the game-thread slate tick. In editor mode (no PIE running)
that queue is often never drained, so the handler returns
`{"message":"Screenshot request submitted"}` but no PNG is ever saved.

Replaced with a synchronous capture:

  Viewport->Draw()
  FlushRenderingCommands()
  Viewport->ReadPixels(Bitmap, FReadSurfaceDataFlags(RCM_UNorm))
  FImageUtils::PNGCompressImageArray(...)
  FFileHelper::SaveArrayToFile(...)

Also prefer the PIE viewport when PIE is running, so `take_screenshot`
during a play session captures the game view rather than the editor
viewport behind it. Response now includes `width`, `height`, and
`fileSizeBytes` so callers can verify the write landed.

### 2. create_niagara_system crashes the editor on UE 5.7

`HandleCreateNiagaraSystem` manually creates a default `UNiagaraEmitter`
and writes `EmitterData->GraphSource = EmitterSource` on the struct
returned by `GetLatestEmitterData()`. In UE 5.1+ this leaves the
versioned emitter data in a default-version slot that isn't reachable
the way the code expects, so `NiagaraEmitter.cpp:804` fails its
`GraphSource != nullptr` ensure on the next tick and the editor dies
with a container-mutation assertion.

The crash is reliable: any call to `create_niagara_system` on UE 5.7
takes the editor down roughly 130ms after the request.

The fix removes the default-emitter creation entirely (empty systems
are valid - callers can use `add_emitter_to_system` to populate them)
and mirrors `UNiagaraSystemFactoryNew::InitializeSystem()` for the
system-level scripts, then calls `RequestCompile(false)`. No more
GraphSource ensure, no more crash.

### 3. Hollow getters: get_project_settings / get_editor_settings / get_world_settings

All three handlers returned `{"success": true}` with no actual data. They
now pull real values from the corresponding UE subsystems:

- get_project_settings: `UGeneralProjectSettings` (project name, version,
  copyright, company, project GUID), `FApp::GetProjectName()`,
  `FEngineVersion::Current()`, build config, project dir.
- get_editor_settings: `ULevelEditorViewportSettings` (mouse sensitivity,
  scroll speed, distance-scaled camera), plus PIE state and editor flags.
- get_world_settings: the full `AWorldSettings` surface - KillZ, gravity,
  time dilation, world-bounds checks, default game mode, package name,
  timeSeconds/realTimeSeconds/deltaSeconds, hasBegunPlay, isPlayInEditor.
  Also looks through `GEditor->PlayWorld` first so PIE queries return the
  live world rather than the edit world.

### 4. Hollow getters: get_performance_stats / get_memory_stats

Both returned a literal `"placeholder - implement with actual metrics"`
string. They now return real stats:

- get_performance_stats: fps, frameTimeMs, gameThreadMs, renderThreadMs,
  rhiThreadMs, gpuMs, deltaSeconds (from GGameThreadTime / GRenderThreadTime
  / etc. via FPlatformTime::ToMilliseconds).
- get_memory_stats: used/peak/available physical and virtual MB and totals
  from FPlatformMemory::GetStats() and GetConstants().

### 5. get_game_framework_info empty outside PIE

In editor mode `World->GetAuthGameMode()` returns nullptr, so the handler
fell through and emitted `"gameFrameworkInfo": {}`. Now it resolves the
game mode in order of:

  1. live `PlayWorld->GetAuthGameMode()` (if PIE is running)
  2. `AWorldSettings::DefaultGameMode` (per-level override)
  3. `UGameMapsSettings::GetGlobalDefaultGameMode()` (project default)

...and reports which source won via a `source` field. All the CDO-level
classes (DefaultPawn, PlayerController, GameState, PlayerState, HUD) are
read off the resolved class's default object.

### 6. find_actors_by_class ignores the classPath param

The `control_actor` tool schema advertises `classPath` for
`find_actors_by_class`, but `HandleControlActorFindByClass` only reads
`className` (with `class` as a fallback). Calls using the schema-declared
name silently hit "className or class is required". Plugin now accepts
classPath too, so the schema and handler agree.

## Server-side fixes

### 7. create_niagara_system silently defaults name to NS_Custom

`effect-handlers.ts` only looked at `argsTyped.name` before falling
through to the `NS_Custom` placeholder. The manage_effect tool schema
exposes the parameter as `systemName`, so every call hit the default
and the user-supplied name was lost. Same bug for
`create_niagara_emitter` with `emitterName`. Both handlers now check
the schema-declared key first, preserving caller intent.

### 8. find_by_class rejects schema-declared classPath

`actor-handlers.ts` had the server-side analogue of bug ChiR24#6: the
normalizeArgs call only accepted `className`/`class_name`/`class`, so a
client passing the schema's `classPath` got "Missing required
argument: className". Added `classPath` to the alias list.

## Testing

All six plugin-side fixes built and were verified live against UE 5.7.4
editor with the MCP automation bridge loaded:

- take_screenshot (editor mode and PIE): produces a valid PNG on disk
  with the expected dimensions (2343x1190 for my setup, 620-835KB file).
- create_niagara_system: asset is created, saves to disk, opens in the
  Niagara editor without corruption, no crash.
- get_project_settings / get_editor_settings / get_world_settings /
  get_performance_stats / get_memory_stats / get_game_framework_info:
  all return populated JSON with the expected fields.
- find_actors_by_class with classPath: matches the plugin correctly.

The TypeScript server fixes are purely additive (new fallback branches
in two handler functions) and were not rebuilt locally - the repo's
lockfile refuses `npm install` against a missing published version.
@coderabbitai

coderabbitai Bot commented Apr 16, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: a89a33b2-0a3f-48b7-8cc7-34729aaffc89

📥 Commits

Reviewing files that changed from the base of the PR and between 78941f6 and 36b47cf.

📒 Files selected for processing (1)
  • plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_EnvironmentHandlers.cpp

📝 Walkthrough

Summary by CodeRabbit

  • New Features

    • Viewport screenshot now captured synchronously and returns width, height, sizeBytes and fileSizeBytes immediately.
  • Improvements

    • Expanded inspection reports: project, editor, world, performance (per-thread timings, GPU) and memory (values in MB).
    • Better game-mode/world resolution in editor/PIE and added world/play metadata.
    • Streamlined Niagara system creation and updated name-selection precedence for Niagara assets.
    • Added additional parameter aliases (e.g., classPath) and more robust material function input handling.

Walkthrough

This PR expands environment inspection payloads, makes editor viewport screenshots synchronous, improves game-mode resolution fallbacks, refactors Niagara system initialization, normalizes handler parameter/name aliases, and centralizes material function expression insertion.

Changes

Automation Bridge and Handler Improvements

Layer / File(s) Summary
Environment inspection handlers
plugins/McpAutomationBridge/.../McpAutomationBridge_EnvironmentHandlers.cpp
Inspector action handlers return expanded payloads: get_project_settings includes project identity and selected UGeneralProjectSettings fields; get_editor_settings exposes viewport/input tuning and runtime flags; get_world_settings prefers PlayWorld, safely handles missing level/world settings, and returns extended world/time/play and AWorldSettings values including defaultGameMode path; get_performance_stats computes per-thread timing in milliseconds and GPU frame time; get_memory_stats reports totals/available/used/peak in MB.
Synchronous screenshot capture pipeline
plugins/McpAutomationBridge/.../McpAutomationBridge_ControlHandlers.cpp
Screenshot handler replaces async FScreenshotRequest with synchronous viewport capture: prefers PIE viewport when active, validates viewport size, draws and flushes rendering, reads pixels, forces alpha to 255, PNG-encodes, and saves file synchronously. Response includes width, height, sizeBytes, and fileSizeBytes; explicit errors added for missing/zero-sized viewport, pixel-read/encode/save failures, and image-too-large for base64.
Game framework info resolution
plugins/McpAutomationBridge/.../McpAutomationBridge_GameFrameworkHandlers.cpp
get_game_framework_info reworked to resolve active AGameModeBase preferring GEditor->PlayWorld (fallback to editor world) and resolving game mode class by: live World->GetAuthGameMode(), AWorldSettings::DefaultGameMode, then project default via FSoftClassPath; response sets source, worldName, and isPlayInEditor, and extracts class defaults from the resolved class CDO.
Niagara system creation refactoring
plugins/McpAutomationBridge/.../McpAutomationBridge_NiagaraHandlers.cpp
HandleCreateNiagaraSystem removes default emitter creation and emitter handle registration; initializes system-level script source/graph for spawn script, conditionally attaches to update script, changes factory init arg true->false, and calls RequestCompile(false) after setup.
Handler parameter aliases and name resolution
src/tools/handlers/actor-handlers.ts, src/tools/handlers/effect-handlers.ts
Actor handler find_by_class adds classPath as an accepted alias for the required class parameter. Niagara handlers now prefer systemName/emitterName over legacy name, fall back to path-derived or mutableArgs-provided names, then to defaults (NS_Custom, DefaultEmitter).
Material function expression container
plugins/McpAutomationBridge/.../McpAutomationBridge_MaterialAuthoringHandlers.cpp
add_function_input/add_function_output now insert created material function expressions via AddExpressionToContainer(nullptr, Func, NewExpr) instead of direct, version-specific expression collection manipulation.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • ChiR24/Unreal_mcp#53: Both PRs touch the Game Framework handler layer: the main PR changes get_game_framework_info logic in McpAutomationBridge_GameFrameworkHandlers.cpp.
  • ChiR24/Unreal_mcp#183: Modifies Niagara system creation flow and default-emitter handling, related to the Niagara refactor here.

Suggested labels

javascript, size/m

Poem

🐰 A rabbit hops through code tonight,
Viewports captured in a single light,
Worlds and modes now found with care,
Niagara trimmed and names made fair,
Inspector data gleams so bright.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 70.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main changes: fixing screenshot functionality, Niagara crashes, hollow getters, and parameter aliases. It is specific enough to convey the primary fixes without being overly detailed.
Description check ✅ Passed The description is comprehensive and well-structured, covering all required template sections including Summary, Changes (8 numbered fixes), Related Issues context, Type of Change (Bug fix), Testing (with detailed test plan checkmarks), and Pre-Merge Checklist completion.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 Infer (1.2.0)
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_EnvironmentHandlers.cpp

In file included from plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_EnvironmentHandlers.cpp:62:
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpVersionCompatibility.h:19:10: fatal error: 'Runtime/Launch/Resources/Version.h' file not found
19 | #include "Runtime/Launch/Resources/Version.h"
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
Error: the following clang command did not run successfully:
/opt/infer-linux-x86_64-v1.2.0/lib/infer/facebook-clang-plugins/clang/install/bin/clang-18
@/tmp/coderabbit-infer/36b47cf0f265189cd36bbf264562040d9ff53757-e98c6e94c03020b3/tmp/clang_command_.tmp.0e44c4.txt
++Contents of '/tmp/coderabbit-infer/36b47cf0f265189cd36bbf264562040d9ff53757-e98c6e94c03020b3/tmp/clang_command_.tmp.0e44c4.txt':
"-cc1" "-load"
"/opt/infer-linux-x86_64-v1.2.0/lib/infer/infer/bin/../../facebook-clang-plugins/libtooling/build/FacebookClangPlugin.dylib"
"-add-plugin" "Bini

... [truncated 1439 characters] ...

al-isystem"
"/usr/lib/gcc/x86_64-linux-gnu/12/../../../../x86_64-linux-gnu/include"
"-internal-externc-isystem" "/usr/include/x86_64-linux-gnu"
"-internal-externc-isystem" "/include" "-internal-externc-isystem"
"/usr/include" "-Wno-ignored-optimization-argument" "-Wno-everything"
"-fdeprecated-macro" "-ferror-limit" "19" "-fgnuc-version=4.2.1"
"-fskip-odr-check-in-gmf" "-fcxx-exceptions" "-fexceptions"
"-D__GCC_HAVE_DWARF2_CFI_ASM=1" "-o"
"/tmp/coderabbit-infer/e98c6e94c03020b3/file.o" "-x" "c++"
"plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_EnvironmentHandlers.cpp"
"-O0" "-fno-builtin" "-include"
"/opt/infer-linux-x86_64-v1.2.0/lib/infer/infer/bin/../lib/clang_wrappers/global_defines.h"
"-Wno-everything"


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

Copy link
Copy Markdown
Contributor

👋 Thanks for your first Pull Request! We love contributions. Please ensure you have signed off your commits and followed the contribution guidelines.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 potential issue.

View 5 additional findings in Devin Review.

Open in Devin Review

LowerSubAction.Equals(TEXT("find_by_tag")) ||
LowerSubAction.Equals(TEXT("inspect_class")) ||
LowerSubAction.Equals(TEXT("inspect_cdo"));
LowerSubAction.Equals(TEXT("inspect_class"));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 inspect_cdo sub-action removed from dispatch, making the feature unreachable

The PR removes inspect_cdo from the bIsGlobalAction classification list and deletes its dispatch code (return HandleInspectCdoAction(...)) in HandleInspectAction. However, the C++ implementation HandleInspectCdoAction still exists (McpAutomationBridge_PropertyHandlers.cpp:2847), the TS handler still sends action: 'inspect_cdo' (src/tools/handlers/inspect-handlers.ts:689-696), and the tool schema still advertises it (consolidated-tool-definitions.ts:885). Since inspect_cdo is no longer classified as a global action, the code falls through to the "Require objectPath for non-global actions" check at line 1472, which fails because inspect_cdo callers provide blueprintPath, not objectPath. This completely breaks the inspect_cdo feature — callers get an objectPath required error instead of CDO inspection results.

Prompt for agents
The inspect_cdo sub-action was removed from the bIsGlobalAction list (around line 1443 in McpAutomationBridge_EnvironmentHandlers.cpp) and its dispatch code (the else-if block that called HandleInspectCdoAction) was also deleted. This breaks the feature because inspect_cdo is still advertised in the tool schema (consolidated-tool-definitions.ts:885), still handled in TypeScript (inspect-handlers.ts:689-696), and still implemented in C++ (McpAutomationBridge_PropertyHandlers.cpp:2847). To fix: 1) Re-add inspect_cdo to the bIsGlobalAction condition around line 1443 (add || LowerSubAction.Equals(TEXT("inspect_cdo"))). 2) Re-add the dispatch block inside the global actions handler section, something like: else if (LowerSubAction.Equals(TEXT("inspect_cdo"))) { return HandleInspectCdoAction(RequestId, Payload, RequestingSocket); } — this should go right after the inspect_class handler block, before the fallback placeholder response.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

…tom pins via MCP

Upstream's material authoring tools only work on UMaterial — any graph-editing
action fails with ASSET_NOT_FOUND when the asset is a UMaterialFunction, even
though the equivalent expression classes (Add, Multiply, Custom, etc.) work on
both. Separately, UMaterialExpressionCustom's named FCustomInput pins were only
ever the default "A" — add_custom_expression had no way to declare more, so
any multi-input Custom node was unreachable from connect_nodes.

**Plugin (C++):**
- New LOAD_MATERIAL_OR_FUNCTION_OR_RETURN macro alongside the existing
  UMaterial-only one. Produces (Material, MaterialFunction) — exactly one
  non-null. Four small helpers (GetMatOrFuncExpressions, GetMatOrFuncContainer,
  NotifyMatOrFuncDirty, FindExpressionInMatOrFunc) branch on whichever.
- Retrofitted add_custom_expression, add_math_node, connect_nodes to use the
  new macro. connect_nodes' "Main" target now errors cleanly on functions —
  callers wire to the FunctionOutput node by ID/name instead.
- add_custom_expression accepts an optional `inputs: [{"name":"TempK"}, ...]`
  array and populates the Custom node's FCustomInput[] on creation.
- New add_custom_input_pin subaction appends a named input to an existing
  Custom node (idempotent; also drops the default unused "A" pin when it's
  the only entry).
- FindExpressionInMatOrFunc additionally resolves FunctionInput/FunctionOutput
  nodes by their InputName/OutputName so name-based connect works both ways.

**Server (TypeScript):**
- add_custom_expression forwards the optional `inputs[]` array to the plugin.
- New add_custom_input_pin case routes to the plugin subaction.
- Schema: added `inputs` param + `add_custom_input_pin` to the action enum.

**Test coverage unchanged:** 210/210 unit tests pass. Exercised end-to-end
authoring a four-input Custom node inside a material function (Phase H.3+
reentry hull heating material in Starbound) — all five connections verified.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added size/xl and removed size/l labels Apr 17, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📏 Large PR Detected

This pull request is quite large (1000+ lines changed), which can make reviewing challenging.

Suggestions:

  • Consider breaking this into smaller, focused PRs
  • Separate refactoring from new features
  • Split bug fixes from feature additions

This helps reviewers provide better feedback and speeds up the merge process. Thank you! 🙏

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 new potential issue.

View 10 additional findings in Devin Review.

Open in Devin Review

Comment on lines +3390 to +3398
#if WITH_EDITORONLY_DATA
if (Material)
{
return Material->GetEditorOnlyData()->ExpressionCollection.Expressions;
}
if (MaterialFunction)
{
return MaterialFunction->GetEditorOnlyData()->ExpressionCollection.Expressions;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 GetMatOrFuncExpressions bypasses UE 5.0 version compatibility, calling GetEditorOnlyData() which doesn't exist in UE 5.0

The new GetMatOrFuncExpressions helper directly calls Material->GetEditorOnlyData()->ExpressionCollection.Expressions and MaterialFunction->GetEditorOnlyData()->ExpressionCollection.Expressions. However, GetEditorOnlyData() only exists in UE 5.1+. The existing codebase uses the MCP_GET_MATERIAL_EXPRESSIONS macro (defined in McpVersionCompatibility.h:42-54) which falls back to Material->Expressions on UE 5.0, and the add_function_input/add_function_output handler at McpAutomationBridge_MaterialAuthoringHandlers.cpp:2001-2008 correctly uses Func->FunctionExpressions on UE 5.0. The project explicitly supports UE 5.0-5.7 (per AGENTS.md). This will cause a compile error on UE 5.0 editor builds where WITH_EDITORONLY_DATA is true but GetEditorOnlyData() doesn't exist.

Suggested change
#if WITH_EDITORONLY_DATA
if (Material)
{
return Material->GetEditorOnlyData()->ExpressionCollection.Expressions;
}
if (MaterialFunction)
{
return MaterialFunction->GetEditorOnlyData()->ExpressionCollection.Expressions;
}
#if WITH_EDITORONLY_DATA
#if ENGINE_MAJOR_VERSION == 5 && ENGINE_MINOR_VERSION >= 1
if (Material)
{
return Material->GetEditorOnlyData()->ExpressionCollection.Expressions;
}
if (MaterialFunction)
{
return MaterialFunction->GetEditorOnlyData()->ExpressionCollection.Expressions;
}
#else
if (Material)
{
return Material->Expressions;
}
if (MaterialFunction)
{
return MaterialFunction->FunctionExpressions;
}
#endif
#endif
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

…uts by name

UMaterialExpressionMaterialFunctionCall stores its inputs in a FunctionInputs
array of FFunctionExpressionInput, not as individual UProperty fields — same
pattern as UMaterialExpressionCustom's Inputs[]. Upstream's connect_nodes used
FindPropertyByName and a PropertyLink walk, so every attempt to wire into a
function-call node's named input returned PIN_NOT_FOUND.

Added a branch that casts TargetExpr to UMaterialExpressionMaterialFunctionCall,
looks up the matching entry by FunctionInputs[i].ExpressionInput->InputName,
and writes the source expression into the entry's per-call FExpressionInput.

Exercised end-to-end authoring a master material (M_SpacecraftHull_Master)
that calls a function (MF_HullHeating) with four named inputs — all four
input connections and the subsequent wire into the main material's
EmissiveColor pin complete cleanly via MCP.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@coderabbitai coderabbitai Bot added the bug Something isn't working label Apr 18, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_MaterialAuthoringHandlers.cpp (1)

1588-1734: ⚠️ Potential issue | 🟠 Major

Set OutputIndex based on sourcePin in all input wiring paths.

Currently, sourcePin is read but not utilized to determine output selection. Custom inputs set OutputIndex = 0 unconditionally when sourcePin is non-empty (line 1683), function-call inputs never set OutputIndex (line 1710), and standard inputs never set OutputIndex (line 1733). Per the type definition, sourcePin should be parsed as either a numeric string (converted via FCString::Atoi to select by index) or matched against output names. Multi-output sources will silently connect the wrong output.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_MaterialAuthoringHandlers.cpp`
around lines 1588 - 1734, The code reads SourcePin but never applies it to
choose which output to wire; update all wiring paths to parse SourcePin and set
the corresponding OutputIndex on the destination FExpressionInput: 1) parse
SourcePin into an int via FCString::Atoi(SourcePin) when non-empty (and fallback
to 0), or attempt to match against the source output names if you want
name-based selection; 2) in the UMaterialExpressionCustom path set
CustomExpr->Inputs[CustomIdx].Input.OutputIndex to the parsed index (instead of
unconditionally 0); 3) in the UMaterialExpressionMaterialFunctionCall path set
Entry.Input.OutputIndex to the parsed index when wiring Entry.Input.Expression =
SourceExpr; 4) in the standard FExpressionInput path set InputPtr->OutputIndex
to the parsed index after assigning InputPtr->Expression = SourceExpr; keep
existing calls to NotifyMatOrFuncDirty and SendAutomationResponse and use the
same SearchName/SourceExpr/SourcePin symbols so the changes integrate with
FindExpressionInMatOrFunc and the other handlers.
🧹 Nitpick comments (5)
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_CharacterHandlers.cpp (1)

786-811: LGTM — fixes prior coupling of extra movement params to walkSpeed/runSpeed branches.

Hoisting crouchSpeed, swimSpeed, flySpeed, acceleration, deceleration, and groundFriction out of the walk/run branches is the right call — callers can now supply any subset without needing to also pass walkSpeed/runSpeed. The walk-vs-run precedence and the warning log are preserved.

Minor nit: lines 786-811 use tab indentation while the rest of the function uses 4-space indentation. Consider normalizing to match surrounding style.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_CharacterHandlers.cpp`
around lines 786 - 811, The code in McpAutomationBridge_CharacterHandlers.cpp
around the configure movement block uses tab indentation while the rest of the
function uses 4-space indentation; please normalize indentation to 4 spaces for
the entire block that references Payload, Movement, GetNumberFieldChar and the
properties MaxWalkSpeed, MaxWalkSpeedCrouched, MaxSwimSpeed, MaxFlySpeed,
MaxAcceleration, BrakingDecelerationWalking, and GroundFriction so the style
matches surrounding code (no code changes, only replace tabs with 4-space
indents).
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_CombatHandlers.cpp (1)

49-51: Forward declarations appear redundant.

The forward declarations for UProjectileMovementComponent, USphereComponent, and UBoxComponent are placed before the subsystem/helpers includes, but all three classes are fully included later within the #if WITH_EDITOR section (lines 70, 73, 75). Since all combat handler implementation code runs exclusively within the WITH_EDITOR guard (starting line 290), these forward declarations are not actually used.

If McpAutomationBridgeSubsystem.h or McpAutomationBridgeHelpers.h require these types, those headers should declare their own dependencies rather than relying on forward declarations in consuming .cpp files.

♻️ Consider removing redundant forward declarations
-
-class UProjectileMovementComponent;
-class USphereComponent;
-class UBoxComponent;
-
 `#include` "McpAutomationBridgeSubsystem.h"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_CombatHandlers.cpp`
around lines 49 - 51, The forward declarations for UProjectileMovementComponent,
USphereComponent, and UBoxComponent in this translation unit are redundant
because the full includes live inside the WITH_EDITOR guarded section and all
combat handler code is editor-only; remove those three forward declarations from
McpAutomationBridge_CombatHandlers.cpp and instead ensure any header that truly
needs those types (e.g., McpAutomationBridgeSubsystem.h or
McpAutomationBridgeHelpers.h) adds its own forward declarations or includes;
keep the WITH_EDITOR includes intact and verify no remaining references outside
the editor guard depend on those forward declarations.
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_NiagaraHandlers.cpp (1)

206-221: Duplicate NiagaraSystem null check.

Lines 216-221 re-check !NiagaraSystem after it was already validated at 206-211 and nothing between them can reset the pointer (FAssetRegistryModule::AssetCreated and McpSafeAssetSave don't replace it). The second block is unreachable — drop it.

♻️ Remove duplicate check
   FAssetRegistryModule::AssetCreated(NiagaraSystem);
   McpSafeAssetSave(NiagaraSystem);
-
-  if (!NiagaraSystem) {
-    SendAutomationError(RequestingSocket, RequestId,
-                        TEXT("Failed to create Niagara system asset"),
-                        TEXT("ASSET_CREATION_FAILED"));
-    return true;
-  }

The same redundant pattern exists in HandleCreateNiagaraEmitter at lines 372-377 — worth cleaning up together.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_NiagaraHandlers.cpp`
around lines 206 - 221, Remove the duplicate null-check blocks that re-check
NiagaraSystem and NiagaraEmitter after asset registration and saving;
specifically, delete the second if (!NiagaraSystem) {
SendAutomationError(RequestingSocket, RequestId, TEXT("Failed to create Niagara
system asset"), TEXT("ASSET_CREATION_FAILED")); return true; } in the Niagara
system handler (the block after FAssetRegistryModule::AssetCreated and
McpSafeAssetSave) and the analogous duplicate if (!NiagaraEmitter) error block
in HandleCreateNiagaraEmitter; keep the initial null checks, retain calls to
FAssetRegistryModule::AssetCreated and McpSafeAssetSave, and leave the original
SendAutomationError/error-return logic only where the first null check occurs.
src/tools/handlers/effect-handlers.ts (1)

98-111: Alias precedence for create_niagara_emitter leaves a redundant fallback.

By the time this branch runs, the normalization block at lines 52-55 has already copied args.emitter into mutableArgs.emitterName when only the legacy alias was supplied. That means argsTyped.emitterName (line 101) and mutableArgs.emitterName (line 103) reference the same slot for schema-supplied values, so the middle argsTyped.name fallback is the only distinct source and line 103 is effectively dead (it only differs if upstream code mutates mutableArgs.emitterName between lines 55 and 101, which it does not).

Not a bug — resolution still produces the caller's name — but you can simplify to:

♻️ Simplified precedence
-    const resolvedName = (argsTyped.emitterName as string | undefined)
-      || (argsTyped.name as string | undefined)
-      || (mutableArgs.emitterName as string | undefined)
-      || 'DefaultEmitter';
+    const resolvedName = (mutableArgs.emitterName as string | undefined)
+      || (argsTyped.name as string | undefined)
+      || 'DefaultEmitter';
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/tools/handlers/effect-handlers.ts` around lines 98 - 111, The alias
precedence in the create_niagara_emitter branch redundantly falls back to
mutableArgs.emitterName even though the earlier normalization already copied any
legacy alias into mutableArgs.emitterName, making that fallback dead; update the
resolvedName expression to only consider argsTyped.emitterName, argsTyped.name,
then the literal 'DefaultEmitter' (i.e., remove the mutableArgs.emitterName
fallback), leaving resolvedSavePath logic unchanged and ensuring
executeAutomationRequest is called with name: resolvedName and savePath:
resolvedSavePath.
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_MaterialAuthoringHandlers.cpp (1)

1441-1458: Prefer struct-based parsing for the new inputs array.

This new block manually walks JSON objects and fields; please move the request shape into a small struct and deserialize with FJsonObjectConverter to match the handler guidelines. As per coding guidelines, Use FJsonObjectConverter for JSON parsing and struct serialization instead of manual JSON parsing.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_MaterialAuthoringHandlers.cpp`
around lines 1441 - 1458, The code manually parses the "inputs" JSON array;
replace this with struct-based deserialization using FJsonObjectConverter:
define a small struct (e.g., FInputSpec with FString Name and optional Type) and
use FJsonObjectConverter::JsonObjectStringToUStruct /
FJsonObjectConverter::JsonObjectToUStruct(or ConvertJsonToUStruct on each
object) to parse each element from Payload's "inputs" array, then populate
TArray<FCustomInput> CustomInputs from those structs (mapping Name -> FName for
FCustomInput.InputName and handling missing/invalid entries as before). Update
the parsing block that references Payload, InputsArr, FCustomInput, and NewInput
to use the new struct and converter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_ControlHandlers.cpp`:
- Around line 3087-3129: The synchronous capture path using Viewport->Draw(),
FlushRenderingCommands(), Viewport->ReadPixels(...) and
FImageUtils::PNGCompressImageArray can stall the editor and produce corrupt
output; add a pixel-count sanity check (verify Bitmap.Num() == ViewportSize.X *
ViewportSize.Y and return
SEND_STANDARD_ERROR_RESPONSE("CAPTURE_FAILED"/"BUFFER_MISMATCH") if it differs),
log and/or cap very large captures by checking ViewportSize.X * ViewportSize.Y
against a threshold (e.g., 8.3e6) and downscale or reject with a clear error,
and replace the deprecated FImageUtils::PNGCompressImageArray call with the
version-aware helper used elsewhere (ThumbnailCompressImageArray for 5.1+ or
CompressImageArray for 5.0) to match the codebase pattern.

In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_GameFrameworkHandlers.cpp`:
- Around line 1863-1889: The project-default fallback
(UGameMapsSettings::GetGlobalDefaultGameMode and its TryLoadClass logic) is
currently inside the "if (World)" guard so it won't run when World is null; move
the entire block that computes DefaultGameModeStr, constructs FSoftClassPath,
TryLoadClass<AGameModeBase>(), assigns ResolvedGameModeClass and sets
InfoObj->SetStringField(TEXT("source"), TEXT("projectDefault")) to run
outside/after the World block, but only perform it when ResolvedGameModeClass is
still null so levelDefault (AWorldSettings::DefaultGameMode) wins if present.

In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_MaterialAuthoringHandlers.cpp`:
- Around line 1553-1563: The current logic removes the sole default "A" input
whenever a new input is added, causing a user-created "A" to be lost when
subsequently adding "B"; to fix, change the flow in the handler that adds inputs
(e.g., add_custom_input_pin) so that if NewName == FName(TEXT("A")) and
CustomExpr->Inputs has exactly one entry whose InputName == FName(TEXT("A")) and
Input.Expression == nullptr you do NOT add a duplicate but instead reuse/leave
the existing default A; otherwise (NewName != "A") you may remove the unused
default A only when it's truly the sole unused default (keep the existing
conditional but add the NewName != FName(TEXT("A")) guard) before creating and
CustomExpr->Inputs.Add(NewInput).
- Around line 3412-3426: GetMatOrFuncExpressions currently returns
MaterialFunction->GetEditorOnlyData()->ExpressionCollection.Expressions
unconditionally which does not exist in UE 5.0; update the function to
version-guard the MaterialFunction branch so that for ENGINE_MAJOR_VERSION == 5
&& ENGINE_MINOR_VERSION >= 1 you return
MaterialFunction->GetEditorOnlyData()->ExpressionCollection.Expressions,
otherwise return MaterialFunction->FunctionExpressions (matching the pattern
used earlier for 5.0 compatibility), while keeping the Material branch and the
Empty fallback unchanged.

In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_NiagaraHandlers.cpp`:
- Around line 176-204: The current implementation reuses one
UNiagaraScriptSource (SystemScriptSource) for both SystemSpawnScript and
SystemUpdateScript which can cause graph mutations to cross-affect both scripts;
instead create two distinct UNiagaraScriptSource instances (e.g.,
SpawnScriptSource and UpdateScriptSource), each with its own UNiagaraGraph
NodeGraph, using the respective script (SystemSpawnScript and
SystemUpdateScript) as the outer, and then call SetLatestSource on
SystemSpawnScript with SpawnScriptSource and on SystemUpdateScript with
UpdateScriptSource; keep the subsequent NiagaraSystem->RequestCompile(false)
call unchanged.

In `@src/tools/consolidated-tool-definitions.ts`:
- Around line 1478-1482: The inputs array schema currently allows objects
without a name; update the items schema for the inputs property in
consolidated-tool-definitions.ts so each item requires a name (add required:
['name'] on the items object) and optionally set additionalProperties: false if
you want to reject unexpected fields; target the inputs property definition and
its items object to enforce that every custom input entry includes a string
name.
- Line 1445: The manage_material_authoring tool's outputSchema does not include
the fields returned by the new action add_custom_input_pin (inputName,
inputCount, alreadyExisted); update the manage_material_authoring.outputSchema
in src/tools/consolidated-tool-definitions.ts to add these properties with
appropriate types (string for inputName, integer/number for inputCount, boolean
for alreadyExisted) and mark required/optional as needed, then ensure the
updated schema is registered/used during startup so responses from
add_custom_input_pin are validated against the schema.

In `@src/tools/handlers/material-authoring-handlers.ts`:
- Around line 469-472: The handler currently calls normalizeArgs to build params
with assetPath aliased to materialPath only, which rejects callers passing
functionPath for material-function graphs; update the normalizeArgs call so the
assetPath entry includes 'functionPath' in its aliases (e.g., { key:
'assetPath', aliases: ['materialPath','functionPath'], required: true }) so
function-target callers are accepted; keep the rest of the keys (nodeId,
inputName) unchanged and ensure any downstream uses of params.assetPath continue
to work with the new alias.

---

Outside diff comments:
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_MaterialAuthoringHandlers.cpp`:
- Around line 1588-1734: The code reads SourcePin but never applies it to choose
which output to wire; update all wiring paths to parse SourcePin and set the
corresponding OutputIndex on the destination FExpressionInput: 1) parse
SourcePin into an int via FCString::Atoi(SourcePin) when non-empty (and fallback
to 0), or attempt to match against the source output names if you want
name-based selection; 2) in the UMaterialExpressionCustom path set
CustomExpr->Inputs[CustomIdx].Input.OutputIndex to the parsed index (instead of
unconditionally 0); 3) in the UMaterialExpressionMaterialFunctionCall path set
Entry.Input.OutputIndex to the parsed index when wiring Entry.Input.Expression =
SourceExpr; 4) in the standard FExpressionInput path set InputPtr->OutputIndex
to the parsed index after assigning InputPtr->Expression = SourceExpr; keep
existing calls to NotifyMatOrFuncDirty and SendAutomationResponse and use the
same SearchName/SourceExpr/SourcePin symbols so the changes integrate with
FindExpressionInMatOrFunc and the other handlers.

---

Nitpick comments:
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_CharacterHandlers.cpp`:
- Around line 786-811: The code in McpAutomationBridge_CharacterHandlers.cpp
around the configure movement block uses tab indentation while the rest of the
function uses 4-space indentation; please normalize indentation to 4 spaces for
the entire block that references Payload, Movement, GetNumberFieldChar and the
properties MaxWalkSpeed, MaxWalkSpeedCrouched, MaxSwimSpeed, MaxFlySpeed,
MaxAcceleration, BrakingDecelerationWalking, and GroundFriction so the style
matches surrounding code (no code changes, only replace tabs with 4-space
indents).

In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_CombatHandlers.cpp`:
- Around line 49-51: The forward declarations for UProjectileMovementComponent,
USphereComponent, and UBoxComponent in this translation unit are redundant
because the full includes live inside the WITH_EDITOR guarded section and all
combat handler code is editor-only; remove those three forward declarations from
McpAutomationBridge_CombatHandlers.cpp and instead ensure any header that truly
needs those types (e.g., McpAutomationBridgeSubsystem.h or
McpAutomationBridgeHelpers.h) adds its own forward declarations or includes;
keep the WITH_EDITOR includes intact and verify no remaining references outside
the editor guard depend on those forward declarations.

In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_MaterialAuthoringHandlers.cpp`:
- Around line 1441-1458: The code manually parses the "inputs" JSON array;
replace this with struct-based deserialization using FJsonObjectConverter:
define a small struct (e.g., FInputSpec with FString Name and optional Type) and
use FJsonObjectConverter::JsonObjectStringToUStruct /
FJsonObjectConverter::JsonObjectToUStruct(or ConvertJsonToUStruct on each
object) to parse each element from Payload's "inputs" array, then populate
TArray<FCustomInput> CustomInputs from those structs (mapping Name -> FName for
FCustomInput.InputName and handling missing/invalid entries as before). Update
the parsing block that references Payload, InputsArr, FCustomInput, and NewInput
to use the new struct and converter.

In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_NiagaraHandlers.cpp`:
- Around line 206-221: Remove the duplicate null-check blocks that re-check
NiagaraSystem and NiagaraEmitter after asset registration and saving;
specifically, delete the second if (!NiagaraSystem) {
SendAutomationError(RequestingSocket, RequestId, TEXT("Failed to create Niagara
system asset"), TEXT("ASSET_CREATION_FAILED")); return true; } in the Niagara
system handler (the block after FAssetRegistryModule::AssetCreated and
McpSafeAssetSave) and the analogous duplicate if (!NiagaraEmitter) error block
in HandleCreateNiagaraEmitter; keep the initial null checks, retain calls to
FAssetRegistryModule::AssetCreated and McpSafeAssetSave, and leave the original
SendAutomationError/error-return logic only where the first null check occurs.

In `@src/tools/handlers/effect-handlers.ts`:
- Around line 98-111: The alias precedence in the create_niagara_emitter branch
redundantly falls back to mutableArgs.emitterName even though the earlier
normalization already copied any legacy alias into mutableArgs.emitterName,
making that fallback dead; update the resolvedName expression to only consider
argsTyped.emitterName, argsTyped.name, then the literal 'DefaultEmitter' (i.e.,
remove the mutableArgs.emitterName fallback), leaving resolvedSavePath logic
unchanged and ensuring executeAutomationRequest is called with name:
resolvedName and savePath: resolvedSavePath.
🪄 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: 86005dc1-34fe-4623-93e6-30a08d6f8950

📥 Commits

Reviewing files that changed from the base of the PR and between f61817a and edf5d71.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (21)
  • .github/workflows/greetings.yml
  • .github/workflows/links.yml
  • .github/workflows/pr-size-labeler.yml
  • .github/workflows/release-drafter.yml
  • plugins/McpAutomationBridge/Source/McpAutomationBridge/McpAutomationBridge.Build.cs
  • plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridgePCH.h
  • plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridgeSubsystem.cpp
  • plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_AssetWorkflowHandlers.cpp
  • plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_CharacterHandlers.cpp
  • plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_CombatHandlers.cpp
  • plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_ControlHandlers.cpp
  • plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_EnvironmentHandlers.cpp
  • plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_GameFrameworkHandlers.cpp
  • plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_MaterialAuthoringHandlers.cpp
  • plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_NiagaraHandlers.cpp
  • plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_TextureHandlers.cpp
  • plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpSafeOperations.h
  • src/tools/consolidated-tool-definitions.ts
  • src/tools/handlers/actor-handlers.ts
  • src/tools/handlers/effect-handlers.ts
  • src/tools/handlers/material-authoring-handlers.ts

Comment on lines +3087 to +3129
// Synchronous capture: force a redraw, read pixels, encode PNG, write file.
// FScreenshotRequest::RequestScreenshot() is unreliable in editor mode
// (game-tick driven) and often never fires, so we capture directly.
const FIntPoint ViewportSize = Viewport->GetSizeXY();
if (ViewportSize.X <= 0 || ViewportSize.Y <= 0)
{
SendStandardErrorResponse(this, Socket, RequestId, TEXT("VIEWPORT_NOT_READY"),
TEXT("Viewport has zero size"), nullptr);
return true;
}

Viewport->Draw();
FlushRenderingCommands();

TArray<FColor> Bitmap;
const FReadSurfaceDataFlags ReadFlags(RCM_UNorm);
if (!Viewport->ReadPixels(Bitmap, ReadFlags))
{
SendStandardErrorResponse(this, Socket, RequestId, TEXT("CAPTURE_FAILED"),
TEXT("Failed to read pixels from viewport"), nullptr);
return true;
}

// Force opaque alpha for consistent PNG output
for (FColor& Pixel : Bitmap)
{
Pixel.A = 255;
}

TArray64<uint8> PngData;
FImageUtils::PNGCompressImageArray(
ViewportSize.X,
ViewportSize.Y,
TArrayView64<const FColor>(Bitmap.GetData(), Bitmap.Num()),
PngData);

if (PngData.Num() == 0 || !FFileHelper::SaveArrayToFile(PngData, *FullPath))
{
SendStandardErrorResponse(this, Socket, RequestId, TEXT("SAVE_FAILED"),
FString::Printf(TEXT("Failed to save screenshot to %s"), *FullPath),
nullptr);
return true;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🌐 Web query:

Unreal Engine 5.7 FImageUtils::PNGCompressImageArray signature TArrayView64

💡 Result:

The function signature for FImageUtils::PNGCompressImageArray in Unreal Engine 5.7 is: static void PNGCompressImageArray( int32 ImageWidth, int32 ImageHeight, const TArrayView64& SrcData, TArray64& DstData ) This function compresses image data to PNG format into a uint8 array. It is marked as deprecated in the documentation; the recommended alternative is FImageUtils::CompressImage with ToFormatExtension set to "png".

Citations:


🏁 Script executed:

# Read the specific file and lines to verify the code snippet
sed -n '3087,3129p' plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_ControlHandlers.cpp

Repository: ChiR24/Unreal_mcp

Length of output: 1555


🏁 Script executed:

# Find and examine the subsystem to understand handler dispatch
find plugins/McpAutomationBridge -name "*Subsystem*" -type f | head -20

Repository: ChiR24/Unreal_mcp

Length of output: 249


🏁 Script executed:

# Search for other viewport operations or async screenshot patterns in the handlers
rg -A 5 -B 5 "Viewport->Draw\(\)|ReadPixels" plugins/McpAutomationBridge --type cpp

Repository: ChiR24/Unreal_mcp

Length of output: 5452


🏁 Script executed:

# Check if PNGCompressImageArray is used elsewhere and how
rg "PNGCompressImageArray|FImageUtils::" plugins/McpAutomationBridge --type cpp -B 2 -A 2

Repository: ChiR24/Unreal_mcp

Length of output: 5041


🏁 Script executed:

# Check how the subsystem dispatches handlers to understand the game-thread blocking concern
head -100 plugins/McpAutomationBridge/Source/McpAutomationBridge/Public/McpAutomationBridgeSubsystem.h | grep -A 10 -B 10 "game thread\|dispatch\|async"

Repository: ChiR24/Unreal_mcp

Length of output: 43


🏁 Script executed:

# Check if there are any resolution caps or viewport size guards elsewhere in handlers
rg "ViewportSize|MaxViewport|CaptureSizeLimit|Resolution.*cap\|Size.*threshold" plugins/McpAutomationBridge --type cpp -B 2 -A 2

Repository: ChiR24/Unreal_mcp

Length of output: 2723


🏁 Script executed:

# Check the subsystem implementation to confirm game-thread dispatch for handlers
sed -n '1,150p' plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridgeSubsystem.cpp | grep -A 20 -B 5 "DispatchCommand\|ExecuteRequest\|game thread"

Repository: ChiR24/Unreal_mcp

Length of output: 43


🏁 Script executed:

# Look at the subsystem implementation to understand request dispatch
wc -l plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridgeSubsystem.cpp

Repository: ChiR24/Unreal_mcp

Length of output: 161


🏁 Script executed:

# Search for where handlers are called in the subsystem
rg "Handle.*Request|Execute.*Handler|dispatch" plugins/McpAutomationBridge --type cpp -B 2 -A 2 | head -40

Repository: ChiR24/Unreal_mcp

Length of output: 5696


🏁 Script executed:

# Look for where handlers are invoked in the subsystem to understand the dispatch mechanism
rg "HandleScreenshot|Handle.*\(" plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridgeSubsystem.cpp -B 2 -A 2 | head -60

Repository: ChiR24/Unreal_mcp

Length of output: 2266


🏁 Script executed:

# Check for game thread dispatch patterns in the subsystem
rg "AsyncTask|IsInGameThread|Async|dispatch|GameThreadTask" plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridgeSubsystem.cpp -B 2 -A 2 | head -40

Repository: ChiR24/Unreal_mcp

Length of output: 1599


🏁 Script executed:

# Find where handlers are invoked - likely in ProcessPendingAutomationRequests or a dispatch function
rg "ProcessPendingAutomationRequests|AutomationHandlers\[|AutomationHandlers\.Find" plugins/McpAutomationBridge/Source/McpAutomationBridge --type cpp -B 3 -A 5 | head -80

Repository: ChiR24/Unreal_mcp

Length of output: 7319


🏁 Script executed:

# Check if handlers are invoked with AsyncTask or on game thread
rg "FFunctionGraphTask|AsyncTask.*GameThread|IsInGameThread" plugins/McpAutomationBridge/Source/McpAutomationBridge/Private --type cpp -B 3 -A 3

Repository: ChiR24/Unreal_mcp

Length of output: 23612


Address synchronous viewport capture blocking and add buffer validation.

Viewport->Draw() + FlushRenderingCommands() + ReadPixels() + PNG encoding all execute synchronously on the game thread and can stall the editor for hundreds of ms to seconds on large viewports, blocking the WebSocket request loop.

Consider:

  • Logging a warning and/or capping capture resolution when ViewportSize.X * ViewportSize.Y exceeds a threshold (e.g., 4K = 8.3M pixels).
  • Adding a sanity check: validate Bitmap.Num() == ViewportSize.X * ViewportSize.Y before pixel iteration and PNG compression, since some ReadPixels failure paths can return mismatched buffer sizes, producing a corrupt PNG instead of an error response.

Also, FImageUtils::PNGCompressImageArray is deprecated in UE 5.7; other handlers in the codebase use version-aware conditionals (ThumbnailCompressImageArray for 5.1+, CompressImageArray for 5.0). Consider aligning this code with that pattern.

Optional hardening: buffer validation
  TArray<FColor> Bitmap;
  const FReadSurfaceDataFlags ReadFlags(RCM_UNorm);
  if (!Viewport->ReadPixels(Bitmap, ReadFlags))
  {
    SendStandardErrorResponse(this, Socket, RequestId, TEXT("CAPTURE_FAILED"),
                              TEXT("Failed to read pixels from viewport"), nullptr);
    return true;
  }
+  const int32 ExpectedPixels = ViewportSize.X * ViewportSize.Y;
+  if (Bitmap.Num() != ExpectedPixels)
+  {
+    SendStandardErrorResponse(this, Socket, RequestId, TEXT("CAPTURE_FAILED"),
+                              FString::Printf(TEXT("Pixel buffer size mismatch: got %d, expected %d"),
+                                              Bitmap.Num(), ExpectedPixels), nullptr);
+    return true;
+  }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_ControlHandlers.cpp`
around lines 3087 - 3129, The synchronous capture path using Viewport->Draw(),
FlushRenderingCommands(), Viewport->ReadPixels(...) and
FImageUtils::PNGCompressImageArray can stall the editor and produce corrupt
output; add a pixel-count sanity check (verify Bitmap.Num() == ViewportSize.X *
ViewportSize.Y and return
SEND_STANDARD_ERROR_RESPONSE("CAPTURE_FAILED"/"BUFFER_MISMATCH") if it differs),
log and/or cap very large captures by checking ViewportSize.X * ViewportSize.Y
against a threshold (e.g., 8.3e6) and downscale or reject with a clear error,
and replace the deprecated FImageUtils::PNGCompressImageArray call with the
version-aware helper used elsewhere (ThumbnailCompressImageArray for 5.1+ or
CompressImageArray for 5.0) to match the codebase pattern.

Comment on lines +1863 to +1889
else if (World)
{
// AWorldSettings::DefaultGameMode — per-level override
if (AWorldSettings* WorldSettings = World->GetWorldSettings())
{
if (UClass* LevelGameMode = WorldSettings->DefaultGameMode.Get())
{
ResolvedGameModeClass = LevelGameMode;
InfoObj->SetStringField(TEXT("source"), TEXT("levelDefault"));
}
}

// Fall back to project settings via the public static accessor
if (!ResolvedGameModeClass)
{
const FString DefaultGameModeStr = UGameMapsSettings::GetGlobalDefaultGameMode();
if (!DefaultGameModeStr.IsEmpty())
{
FSoftClassPath DefaultGameModePath(DefaultGameModeStr);
if (UClass* ProjGameMode = DefaultGameModePath.TryLoadClass<AGameModeBase>())
{
ResolvedGameModeClass = ProjGameMode;
InfoObj->SetStringField(TEXT("source"), TEXT("projectDefault"));
}
}
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Move the project-default fallback outside the world guard.

If World is unavailable, this skips UGameMapsSettings::GetGlobalDefaultGameMode() and returns no GameMode even though the project default can still be resolved.

Proposed fix
-            else if (World)
+            else if (World)
             {
                 // AWorldSettings::DefaultGameMode — per-level override
                 if (AWorldSettings* WorldSettings = World->GetWorldSettings())
                 {
                     if (UClass* LevelGameMode = WorldSettings->DefaultGameMode.Get())
@@
                         InfoObj->SetStringField(TEXT("source"), TEXT("levelDefault"));
                     }
                 }
+            }
 
-                // Fall back to project settings via the public static accessor
-                if (!ResolvedGameModeClass)
+            // Fall back to project settings via the public static accessor
+            if (!ResolvedGameModeClass)
+            {
+                const FString DefaultGameModeStr = UGameMapsSettings::GetGlobalDefaultGameMode();
+                if (!DefaultGameModeStr.IsEmpty())
                 {
-                    const FString DefaultGameModeStr = UGameMapsSettings::GetGlobalDefaultGameMode();
-                    if (!DefaultGameModeStr.IsEmpty())
+                    FSoftClassPath DefaultGameModePath(DefaultGameModeStr);
+                    if (UClass* ProjGameMode = DefaultGameModePath.TryLoadClass<AGameModeBase>())
                     {
-                        FSoftClassPath DefaultGameModePath(DefaultGameModeStr);
-                        if (UClass* ProjGameMode = DefaultGameModePath.TryLoadClass<AGameModeBase>())
-                        {
-                            ResolvedGameModeClass = ProjGameMode;
-                            InfoObj->SetStringField(TEXT("source"), TEXT("projectDefault"));
-                        }
+                        ResolvedGameModeClass = ProjGameMode;
+                        InfoObj->SetStringField(TEXT("source"), TEXT("projectDefault"));
                     }
                 }
             }
📝 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
else if (World)
{
// AWorldSettings::DefaultGameMode — per-level override
if (AWorldSettings* WorldSettings = World->GetWorldSettings())
{
if (UClass* LevelGameMode = WorldSettings->DefaultGameMode.Get())
{
ResolvedGameModeClass = LevelGameMode;
InfoObj->SetStringField(TEXT("source"), TEXT("levelDefault"));
}
}
// Fall back to project settings via the public static accessor
if (!ResolvedGameModeClass)
{
const FString DefaultGameModeStr = UGameMapsSettings::GetGlobalDefaultGameMode();
if (!DefaultGameModeStr.IsEmpty())
{
FSoftClassPath DefaultGameModePath(DefaultGameModeStr);
if (UClass* ProjGameMode = DefaultGameModePath.TryLoadClass<AGameModeBase>())
{
ResolvedGameModeClass = ProjGameMode;
InfoObj->SetStringField(TEXT("source"), TEXT("projectDefault"));
}
}
}
}
else if (World)
{
// AWorldSettings::DefaultGameMode — per-level override
if (AWorldSettings* WorldSettings = World->GetWorldSettings())
{
if (UClass* LevelGameMode = WorldSettings->DefaultGameMode.Get())
{
ResolvedGameModeClass = LevelGameMode;
InfoObj->SetStringField(TEXT("source"), TEXT("levelDefault"));
}
}
}
// Fall back to project settings via the public static accessor
if (!ResolvedGameModeClass)
{
const FString DefaultGameModeStr = UGameMapsSettings::GetGlobalDefaultGameMode();
if (!DefaultGameModeStr.IsEmpty())
{
FSoftClassPath DefaultGameModePath(DefaultGameModeStr);
if (UClass* ProjGameMode = DefaultGameModePath.TryLoadClass<AGameModeBase>())
{
ResolvedGameModeClass = ProjGameMode;
InfoObj->SetStringField(TEXT("source"), TEXT("projectDefault"));
}
}
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_GameFrameworkHandlers.cpp`
around lines 1863 - 1889, The project-default fallback
(UGameMapsSettings::GetGlobalDefaultGameMode and its TryLoadClass logic) is
currently inside the "if (World)" guard so it won't run when World is null; move
the entire block that computes DefaultGameModeStr, constructs FSoftClassPath,
TryLoadClass<AGameModeBase>(), assigns ResolvedGameModeClass and sets
InfoObj->SetStringField(TEXT("source"), TEXT("projectDefault")) to run
outside/after the World block, but only perform it when ResolvedGameModeClass is
still null so levelDefault (AWorldSettings::DefaultGameMode) wins if present.

Comment on lines +1553 to +1563
// Remove the default "A" input if it's the only one and still unused.
if (CustomExpr->Inputs.Num() == 1
&& CustomExpr->Inputs[0].InputName == FName(TEXT("A"))
&& CustomExpr->Inputs[0].Input.Expression == nullptr)
{
CustomExpr->Inputs.Empty();
}

FCustomInput NewInput;
NewInput.InputName = NewName;
CustomExpr->Inputs.Add(NewInput);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Don’t delete the default A pin while “adding” another custom pin.

With the current flow, calling add_custom_input_pin for A then B still removes A when B is added, unless A is already wired. That makes valid custom code such as A + B hard to build incrementally.

🐛 Proposed fix
-    // Remove the default "A" input if it's the only one and still unused.
-    if (CustomExpr->Inputs.Num() == 1
-        && CustomExpr->Inputs[0].InputName == FName(TEXT("A"))
-        && CustomExpr->Inputs[0].Input.Expression == nullptr)
-    {
-      CustomExpr->Inputs.Empty();
-    }
-
     FCustomInput NewInput;
     NewInput.InputName = NewName;
     CustomExpr->Inputs.Add(NewInput);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_MaterialAuthoringHandlers.cpp`
around lines 1553 - 1563, The current logic removes the sole default "A" input
whenever a new input is added, causing a user-created "A" to be lost when
subsequently adding "B"; to fix, change the flow in the handler that adds inputs
(e.g., add_custom_input_pin) so that if NewName == FName(TEXT("A")) and
CustomExpr->Inputs has exactly one entry whose InputName == FName(TEXT("A")) and
Input.Expression == nullptr you do NOT add a duplicate but instead reuse/leave
the existing default A; otherwise (NewName != "A") you may remove the unused
default A only when it's truly the sole unused default (keep the existing
conditional but add the NewName != FName(TEXT("A")) guard) before creating and
CustomExpr->Inputs.Add(NewInput).

Comment on lines +3412 to +3426
static TArray<TObjectPtr<UMaterialExpression>>& GetMatOrFuncExpressions(
UMaterial* Material, UMaterialFunction* MaterialFunction)
{
static TArray<TObjectPtr<UMaterialExpression>> Empty;
#if WITH_EDITORONLY_DATA
if (Material)
{
return Material->GetEditorOnlyData()->ExpressionCollection.Expressions;
}
if (MaterialFunction)
{
return MaterialFunction->GetEditorOnlyData()->ExpressionCollection.Expressions;
}
#endif
return Empty;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Read-only check: compare the new helper against existing version-guarded expression collection access.
rg -n -C3 'GetMatOrFuncExpressions|FunctionExpressions|GetEditorOnlyData\(\)->ExpressionCollection\.Expressions|MCP_GET_MATERIAL_EXPRESSIONS|ENGINE_MINOR_VERSION >= 1' \
  plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_MaterialAuthoringHandlers.cpp

Repository: ChiR24/Unreal_mcp

Length of output: 8619


🏁 Script executed:

#!/bin/bash
# Check plugin configuration for minimum UE version
find plugins/McpAutomationBridge -name "*.uplugin" -exec cat {} \; | head -50

# Also check if there's any version documentation in AGENTS.md or README
find plugins/McpAutomationBridge -name "AGENTS.md" -o -name "README.md" | xargs grep -l "5\.0\|5\.1\|version" 2>/dev/null | head -5

Repository: ChiR24/Unreal_mcp

Length of output: 1603


🏁 Script executed:

#!/bin/bash
# Check the context around GetMatOrFuncExpressions calls with MaterialFunction
# Lines 1110 and 1493 to see if they're version-guarded
sed -n '1100,1120p' plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_MaterialAuthoringHandlers.cpp
echo "---"
sed -n '1485,1500p' plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_MaterialAuthoringHandlers.cpp

Repository: ChiR24/Unreal_mcp

Length of output: 1371


🏁 Script executed:

#!/bin/bash
# Check the function signatures and call sites for add_math_node and add_custom_expression
# to see if MaterialFunction parameter is actually populated from request data

# Find the handler function for add_math_node
grep -n "add_math_node" plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_MaterialAuthoringHandlers.cpp | head -3

# Find where MaterialFunction is passed/assigned in add_math_node handler
sed -n '1000,1150p' plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_MaterialAuthoringHandlers.cpp | grep -n "MaterialFunction\|add_math_node"

Repository: ChiR24/Unreal_mcp

Length of output: 557


🏁 Script executed:

#!/bin/bash
# Get the full function signature and parameter handling for add_math_node
sed -n '1050,1120p' plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_MaterialAuthoringHandlers.cpp

Repository: ChiR24/Unreal_mcp

Length of output: 3112


🏁 Script executed:

#!/bin/bash
# Find the LOAD_MATERIAL_OR_FUNCTION_OR_RETURN macro definition
grep -n "define LOAD_MATERIAL_OR_FUNCTION_OR_RETURN\|LOAD_MATERIAL_OR_FUNCTION_OR_RETURN()" plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_MaterialAuthoringHandlers.cpp -A 20 | head -40

Repository: ChiR24/Unreal_mcp

Length of output: 2637


🏁 Script executed:

#!/bin/bash
# Check when GetEditorOnlyData was introduced for MaterialFunction
# Also check if there's any documentation or code comments about UE 5.0 MaterialFunction support

# Search for any version-guarded code that handles UE 5.0 MaterialFunction separately
grep -n "FunctionExpressions\|GetEditorOnlyData" plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_MaterialAuthoringHandlers.cpp | grep -B2 -A2 "MaterialFunction\|Func"

Repository: ChiR24/Unreal_mcp

Length of output: 572


🏁 Script executed:

#!/bin/bash
# Verify that GetMatOrFuncExpressions is indeed called with MaterialFunction parameter
# in both add_math_node (line 1110) and add_custom_expression (line 1493)
grep -n "GetMatOrFuncExpressions.*MaterialFunction" plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_MaterialAuthoringHandlers.cpp

Repository: ChiR24/Unreal_mcp

Length of output: 278


Add version guards to GetMatOrFuncExpressions for UE 5.0 MaterialFunction compatibility.

The plugin supports UE 5.0-5.7 (per .uplugin), but this helper unconditionally uses GetEditorOnlyData()->ExpressionCollection.Expressions for MaterialFunction, which doesn't exist in UE 5.0. The same file documents at line 2028 that UE 5.0 MaterialFunction requires FunctionExpressions instead. This will break or crash add_math_node and add_custom_expression when called with MaterialFunction on UE 5.0.

Update the helper to match the existing pattern in lines 2029-2033:

Version-guarded fix
static TArray<TObjectPtr<UMaterialExpression>>& GetMatOrFuncExpressions(
    UMaterial* Material, UMaterialFunction* MaterialFunction)
{
  static TArray<TObjectPtr<UMaterialExpression>> Empty;
`#if` WITH_EDITORONLY_DATA
  if (Material)
  {
    return Material->GetEditorOnlyData()->ExpressionCollection.Expressions;
  }
  if (MaterialFunction)
  {
`#if` ENGINE_MAJOR_VERSION == 5 && ENGINE_MINOR_VERSION >= 1
    return MaterialFunction->GetEditorOnlyData()->ExpressionCollection.Expressions;
`#else`
    return MaterialFunction->FunctionExpressions;
`#endif`
  }
`#endif`
  return Empty;
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_MaterialAuthoringHandlers.cpp`
around lines 3412 - 3426, GetMatOrFuncExpressions currently returns
MaterialFunction->GetEditorOnlyData()->ExpressionCollection.Expressions
unconditionally which does not exist in UE 5.0; update the function to
version-guard the MaterialFunction branch so that for ENGINE_MAJOR_VERSION == 5
&& ENGINE_MINOR_VERSION >= 1 you return
MaterialFunction->GetEditorOnlyData()->ExpressionCollection.Expressions,
otherwise return MaterialFunction->FunctionExpressions (matching the pattern
used earlier for 5.0 compatibility), while keeping the Material branch and the
Empty fallback unchanged.

Comment on lines 176 to 204
UNiagaraSystem *NiagaraSystem = NewObject<UNiagaraSystem>(Package, FName(*AssetName), RF_Public | RF_Standalone | RF_Transactional);
if (NiagaraSystem)
{
// Initialize system scripts
UNiagaraScript* SystemSpawnScript = NiagaraSystem->GetSystemSpawnScript();
UNiagaraScript* SystemUpdateScript = NiagaraSystem->GetSystemUpdateScript();
// Create script source and graph for system
UNiagaraScriptSource* SystemScriptSource = NewObject<UNiagaraScriptSource>(SystemSpawnScript, TEXT("SystemScriptSource"), RF_Transactional);

UNiagaraScriptSource* SystemScriptSource = NewObject<UNiagaraScriptSource>(
SystemSpawnScript, TEXT("SystemScriptSource"), RF_Transactional);
if (SystemScriptSource)
{
UNiagaraGraph* SystemGraph = NewObject<UNiagaraGraph>(SystemScriptSource, TEXT("SystemScriptGraph"), RF_Transactional);
SystemScriptSource->NodeGraph = SystemGraph;

// Set source on both system scripts
SystemScriptSource->NodeGraph = NewObject<UNiagaraGraph>(
SystemScriptSource, TEXT("SystemScriptGraph"), RF_Transactional);
}

if (SystemSpawnScript)
{
SystemSpawnScript->SetLatestSource(SystemScriptSource);
SystemUpdateScript->SetLatestSource(SystemScriptSource);
}

// Add default emitter with proper GraphSource initialization
UNiagaraEmitter *NewEmitter = NewObject<UNiagaraEmitter>(NiagaraSystem, FName(TEXT("DefaultEmitter")), RF_Transactional);
if (NewEmitter)
if (SystemUpdateScript)
{
// Create script source and graph for emitter
UNiagaraScriptSource* EmitterSource = NewObject<UNiagaraScriptSource>(NewEmitter, NAME_None, RF_Transactional);
if (EmitterSource)
{
UNiagaraGraph* EmitterGraph = NewObject<UNiagaraGraph>(EmitterSource, NAME_None, RF_Transactional);
EmitterSource->NodeGraph = EmitterGraph;

// Set GraphSource - API differs between engine versions
// UE 5.0: GraphSource is directly on UNiagaraEmitter
// UE 5.1+: GraphSource is on FVersionedNiagaraEmitterData, accessed via GetLatestEmitterData()
#if ENGINE_MAJOR_VERSION == 5 && ENGINE_MINOR_VERSION == 0
// UE 5.0: Set GraphSource directly on emitter
NewEmitter->GraphSource = EmitterSource;

// Set source on emitter scripts
if (NewEmitter->SpawnScriptProps.Script)
NewEmitter->SpawnScriptProps.Script->SetLatestSource(EmitterSource);
if (NewEmitter->UpdateScriptProps.Script)
NewEmitter->UpdateScriptProps.Script->SetLatestSource(EmitterSource);
#if WITH_EDITORONLY_DATA
if (NewEmitter->EmitterSpawnScriptProps.Script)
NewEmitter->EmitterSpawnScriptProps.Script->SetLatestSource(EmitterSource);
if (NewEmitter->EmitterUpdateScriptProps.Script)
NewEmitter->EmitterUpdateScriptProps.Script->SetLatestSource(EmitterSource);
#endif
#else
// UE 5.1+: Access via GetLatestEmitterData()
FVersionedNiagaraEmitterData* EmitterData = NewEmitter->GetLatestEmitterData();
if (EmitterData)
{
EmitterData->GraphSource = EmitterSource;

// Set source on emitter scripts
if (EmitterData->SpawnScriptProps.Script)
EmitterData->SpawnScriptProps.Script->SetLatestSource(EmitterSource);
if (EmitterData->UpdateScriptProps.Script)
EmitterData->UpdateScriptProps.Script->SetLatestSource(EmitterSource);
#if WITH_EDITORONLY_DATA
if (EmitterData->EmitterSpawnScriptProps.Script)
EmitterData->EmitterSpawnScriptProps.Script->SetLatestSource(EmitterSource);
if (EmitterData->EmitterUpdateScriptProps.Script)
EmitterData->EmitterUpdateScriptProps.Script->SetLatestSource(EmitterSource);
#endif
}
#endif
}

// AddEmitterHandle: UE 5.0 uses 2 params, UE 5.1+ uses 3 params (with FGuid)
#if ENGINE_MAJOR_VERSION == 5 && ENGINE_MINOR_VERSION == 0
NiagaraSystem->AddEmitterHandle(*NewEmitter, FName(TEXT("DefaultEmitter")));
#else
NiagaraSystem->AddEmitterHandle(*NewEmitter, FName(TEXT("DefaultEmitter")), FGuid::NewGuid());
#endif
SystemUpdateScript->SetLatestSource(SystemScriptSource);
}

// ResetGraphForOutput() would add required output nodes to the graph but
// lives in NiagaraEditor without NIAGARAEDITOR_API export, so we can't call
// it from an external plugin. The system is still a valid asset without
// output nodes; the stack UI will prompt to add them on first open.
NiagaraSystem->RequestCompile(false);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

UNiagaraSystemFactoryNew InitializeSystem source code separate UNiagaraScriptSource for spawn and update scripts

💡 Result:

No publicly available source code for UNiagaraSystemFactoryNew::InitializeSystem was found in web searches across official Unreal Engine documentation, GitHub repositories, forums, or other technical sources. UNiagaraSystemFactoryNew is a UFactory subclass in the NiagaraEditor module (NiagaraSystemFactoryNew.h), used for creating new Niagara System assets. Niagara Systems use separate UNiagaraScript assets for different usages: System Spawn Script (SystemSpawnScript property in UNiagaraSystem for system-level spawn logic), and emitter scripts for spawn/update (Particle Spawn, Particle Update groups). These are managed via UNiagaraScriptSource and factories like UNiagaraScriptFactoryNew. System initialization occurs in FNiagaraSystemInstance::Init and FNiagaraSystemInstanceController::Initialize, binding parameters from the System's scripts including spawn/update logic to emitters. Separate UNiagaraScriptSource instances exist per script usage (spawn vs update), compiled into UNiagaraScript assets referenced by emitters/system.

Citations:


🏁 Script executed:

# First, locate and examine the actual file
find . -name "*NiagaraHandlers.cpp" -type f

Repository: ChiR24/Unreal_mcp

Length of output: 165


🏁 Script executed:

# Read the file to see the context around lines 176-204
cat -n plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_NiagaraHandlers.cpp | sed -n '170,210p'

Repository: ChiR24/Unreal_mcp

Length of output: 2264


🏁 Script executed:

# Search for any other Niagara system creation patterns in the codebase
rg -B 3 -A 3 "UNiagaraSystem" --type cpp -i "factory\|initialize\|createnode\|newobject" plugins/

Repository: ChiR24/Unreal_mcp

Length of output: 50473


🏁 Script executed:

# Look for Niagara-related comments or documentation files
find plugins/ -type f \( -name "*.h" -o -name "*.cpp" -o -name "*.md" \) | xargs grep -l "NiagaraScript\|NiagaraScriptSource" | head -10

Repository: ChiR24/Unreal_mcp

Length of output: 485


Create separate UNiagaraScriptSource instances for spawn and update scripts.

The code comment at line 170 claims to mirror UNiagaraSystemFactoryNew::InitializeSystem(), but the implementation shares a single UNiagaraScriptSource between both SystemSpawnScript->SetLatestSource() (line 192) and SystemUpdateScript->SetLatestSource() (line 196). Evidence indicates the factory creates separate source instances per script usage. With a shared source, graph mutations on one script's logic will affect the other, and the Niagara editor's stack UI may become confused when it expects two independent graphs.

♻️ Suggested: separate sources per script
-    UNiagaraScriptSource* SystemScriptSource = NewObject<UNiagaraScriptSource>(
-        SystemSpawnScript, TEXT("SystemScriptSource"), RF_Transactional);
-    if (SystemScriptSource)
-    {
-      SystemScriptSource->NodeGraph = NewObject<UNiagaraGraph>(
-          SystemScriptSource, TEXT("SystemScriptGraph"), RF_Transactional);
-    }
-
-    if (SystemSpawnScript)
-    {
-      SystemSpawnScript->SetLatestSource(SystemScriptSource);
-    }
-    if (SystemUpdateScript)
-    {
-      SystemUpdateScript->SetLatestSource(SystemScriptSource);
-    }
+    auto MakeSource = [](UNiagaraScript* Outer, const TCHAR* SourceName, const TCHAR* GraphName) -> UNiagaraScriptSource*
+    {
+      if (!Outer) return nullptr;
+      UNiagaraScriptSource* Source = NewObject<UNiagaraScriptSource>(Outer, SourceName, RF_Transactional);
+      if (Source)
+      {
+        Source->NodeGraph = NewObject<UNiagaraGraph>(Source, GraphName, RF_Transactional);
+      }
+      return Source;
+    };
+    if (UNiagaraScriptSource* SpawnSource = MakeSource(SystemSpawnScript, TEXT("SystemSpawnScriptSource"), TEXT("SystemSpawnScriptGraph")))
+    {
+      SystemSpawnScript->SetLatestSource(SpawnSource);
+    }
+    if (UNiagaraScriptSource* UpdateSource = MakeSource(SystemUpdateScript, TEXT("SystemUpdateScriptSource"), TEXT("SystemUpdateScriptGraph")))
+    {
+      SystemUpdateScript->SetLatestSource(UpdateSource);
+    }
📝 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
UNiagaraSystem *NiagaraSystem = NewObject<UNiagaraSystem>(Package, FName(*AssetName), RF_Public | RF_Standalone | RF_Transactional);
if (NiagaraSystem)
{
// Initialize system scripts
UNiagaraScript* SystemSpawnScript = NiagaraSystem->GetSystemSpawnScript();
UNiagaraScript* SystemUpdateScript = NiagaraSystem->GetSystemUpdateScript();
// Create script source and graph for system
UNiagaraScriptSource* SystemScriptSource = NewObject<UNiagaraScriptSource>(SystemSpawnScript, TEXT("SystemScriptSource"), RF_Transactional);
UNiagaraScriptSource* SystemScriptSource = NewObject<UNiagaraScriptSource>(
SystemSpawnScript, TEXT("SystemScriptSource"), RF_Transactional);
if (SystemScriptSource)
{
UNiagaraGraph* SystemGraph = NewObject<UNiagaraGraph>(SystemScriptSource, TEXT("SystemScriptGraph"), RF_Transactional);
SystemScriptSource->NodeGraph = SystemGraph;
// Set source on both system scripts
SystemScriptSource->NodeGraph = NewObject<UNiagaraGraph>(
SystemScriptSource, TEXT("SystemScriptGraph"), RF_Transactional);
}
if (SystemSpawnScript)
{
SystemSpawnScript->SetLatestSource(SystemScriptSource);
SystemUpdateScript->SetLatestSource(SystemScriptSource);
}
// Add default emitter with proper GraphSource initialization
UNiagaraEmitter *NewEmitter = NewObject<UNiagaraEmitter>(NiagaraSystem, FName(TEXT("DefaultEmitter")), RF_Transactional);
if (NewEmitter)
if (SystemUpdateScript)
{
// Create script source and graph for emitter
UNiagaraScriptSource* EmitterSource = NewObject<UNiagaraScriptSource>(NewEmitter, NAME_None, RF_Transactional);
if (EmitterSource)
{
UNiagaraGraph* EmitterGraph = NewObject<UNiagaraGraph>(EmitterSource, NAME_None, RF_Transactional);
EmitterSource->NodeGraph = EmitterGraph;
// Set GraphSource - API differs between engine versions
// UE 5.0: GraphSource is directly on UNiagaraEmitter
// UE 5.1+: GraphSource is on FVersionedNiagaraEmitterData, accessed via GetLatestEmitterData()
#if ENGINE_MAJOR_VERSION == 5 && ENGINE_MINOR_VERSION == 0
// UE 5.0: Set GraphSource directly on emitter
NewEmitter->GraphSource = EmitterSource;
// Set source on emitter scripts
if (NewEmitter->SpawnScriptProps.Script)
NewEmitter->SpawnScriptProps.Script->SetLatestSource(EmitterSource);
if (NewEmitter->UpdateScriptProps.Script)
NewEmitter->UpdateScriptProps.Script->SetLatestSource(EmitterSource);
#if WITH_EDITORONLY_DATA
if (NewEmitter->EmitterSpawnScriptProps.Script)
NewEmitter->EmitterSpawnScriptProps.Script->SetLatestSource(EmitterSource);
if (NewEmitter->EmitterUpdateScriptProps.Script)
NewEmitter->EmitterUpdateScriptProps.Script->SetLatestSource(EmitterSource);
#endif
#else
// UE 5.1+: Access via GetLatestEmitterData()
FVersionedNiagaraEmitterData* EmitterData = NewEmitter->GetLatestEmitterData();
if (EmitterData)
{
EmitterData->GraphSource = EmitterSource;
// Set source on emitter scripts
if (EmitterData->SpawnScriptProps.Script)
EmitterData->SpawnScriptProps.Script->SetLatestSource(EmitterSource);
if (EmitterData->UpdateScriptProps.Script)
EmitterData->UpdateScriptProps.Script->SetLatestSource(EmitterSource);
#if WITH_EDITORONLY_DATA
if (EmitterData->EmitterSpawnScriptProps.Script)
EmitterData->EmitterSpawnScriptProps.Script->SetLatestSource(EmitterSource);
if (EmitterData->EmitterUpdateScriptProps.Script)
EmitterData->EmitterUpdateScriptProps.Script->SetLatestSource(EmitterSource);
#endif
}
#endif
}
// AddEmitterHandle: UE 5.0 uses 2 params, UE 5.1+ uses 3 params (with FGuid)
#if ENGINE_MAJOR_VERSION == 5 && ENGINE_MINOR_VERSION == 0
NiagaraSystem->AddEmitterHandle(*NewEmitter, FName(TEXT("DefaultEmitter")));
#else
NiagaraSystem->AddEmitterHandle(*NewEmitter, FName(TEXT("DefaultEmitter")), FGuid::NewGuid());
#endif
SystemUpdateScript->SetLatestSource(SystemScriptSource);
}
// ResetGraphForOutput() would add required output nodes to the graph but
// lives in NiagaraEditor without NIAGARAEDITOR_API export, so we can't call
// it from an external plugin. The system is still a valid asset without
// output nodes; the stack UI will prompt to add them on first open.
NiagaraSystem->RequestCompile(false);
}
UNiagaraSystem *NiagaraSystem = NewObject<UNiagaraSystem>(Package, FName(*AssetName), RF_Public | RF_Standalone | RF_Transactional);
if (NiagaraSystem)
{
UNiagaraScript* SystemSpawnScript = NiagaraSystem->GetSystemSpawnScript();
UNiagaraScript* SystemUpdateScript = NiagaraSystem->GetSystemUpdateScript();
auto MakeSource = [](UNiagaraScript* Outer, const TCHAR* SourceName, const TCHAR* GraphName) -> UNiagaraScriptSource*
{
if (!Outer) return nullptr;
UNiagaraScriptSource* Source = NewObject<UNiagaraScriptSource>(Outer, SourceName, RF_Transactional);
if (Source)
{
Source->NodeGraph = NewObject<UNiagaraGraph>(Source, GraphName, RF_Transactional);
}
return Source;
};
if (UNiagaraScriptSource* SpawnSource = MakeSource(SystemSpawnScript, TEXT("SystemSpawnScriptSource"), TEXT("SystemSpawnScriptGraph")))
{
SystemSpawnScript->SetLatestSource(SpawnSource);
}
if (UNiagaraScriptSource* UpdateSource = MakeSource(SystemUpdateScript, TEXT("SystemUpdateScriptSource"), TEXT("SystemUpdateScriptGraph")))
{
SystemUpdateScript->SetLatestSource(UpdateSource);
}
// ResetGraphForOutput() would add required output nodes to the graph but
// lives in NiagaraEditor without NIAGARAEDITOR_API export, so we can't call
// it from an external plugin. The system is still a valid asset without
// output nodes; the stack UI will prompt to add them on first open.
NiagaraSystem->RequestCompile(false);
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_NiagaraHandlers.cpp`
around lines 176 - 204, The current implementation reuses one
UNiagaraScriptSource (SystemScriptSource) for both SystemSpawnScript and
SystemUpdateScript which can cause graph mutations to cross-affect both scripts;
instead create two distinct UNiagaraScriptSource instances (e.g.,
SpawnScriptSource and UpdateScriptSource), each with its own UNiagaraGraph
NodeGraph, using the respective script (SystemSpawnScript and
SystemUpdateScript) as the outer, and then call SetLatestSource on
SystemSpawnScript with SpawnScriptSource and on SystemUpdateScript with
UpdateScriptSource; keep the subsequent NiagaraSystem->RequestCompile(false)
call unchanged.

'add_static_switch_parameter', 'add_math_node', 'add_world_position', 'add_vertex_normal',
'add_pixel_depth', 'add_fresnel', 'add_reflection_vector', 'add_panner', 'add_rotator',
'add_noise', 'add_voronoi', 'add_if', 'add_switch', 'add_custom_expression',
'add_custom_input_pin',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Schema the new action’s response fields.

add_custom_input_pin can return inputName, inputCount, and alreadyExisted, but manage_material_authoring.outputSchema does not expose those fields, leaving the new action’s output contract unschema-backed.

Suggested schema addition
         ...commonSchemas.outputBase,
         assetPath: commonSchemas.assetPath,
         nodeId: commonSchemas.nodeId,
+        inputName: commonSchemas.inputName,
+        inputCount: commonSchemas.numberProp,
+        alreadyExisted: commonSchemas.booleanProp,
         materialInfo: {

As per coding guidelines, “Output schemas are registered during startup and validated before responses leave the server. Keep new tool outputs schema-backed.”

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/tools/consolidated-tool-definitions.ts` at line 1445, The
manage_material_authoring tool's outputSchema does not include the fields
returned by the new action add_custom_input_pin (inputName, inputCount,
alreadyExisted); update the manage_material_authoring.outputSchema in
src/tools/consolidated-tool-definitions.ts to add these properties with
appropriate types (string for inputName, integer/number for inputCount, boolean
for alreadyExisted) and mark required/optional as needed, then ensure the
updated schema is registered/used during startup so responses from
add_custom_input_pin are validated against the schema.

Comment on lines +1478 to +1482
inputs: {
type: 'array',
items: { type: 'object', properties: { name: { type: 'string' } } },
description: 'Optional: declare FCustomInput pins on add_custom_expression (e.g. [{"name":"TempK"},{"name":"Normal"}]).',
},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Require name for each custom input entry.

The schema currently accepts inputs: [{}], despite the documented shape requiring named pins. Reject malformed entries at schema validation time.

Suggested schema tightening
         inputs: {
           type: 'array',
-          items: { type: 'object', properties: { name: { type: 'string' } } },
+          items: {
+            type: 'object',
+            properties: { name: { type: 'string' } },
+            required: ['name'],
+            additionalProperties: false,
+          },
           description: 'Optional: declare FCustomInput pins on add_custom_expression (e.g. [{"name":"TempK"},{"name":"Normal"}]).',
         },
📝 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
inputs: {
type: 'array',
items: { type: 'object', properties: { name: { type: 'string' } } },
description: 'Optional: declare FCustomInput pins on add_custom_expression (e.g. [{"name":"TempK"},{"name":"Normal"}]).',
},
inputs: {
type: 'array',
items: {
type: 'object',
properties: { name: { type: 'string' } },
required: ['name'],
additionalProperties: false,
},
description: 'Optional: declare FCustomInput pins on add_custom_expression (e.g. [{"name":"TempK"},{"name":"Normal"}]).',
},
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/tools/consolidated-tool-definitions.ts` around lines 1478 - 1482, The
inputs array schema currently allows objects without a name; update the items
schema for the inputs property in consolidated-tool-definitions.ts so each item
requires a name (add required: ['name'] on the items object) and optionally set
additionalProperties: false if you want to reject unexpected fields; target the
inputs property definition and its items object to enforce that every custom
input entry includes a string name.

Comment on lines +469 to +472
const params = normalizeArgs(args, [
{ key: 'assetPath', aliases: ['materialPath'], required: true },
{ key: 'nodeId', required: true },
{ key: 'inputName', aliases: ['pinName'], required: true },

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Accept functionPath for material-function custom pins.

This action is needed for material function graphs too, but the handler rejects callers that pass functionPath. The bridge-side handler supports material/function targets, so keep the TypeScript alias consistent with the other function-oriented branches.

Suggested fix
         const params = normalizeArgs(args, [
-          { key: 'assetPath', aliases: ['materialPath'], required: true },
+          { key: 'assetPath', aliases: ['materialPath', 'functionPath'], required: true },
           { key: 'nodeId', required: true },
           { key: 'inputName', aliases: ['pinName'], required: true },
         ]);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/tools/handlers/material-authoring-handlers.ts` around lines 469 - 472,
The handler currently calls normalizeArgs to build params with assetPath aliased
to materialPath only, which rejects callers passing functionPath for
material-function graphs; update the normalizeArgs call so the assetPath entry
includes 'functionPath' in its aliases (e.g., { key: 'assetPath', aliases:
['materialPath','functionPath'], required: true }) so function-target callers
are accepted; keep the rest of the keys (nodeId, inputName) unchanged and ensure
any downstream uses of params.assetPath continue to work with the new alias.

@ChiR24 ChiR24 self-assigned this Jun 2, 2026
@ChiR24 ChiR24 changed the base branch from main to dev June 2, 2026 05:46
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
@ChiR24

ChiR24 commented Jun 2, 2026

Copy link
Copy Markdown
Owner

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 2, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
@github-actions github-actions Bot removed the size/xl label Jun 2, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_ControlHandlers.cpp (2)

3453-3657: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Honor or reject resolution instead of silently ignoring it.

src/tools/handlers/editor-handlers.ts forwards resolution for screenshots, but this handler never reads it in either branch. Requests asking for a deterministic capture size will always get the current window/viewport dimensions back, which breaks the screenshot contract. If resizing is not supported here yet, return INVALID_ARGUMENT instead of succeeding with a different size.

🤖 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/McpAutomationBridge_ControlHandlers.cpp`
around lines 3453 - 3657, HandleControlEditorScreenshot currently ignores the
optional "resolution" field in Payload; either honor it by parsing the
resolution (e.g., "resolution" or width/height fields) and resizing the target
capture area before capturing (both in the full_editor_window path that uses
CaptureSlateWindowPngForMcp and in the editor_viewport path that reads pixels
from Viewport), or explicitly reject unsupported resolution requests by checking
Payload->TryGetStringField / TryGetNumberField for resolution and returning
SendStandardErrorResponse(..., TEXT("INVALID_ARGUMENT"), ...) if a resolution is
present; update the code paths around Mode == "full_editor_window" and the
viewport capture branch in HandleControlEditorScreenshot to validate the
resolution input and either apply a resize step before capture or return the
INVALID_ARGUMENT error.

3489-3607: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Defer capture while the editor is in an unsafe state.

Both screenshot paths now do immediate capture work, but there is no guard for package saves, GC, or async loading before entering the capture flow. That leaves this handler executing exactly in the states the bridge guidelines call out as unsafe. Please defer or reject the request until the editor is safe before running either capture path. As per coding guidelines, "Defer work while Unreal is saving packages, garbage collecting, or async loading; do not add bypasses around unsafe-state checks".

🤖 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.

Outside diff comments:
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_ControlHandlers.cpp`:
- Around line 3453-3657: HandleControlEditorScreenshot currently ignores the
optional "resolution" field in Payload; either honor it by parsing the
resolution (e.g., "resolution" or width/height fields) and resizing the target
capture area before capturing (both in the full_editor_window path that uses
CaptureSlateWindowPngForMcp and in the editor_viewport path that reads pixels
from Viewport), or explicitly reject unsupported resolution requests by checking
Payload->TryGetStringField / TryGetNumberField for resolution and returning
SendStandardErrorResponse(..., TEXT("INVALID_ARGUMENT"), ...) if a resolution is
present; update the code paths around Mode == "full_editor_window" and the
viewport capture branch in HandleControlEditorScreenshot to validate the
resolution input and either apply a resize step before capture or return the
INVALID_ARGUMENT error.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 60574153-2265-4849-a8ec-6b50e893c82d

📥 Commits

Reviewing files that changed from the base of the PR and between 2c7f0eb and 78941f6.

📒 Files selected for processing (2)
  • plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_ControlHandlers.cpp
  • plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_EnvironmentHandlers.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
  • plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_EnvironmentHandlers.cpp

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
@coderabbitai coderabbitai Bot added javascript Pull requests that update javascript code size/m labels Jun 2, 2026
@ChiR24 ChiR24 merged commit 006b01f into ChiR24:dev Jun 2, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/plugin area/server bug Something isn't working javascript Pull requests that update javascript code size/l size/m

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants