fix: set TargetType on DynamicCast nodes via targetClass param#478
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesTargetClass support for cast and widget nodes
Sequence Diagram(s)sequenceDiagram
participant Client as MCP Client
participant Handler as HandleBlueprintAddNode
participant Factory as CreateBlueprintGraphNode
participant UE as UObject / StaticFindObject
participant DynCastNode as UK2Node_DynamicCast
Client->>Handler: add_node payload (nodeType, targetClass, ...)
Handler->>Handler: Extract targetClass from LocalPayload
Handler->>Handler: Fallback: memberClass, nodeClass, or derive from nodeType
Handler->>Factory: CreateBlueprintGraphNode(..., TargetClass, ...)
Factory->>Factory: NodeType matches cast keywords?
alt TargetClass is empty
Factory-->>Handler: OutErrorCode = INVALID_ARGUMENT
Handler-->>Client: error response
else TargetClass provided
Factory->>UE: StaticFindObject(TargetClass)
alt Class not found
Factory-->>Handler: OutErrorCode = CLASS_NOT_FOUND
Handler-->>Client: error response
else Class resolved
Factory->>DynCastNode: NewObject<UK2Node_DynamicCast>
Factory->>DynCastNode: TargetType = ResolvedUClass
Factory-->>Handler: UEdGraphNode*
Handler-->>Client: success response
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 10-24: The CHANGELOG.md contains transient merge/stash markers
("Updated upstream" and "Stashed changes") that are workspace metadata and
should not appear in published release notes. Remove both of these lines from
the changelog to keep only the actual release note content describing the bug
fixes and features.
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/Blueprint/Graph/McpAutomationBridge_BlueprintHandlersAddNode.cpp`:
- Around line 54-55: The code currently only reads the `targetClass` field from
the payload before forwarding it at lines 119-122, but existing request paths
still use legacy field names `memberClass` or `nodeClass`. Add fallback logic
after the initial TryGetStringField call for `targetClass` to also attempt
reading from `memberClass` and `nodeClass` fields if `targetClass` is empty.
This ensures the `TargetClass` variable gets properly populated regardless of
which legacy or current field name is present in the payload, preventing
DynamicCast creation failures with INVALID_ARGUMENT errors.
🪄 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: 58814abc-a279-4e16-946a-84348ca3bcc6
📒 Files selected for processing (4)
CHANGELOG.mdplugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/Blueprint/Graph/McpAutomationBridge_BlueprintHandlersAddNode.cppplugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/Blueprint/Graph/McpAutomationBridge_BlueprintHandlersAddNodeGraph.cppplugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/Blueprint/McpAutomationBridge_BlueprintHandlersDeclarations.h
mhsm555
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/BlueprintGraph/McpAutomationBridge_BlueprintGraphHandlersNodeCreation.cpp (1)
6-6: ⚡ Quick winAdd
__has_includeguards for cross-version compatibility.The parallel implementation in
McpAutomationBridge_BlueprintHandlersAddNodeGraph.cpp(Context snippet 1) wraps theK2Node_DynamicCast.hinclude with__has_includefallbacks to handle different header locations across Unreal Engine versions. This file should use the same defensive pattern to ensure compilation succeeds regardless of UE version.🔀 Suggested include guards
-#include "K2Node_DynamicCast.h" +#if defined(__has_include) +#if __has_include("BlueprintGraph/K2Node_DynamicCast.h") +#include "BlueprintGraph/K2Node_DynamicCast.h" +#elif __has_include("BlueprintGraph/Classes/K2Node_DynamicCast.h") +#include "BlueprintGraph/Classes/K2Node_DynamicCast.h" +#elif __has_include("K2Node_DynamicCast.h") +#include "K2Node_DynamicCast.h" +#endif +#else +#include "K2Node_DynamicCast.h" +#endif🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/BlueprintGraph/McpAutomationBridge_BlueprintGraphHandlersNodeCreation.cpp` at line 6, The include statement for K2Node_DynamicCast.h needs defensive preprocessing directives for cross-version compatibility. Replace the direct `#include` "K2Node_DynamicCast.h" with __has_include guards that check for the header availability and provide fallback alternatives for different Unreal Engine versions, following the same pattern already implemented in McpAutomationBridge_BlueprintHandlersAddNodeGraph.cpp. This ensures the compilation succeeds regardless of which UE version is being used.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/BlueprintGraph/McpAutomationBridge_BlueprintGraphHandlersNodeCreation.cpp`:
- Around line 150-223: The DynamicCast node resolution block contains inline
class-resolution logic (the section starting with checking if TargetClass starts
with "/" and progressing through LoadObject, FindNodeClassByName, and
UClass::TryFindTypeSlow) that duplicates the ResolveClassByName helper function.
Replace this entire inline resolution block with a single call to
ResolveClassByName(TargetClass), assigning the result to ResolvedTarget. This
eliminates the bug where a dot in the path prevents PackageName from being
updated correctly (causing malformed paths like
"/Game/Blueprints/BP_Cole.BP_Cole.BP_Cole_C"), and ensures consistency with
other code paths in the file that already use ResolveClassByName for class
resolution.
- Line 227: The CastNode SetPurity call is hardcoded to false instead of being
configurable like other payload-based settings in the BlueprintGraph handlers.
Extract an optional "pure" field from the payload object (defaulting to false if
not present) and pass that extracted value to the SetPurity method on the
CastNode instead of the hardcoded false value, making the cast purity
configurable by callers through the payload.
---
Nitpick comments:
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/BlueprintGraph/McpAutomationBridge_BlueprintGraphHandlersNodeCreation.cpp`:
- Line 6: The include statement for K2Node_DynamicCast.h needs defensive
preprocessing directives for cross-version compatibility. Replace the direct
`#include` "K2Node_DynamicCast.h" with __has_include guards that check for the
header availability and provide fallback alternatives for different Unreal
Engine versions, following the same pattern already implemented in
McpAutomationBridge_BlueprintHandlersAddNodeGraph.cpp. This ensures the
compilation succeeds regardless of which UE version is being used.
🪄 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: cfcfbc86-bde8-44e0-9d22-02d921137ddd
📒 Files selected for processing (1)
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/BlueprintGraph/McpAutomationBridge_BlueprintGraphHandlersNodeCreation.cpp
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/BlueprintGraph/McpAutomationBridge_BlueprintGraphHandlersNodeCreation.cpp (1)
27-44:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
PackageNameis never updated when dot is found, producing malformed paths.When
InClassStringcontains a dot (e.g.,"/Game/Blueprints/BP_Cole.BP_Cole"),PackageNameremains the full path while onlyObjectNameis updated. Line 44 then produces"/Game/Blueprints/BP_Cole.BP_Cole.BP_Cole_C"instead of the correct"/Game/Blueprints/BP_Cole.BP_Cole_C".🐛 Proposed fix
int32 DotIdx; if (ClassPath.FindChar('.', DotIdx)) { + PackageName = ClassPath.Left(DotIdx); ObjectName = ClassPath.RightChop(DotIdx + 1); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/BlueprintGraph/McpAutomationBridge_BlueprintGraphHandlersNodeCreation.cpp` around lines 27 - 44, The issue is that PackageName is initialized to the full ClassPath value but never updated when a dot is found in the path. When ClassPath contains a dot (e.g., "/Game/Blueprints/BP_Cole.BP_Cole"), the code correctly extracts ObjectName using RightChop after finding the dot position with FindChar, but PackageName remains unchanged as the full path. This causes line 44 to construct an incorrect path by concatenating the unmodified PackageName with the ObjectName. Fix this by updating PackageName to contain only the portion before the dot using Left(DotIdx) when a dot is found, so that both PackageName and ObjectName are properly extracted from the input ClassPath.
🤖 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.
Duplicate comments:
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/BlueprintGraph/McpAutomationBridge_BlueprintGraphHandlersNodeCreation.cpp`:
- Around line 27-44: The issue is that PackageName is initialized to the full
ClassPath value but never updated when a dot is found in the path. When
ClassPath contains a dot (e.g., "/Game/Blueprints/BP_Cole.BP_Cole"), the code
correctly extracts ObjectName using RightChop after finding the dot position
with FindChar, but PackageName remains unchanged as the full path. This causes
line 44 to construct an incorrect path by concatenating the unmodified
PackageName with the ObjectName. Fix this by updating PackageName to contain
only the portion before the dot using Left(DotIdx) when a dot is found, so that
both PackageName and ObjectName are properly extracted from the input ClassPath.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 743c148b-28fb-4e6e-a8b6-55ee45df620d
📒 Files selected for processing (2)
CHANGELOG.mdplugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/BlueprintGraph/McpAutomationBridge_BlueprintGraphHandlersNodeCreation.cpp
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Thanks for the fix! I think this needs one small follow-up before merging. The DynamicCast part looks good, but the CreateWidget fix seems to only cover the newer Could you also mirror the CreateWidget handling in the |
Mirror the create_node CreateWidget handling in the older add_node path so CreateWidget nodes are no longer created untyped there: - add_node payload now backfills targetClass from the legacy widgetType field, matching ReadTargetClassPayload on the create_node path - CreateBlueprintGraphNode resolves the requested widget class and sets the node's WidgetType UClass property via reflection before pin allocation, so the typed Return Value is built; returns INVALID_ARGUMENT / CLASS_NOT_FOUND on missing/unresolvable class - changelog: drop leftover branch-label lines from the Unreleased block
Thanks for the review — both addressed in the latest push: CreateWidget in the add_node path: mirrored the create_node handling. CreateBlueprintGraphNode now has a dedicated CreateWidget branch that resolves targetClass and sets the node's WidgetType via reflection before pin allocation, so the typed Return Value gets built. The add_node payload also now backfills targetClass from the legacy widgetType field, matching ReadTargetClassPayload. Missing/unresolvable class returns INVALID_ARGUMENT / CLASS_NOT_FOUND, consistent with the create_node path. |
2 participants
Summary
Creating a
K2Node_DynamicCast("Cast To ...") over MCP fell through to a generic node-instantiation path that never setTargetType, producing an unusable "Bad cast node" with only a wildcardObjectpin and no typedAs <Class>output. This made every cast created over MCP non-functional. This PR resolves the target class and assigns it so casts are created correctly.Changes
CreateBlueprintGraphNodethat reads atargetClassparameter (Blueprint asset path like/Game/Blueprints/BP_Cole, or a native class name), resolves it viaResolveClassByName, and assignsUK2Node_DynamicCast::TargetType.targetClassfrom theadd_node/create_nodepayload through to the node factory.CreateBlueprintGraphNodedeclaration signature accordingly.K2Node_DynamicCast.hinclude (with path fallbacks).INVALID_ARGUMENT/CLASS_NOT_FOUNDerrors when the target is missing or unresolvable.Related Issues
Type of Change
Testing
Pre-Merge Checklist