From 8d976c190d40120ba7fd77cd25fc85f350235ce6 Mon Sep 17 00:00:00 2001 From: jyelon Date: Tue, 10 Mar 2026 07:22:01 -0400 Subject: [PATCH] Clean up after refactor. --- .../Private/Handlers/RemainingIssues.md | 56 +++++++++++++ ...UMCPHandler_AddAnimStateToMachine.Notes.md | 29 ------- ...MCPHandler_AddAnimStateTransition.Notes.md | 29 ------- ...UMCPHandler_AddBlueprintComponent.Notes.md | 33 -------- ...UMCPHandler_AddBlueprintInterface.Notes.md | 24 ------ .../UMCPHandler_AddBlueprintVariable.Notes.md | 23 ------ .../UMCPHandler_AddEventDispatcher.Notes.md | 31 -------- .../UMCPHandler_AddFunctionParameter.Notes.md | 29 ------- ...UMCPHandler_AddMaterialExpression.Notes.md | 78 ------------------- .../UMCPHandler_AddStructField.Notes.md | 20 ----- .../Handlers/UMCPHandler_BackupAsset.Notes.md | 26 ------- ...ndler_ChangeBlueprintVariableType.Notes.md | 25 ------ ...ndler_ChangeFunctionParameterType.Notes.md | 48 ------------ .../UMCPHandler_ChangeStructNodeType.Notes.md | 39 ---------- ...r_CheckPinConnectionCompatibility.Notes.md | 29 ------- .../UMCPHandler_CompileBlueprint.Notes.md | 25 ------ .../UMCPHandler_CompileMaterial.Notes.md | 25 ------ ...ler_ConnectMaterialExpressionPins.Notes.md | 40 ---------- .../Handlers/UMCPHandler_ConnectPins.Notes.md | 28 ------- ...PHandler_CreateAnimBlueprintAsset.Notes.md | 24 ------ ...UMCPHandler_CreateBlendSpaceAsset.Notes.md | 32 -------- .../UMCPHandler_CreateBlueprintAsset.Notes.md | 29 ------- .../UMCPHandler_CreateBlueprintGraph.Notes.md | 28 ------- .../UMCPHandler_CreateEnumAsset.Notes.md | 28 ------- .../UMCPHandler_CreateMaterialAsset.Notes.md | 60 -------------- ...ndler_CreateMaterialFunctionAsset.Notes.md | 27 ------- ...ndler_CreateMaterialInstanceAsset.Notes.md | 29 ------- .../UMCPHandler_CreateStructAsset.Notes.md | 32 -------- .../Handlers/UMCPHandler_DeleteAsset.Notes.md | 31 -------- .../UMCPHandler_DeleteBlueprintGraph.Notes.md | 30 ------- ...PHandler_DeleteMaterialExpression.Notes.md | 30 ------- .../UMCPHandler_DeleteNodeFromGraph.Notes.md | 22 ------ ...Handler_DescribeMaterialInEnglish.Notes.md | 40 ---------- .../UMCPHandler_DiffTwoBlueprints.Notes.md | 31 -------- ...r_DisconnectMaterialExpressionPin.Notes.md | 31 -------- .../UMCPHandler_DisconnectPins.Notes.md | 26 ------- .../UMCPHandler_DumpBlueprint.Notes.md | 33 -------- .../Handlers/UMCPHandler_DumpGraphs.Notes.md | 39 ---------- .../UMCPHandler_DumpMaterial.Notes.md | 40 ---------- ...ndler_DumpMaterialExpressionGraph.Notes.md | 35 --------- .../UMCPHandler_DumpMaterialFunction.Notes.md | 34 -------- ...er_DumpMaterialInstanceParameters.Notes.md | 27 ------- ...UMCPHandler_DuplicateNodesInGraph.Notes.md | 32 -------- .../UMCPHandler_FindAssetReferences.Notes.md | 32 -------- ...MCPHandler_FindMaterialReferences.Notes.md | 28 ------- .../UMCPHandler_GetNodeComment.Notes.md | 30 ------- .../UMCPHandler_GetPinDetails.Notes.md | 27 ------- .../UMCPHandler_ListAnimSlotNames.Notes.md | 24 ------ .../UMCPHandler_ListAnimSyncGroups.Notes.md | 23 ------ .../UMCPHandler_ListBlueprintAssets.Notes.md | 26 ------- ...CPHandler_ListBlueprintComponents.Notes.md | 27 ------- ...CPHandler_ListBlueprintInterfaces.Notes.md | 25 ------ .../UMCPHandler_ListClassProperties.Notes.md | 31 -------- .../UMCPHandler_ListEventDispatchers.Notes.md | 55 ------------- ...MCPHandler_RefreshAllNodesInGraph.Notes.md | 20 ----- ...andler_RemoveAnimStateFromMachine.Notes.md | 26 ------- ...PHandler_RemoveBlueprintComponent.Notes.md | 29 ------- ...PHandler_RemoveBlueprintInterface.Notes.md | 24 ------ ...CPHandler_RemoveBlueprintVariable.Notes.md | 29 ------- ...CPHandler_RemoveFunctionParameter.Notes.md | 21 ----- .../UMCPHandler_RemoveStructField.Notes.md | 24 ------ .../Handlers/UMCPHandler_RenameAsset.Notes.md | 24 ------ .../UMCPHandler_RenameBlueprintGraph.Notes.md | 24 ------ .../UMCPHandler_ReparentBlueprint.Notes.md | 22 ------ ...PHandler_ReparentMaterialInstance.Notes.md | 29 ------- ...r_ReplaceFunctionCallsInBlueprint.Notes.md | 29 ------- .../UMCPHandler_RestoreAsset.Notes.md | 27 ------- .../UMCPHandler_SearchAssets.Notes.md | 23 ------ ...PHandler_SearchSpawnableNodeTypes.Notes.md | 29 ------- ...ndler_SearchTypeUsageInBlueprints.Notes.md | 28 ------- .../UMCPHandler_SearchUnrealClasses.Notes.md | 24 ------ ...MCPHandler_SearchWithinBlueprints.Notes.md | 28 ------- ...UMCPHandler_SearchWithinMaterials.Notes.md | 29 ------- ...UMCPHandler_SetAnimStateAnimation.Notes.md | 22 ------ ...MCPHandler_SetAnimStateBlendSpace.Notes.md | 32 -------- ...UMCPHandler_SetAnimTransitionRule.Notes.md | 31 -------- ...Handler_SetBlendSpaceSamplePoints.Notes.md | 35 --------- ...dler_SetBlueprintVariableMetadata.Notes.md | 28 ------- .../UMCPHandler_SetClassDefaultValue.Notes.md | 32 -------- ...ler_SetMaterialExpressionPosition.Notes.md | 25 ------ ...ler_SetMaterialExpressionProperty.Notes.md | 37 --------- ...dler_SetMaterialInstanceParameter.Notes.md | 33 -------- .../UMCPHandler_SetMaterialProperty.Notes.md | 33 -------- .../UMCPHandler_SetNodeComment.Notes.md | 19 ----- .../UMCPHandler_SetNodePositions.Notes.md | 22 ------ .../UMCPHandler_SetPinDefaultValues.Notes.md | 27 ------- .../UMCPHandler_ShowCommands.Notes.md | 18 ----- .../UMCPHandler_SpawnNodesInGraph.Notes.md | 33 -------- .../Source/BlueprintMCP/Private/TODO.md | 7 -- .../Source/BlueprintMCP/Private/paths.md | 11 --- 90 files changed, 56 insertions(+), 2613 deletions(-) create mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/RemainingIssues.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_AddAnimStateToMachine.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_AddAnimStateTransition.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_AddBlueprintComponent.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_AddBlueprintInterface.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_AddBlueprintVariable.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_AddEventDispatcher.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_AddFunctionParameter.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_AddMaterialExpression.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_AddStructField.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_BackupAsset.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ChangeBlueprintVariableType.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ChangeFunctionParameterType.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ChangeStructNodeType.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CheckPinConnectionCompatibility.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CompileBlueprint.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CompileMaterial.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ConnectMaterialExpressionPins.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ConnectPins.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateAnimBlueprintAsset.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateBlendSpaceAsset.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateBlueprintAsset.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateBlueprintGraph.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateEnumAsset.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateMaterialAsset.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateMaterialFunctionAsset.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateMaterialInstanceAsset.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateStructAsset.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DeleteAsset.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DeleteBlueprintGraph.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DeleteMaterialExpression.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DeleteNodeFromGraph.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DescribeMaterialInEnglish.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DiffTwoBlueprints.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DisconnectMaterialExpressionPin.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DisconnectPins.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DumpBlueprint.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DumpGraphs.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DumpMaterial.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DumpMaterialExpressionGraph.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DumpMaterialFunction.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DumpMaterialInstanceParameters.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DuplicateNodesInGraph.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_FindAssetReferences.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_FindMaterialReferences.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_GetNodeComment.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_GetPinDetails.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ListAnimSlotNames.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ListAnimSyncGroups.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ListBlueprintAssets.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ListBlueprintComponents.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ListBlueprintInterfaces.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ListClassProperties.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ListEventDispatchers.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RefreshAllNodesInGraph.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RemoveAnimStateFromMachine.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RemoveBlueprintComponent.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RemoveBlueprintInterface.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RemoveBlueprintVariable.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RemoveFunctionParameter.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RemoveStructField.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RenameAsset.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RenameBlueprintGraph.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ReparentBlueprint.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ReparentMaterialInstance.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ReplaceFunctionCallsInBlueprint.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RestoreAsset.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SearchAssets.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SearchSpawnableNodeTypes.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SearchTypeUsageInBlueprints.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SearchUnrealClasses.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SearchWithinBlueprints.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SearchWithinMaterials.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetAnimStateAnimation.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetAnimStateBlendSpace.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetAnimTransitionRule.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetBlendSpaceSamplePoints.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetBlueprintVariableMetadata.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetClassDefaultValue.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetMaterialExpressionPosition.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetMaterialExpressionProperty.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetMaterialInstanceParameter.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetMaterialProperty.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetNodeComment.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetNodePositions.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetPinDefaultValues.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ShowCommands.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SpawnNodesInGraph.Notes.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/TODO.md delete mode 100644 Plugins/BlueprintMCP/Source/BlueprintMCP/Private/paths.md diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/RemainingIssues.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/RemainingIssues.md new file mode 100644 index 00000000..642dd5c2 --- /dev/null +++ b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/RemainingIssues.md @@ -0,0 +1,56 @@ +# Serious Issues Remaining in BlueprintMCP Handlers + +## Breaking API Changes + +Several handlers switched from GUID-based node matching to +`MCPUtils::Identifies()`. Since the only caller is our own MCP +bridge (which uses `FormatName` output from dump commands), this +is actually fine — but it's a one-way door. + +- **ChangeStructNodeType, SetMaterialExpressionPosition, + DeleteMaterialExpression, DisconnectMaterialExpressionPin** — + node param now expects FormatName identifiers, not GUIDs +- **RemoveStructField, AddStructField** — switched from + MCPAssets (accepts bare names) to MCPFetcher (requires full + paths) +- **SetPinDefaultValues** — entry struct changed from + `{Blueprint, Node, PinName, Value}` to `{Pin, Value}` where + Pin is a full fetcher path +- **SpawnNodesInGraph** — changed from `Blueprint` + `Graph` + params to single `Graph` path + +## Potential Crashes + +- **SearchAssets** — `Data.GetClass()` can return null with + `.Info()` (asset class not loaded). Would crash on + `FormatName`. +- **SpawnNodesInGraph** — assumes `GetOuter()` of a graph is + always a `UBlueprint`. Fails for level blueprints. + +## Silent Error Handling Gaps + +- **AddAnimStateToMachine, SetAnimStateAnimation, + SetAnimTransitionRule** — `FindStateMachineGraph` and + `FindStateByName` lack MCPErrorCallback. If the state machine + or state isn't found, error reporting is ad-hoc/incomplete. + +## Behavioral Changes + +- **SetBlendSpaceSamplePoints** — now aborts on animation + lookup failure instead of silently inserting a blank sample. + Probably better behavior, but different. + +## Static Function Violation + +- **CompileBlueprint** — has a `static` `CompileAndReport` + method, violating the no-static-functions rule. + +## Minor Concerns + +- **SetNodePositions** — node search across all graphs could + match wrong node if names collide across graphs +- **ReplaceFunctionCallsInBlueprint** — connection survival + uses pointer comparison; could be unsafe if pins are + recreated during the replacement +- **DumpMaterialInstanceParameters** — parent chain lost class + type info (Material vs MaterialInstance) diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_AddAnimStateToMachine.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_AddAnimStateToMachine.Notes.md deleted file mode 100644 index 8356cacb..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_AddAnimStateToMachine.Notes.md +++ /dev/null @@ -1,29 +0,0 @@ -# UMCPHandler_AddAnimStateToMachine - Refactoring Notes - -## Changes Made - -- **Plain-text output**: Switched from `Handle(FJsonObject*)` to `Handle(FStringBuilderBase&)`. Output is now concise plain text instead of JSON fields. - -- **MCPFetcher for blueprint resolution**: Replaced `MCPAssets` for the initial blueprint lookup with `MCPFetcher(Result).Walk(Blueprint).Cast()`. This gives consistent path/name resolution and automatic error reporting. - -- **FormatName for all output**: All names in output and error messages now use `MCPUtils::FormatName()` instead of raw string parameters. - -- **Error reporting via Result**: All errors go through the `FStringBuilderBase& Result` output. The animation asset search uses `.Errors(Result)` so MCPAssets reports errors directly to the caller. - -- **Removed unused includes**: Dropped `EdGraphNode.h`, `EdGraphPin.h`, `BlendSpace.h`, `AnimGraphNode_BlendSpacePlayer.h`, `AnimStateTransitionNode.h`, `K2Node_VariableGet.h` -- none were used by this handler. - -- **Removed dead-end on animation asset failure**: The old code silently swallowed animation asset lookup failures (it set `AnimSeq = nullptr` and continued without reporting). Now the handler returns an error via MCPAssets' error callback if the animation asset is not found or ambiguous. - -## What Looks Good - -- The core logic for creating a state node, renaming its bound graph, and optionally attaching a sequence player is solid and well-structured. -- The duplicate state name check is a good guard. -- Nesting depth is within limits. - -## Areas for Future Consideration - -- **FindStateMachineGraph vs MCPFetcher**: I kept `MCPUtils::FindStateMachineGraph` because state machine graphs are nested sub-graphs and may not be reachable via MCPFetcher's `Graph()` walker (which walks top-level blueprint graphs). If the fetcher is later extended to walk sub-graphs or state machine graphs specifically, this handler could be simplified further. - -- **Batch support**: This handler creates a single state per call. If adding many states is a common workflow, a batch version (accepting an array of state definitions) would reduce round-trips. I did not add this since it was not requested and would change the API. - -- **Save result**: The old code returned `bSaved` as a JSON field. The new version calls `SaveBlueprintPackage` but does not report save failure. If save failure is important to surface, a check could be added, but it would add noise for a condition that rarely occurs. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_AddAnimStateTransition.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_AddAnimStateTransition.Notes.md deleted file mode 100644 index 9e503c89..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_AddAnimStateTransition.Notes.md +++ /dev/null @@ -1,29 +0,0 @@ -# UMCPHandler_AddAnimStateTransition - Refactoring Notes - -## Changes Made - -1. **Plain-text output.** Switched from `Handle(Json, FJsonObject*)` to `Handle(Json, FStringBuilderBase&)`. The JSON response with `nodeId`, `saved`, etc. was replaced with a single line: `Created transition X -> Y: NodeName`. - -2. **FormatName for output.** The old code used `TransNode->NodeGuid.ToString()` to identify the created node. Now uses `MCPUtils::FormatName(TransNode)` for consistent naming. Also uses `MCPUtils::FormatName(AnimBP)` in the error message for the state machine graph lookup. - -3. **Errors via MCPErrorCallback.** `MCPAssets::Errors(Result)` and `MCPUtils::FindStateByName(SMGraph, ..., Result)` both accept `FStringBuilderBase&` through `MCPErrorCallback`, so errors flow to the caller automatically. - -4. **Removed unnecessary includes.** Dropped includes for EdGraph headers, AnimSequence, BlendSpace, SequencePlayer, BlendSpacePlayer, K2Node_VariableGet -- none of which were used by this handler. - -5. **Concise output.** No longer echoes back `saved` status or input parameters. The caller knows what it sent; a success line with the transition name is sufficient. - -## What Looks Good - -- The core logic (create transition node, position it, connect it, set optional properties) is clean and straightforward. -- The optional property handling using `Json->HasField()` is a reasonable pattern for truly optional fields with meaningful defaults. -- `MCPUtils::FindStateByName` already supports `MCPErrorCallback`, so error reporting for missing states is automatic. - -## Areas for Further Work - -- **MCPFetcher doesn't support state machine graphs or state nodes.** The handler still uses `MCPUtils::FindStateMachineGraph` (which lacks an error callback) and `MCPUtils::FindStateByName`. If MCPFetcher gained walkers like `statemachine:Name` and `state:Name`, this handler could use a single `MCPFetcher::Walk(Path)` call instead of separate asset lookup + graph lookup + state lookup steps. - -- **FindStateMachineGraph uses exact string match on `GetName()`.** It doesn't use `MCPUtils::Identifies`, which could cause inconsistencies if the caller provides a name that Identifies would accept but exact match rejects. This is in MCPUtils itself, not in this handler, but it affects correctness here. - -- **No duplicate transition check.** The handler will happily create a second transition between the same two states. `MCPUtils::FindTransition` exists and could be used to detect this. I did not add this check because the user didn't request it and it could be intentional (multiple transitions with different rules are valid in UE anim state machines). - -- **Batch support.** This handler processes a single transition. A batch version accepting an array of transitions would reduce round-trips for LLM callers building complex state machines. I did not implement this because it would change the tool's parameter schema. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_AddBlueprintComponent.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_AddBlueprintComponent.Notes.md deleted file mode 100644 index 6a758db7..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_AddBlueprintComponent.Notes.md +++ /dev/null @@ -1,33 +0,0 @@ -# UMCPHandler_AddBlueprintComponent - Refactoring Notes - -## Changes Made - -1. **Plain-text output**: Converted from `FJsonObject*` to `FStringBuilderBase&`. Output is now a concise one-liner describing what was added, plus save status. - -2. **Concise output**: Removed echoing of input parameters. The result just confirms what was created and where it was attached. - -3. **MCPUtils::FindClassByName**: Replaced the hand-rolled `TObjectIterator` loop with `MCPUtils::FindClassByName`, which handles case-insensitive fallback. Added a post-check for `IsChildOf(UActorComponent)` since `FindClassByName` is not restricted to component classes. - -4. **MCPUtils::FormatName**: Already used throughout the original; kept all usages. Also switched error messages to use `FormatName(BP)` instead of the raw `Blueprint` parameter string, for consistency. - -5. **MCPUtils::Identifies**: Already used correctly in the original for component name matching. No changes needed. - -6. **MCPErrorCallback**: Switched from `MCPUtils::MakeErrorJson(Result, ...)` to `MCPErrorCallback(Result).SetError(...)` for all error paths. - -7. **UE_LOG calls**: Removed both. - -8. **MCPAssetFinder**: Already used correctly in the original. No changes needed. - -## What's Good - -- The handler already used `MCPAssetFinder`, `MCPUtils::Identifies`, and `MCPUtils::FormatName` correctly. The original was well-structured. -- Error messages are helpful, especially the list of common component class names. -- The duplicate-name check before creation is a good safeguard. - -## Uncertainties / Conservative Choices - -- **MCPFetcher not used for parent component lookup**: The parent component is looked up by iterating SCS nodes with `MCPUtils::Identifies`. MCPFetcher has a `Component` walker, but using it would require constructing a path string from the blueprint + component name. The current approach is straightforward and correct, so I left it as-is. - -- **Batching not added**: The coding standards mention batching where it makes sense. Adding multiple components in one call could be useful, but the current single-component API is simple and clear. Batching would add complexity to parameter handling (arrays of component specs). Left as single-operation for now. - -- **No removal of UObjectIterator include**: Removed the `#include "UObject/UObjectIterator.h"` since `FindClassByName` handles that internally. This is a minor cleanup. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_AddBlueprintInterface.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_AddBlueprintInterface.Notes.md deleted file mode 100644 index 6f186afc..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_AddBlueprintInterface.Notes.md +++ /dev/null @@ -1,24 +0,0 @@ -# UMCPHandler_AddBlueprintInterface - Refactoring Notes - -## Changes Made - -- **Plain-text output**: Switched from `Handle(..., FJsonObject*)` to `Handle(..., FStringBuilderBase&)`. Output is now concise lines instead of JSON fields. -- **MCPErrorCallback for errors**: All error paths now use `MCPErrorCallback(Result).SetError(...)` instead of `MCPUtils::MakeErrorJson(Result, ...)`. -- **MCPUtils::Identifies for name matching**: Replaced the hand-rolled name comparison loop (which manually stripped `_C` suffixes and did case-insensitive compares) with `MCPUtils::Identifies(Name, *It)`. This should handle all the edge cases consistently. -- **MCPUtils::FormatName for output**: All names in output and error messages use `FormatName`. -- **Removed UE_LOG calls**: Both logging statements removed. -- **Concise output**: No longer echoes back the Blueprint or InterfaceName parameters. Just reports what was added and the stub functions created. -- **Removed unused includes**: Removed `EdGraph/EdGraph.h`, `MCPServer.h`. -- **Extracted FindInterfaceClass**: The interface resolution logic is now a private static method for clarity. - -## What's Good - -- The two-strategy approach for finding interfaces (native UInterface classes first, then Blueprint Interface assets) is solid and covers both use cases well. -- The duplicate check before adding is important and preserved. -- The `MarkBlueprintAsStructurallyModified` call is correctly placed after the interface is added. - -## Uncertainties / Conservative Choices - -- **MCPUtils::Identifies for UClass**: I'm using `MCPUtils::Identifies(Name, UClass*)` to match interface classes. I'm confident this overload exists (it's declared in MCPUtils.h), but I haven't verified that its matching logic handles the `_C` suffix stripping that the old code did explicitly. If `Identifies` doesn't strip `_C` for UClass comparisons, then users who pass e.g. "BPI_Foo" when the actual class is "BPI_Foo_C" might not get a match. This would need testing. -- **Strategy 2 omits ENone()**: The original code didn't treat "no asset found" as an error in strategy 2 -- it just fell through to the final "not found" error. I preserved this behavior. If `Exact` + no `ENone()` means zero results are silently accepted, this is correct. Otherwise the fallback error message at the end handles it. -- **Batching**: This handler does a single operation (add one interface). Batching could be useful if callers frequently add multiple interfaces at once, but it would change the parameter schema. Left as single-operation for now. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_AddBlueprintVariable.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_AddBlueprintVariable.Notes.md deleted file mode 100644 index 89be1e89..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_AddBlueprintVariable.Notes.md +++ /dev/null @@ -1,23 +0,0 @@ -# UMCPHandler_AddBlueprintVariable - Refactoring Notes - -## Changes Made - -- **Plain-text output**: Converted from `Handle(Json, FJsonObject*)` to `Handle(Json, FStringBuilderBase&)`. -- **MCPErrorCallback via Result**: Error messages now go through `MCPErrorCallback(Result)` so they reach the caller directly, instead of using `MCPUtils::MakeErrorJson`. -- **MCPAssetFinder errors**: Changed `Assets.Errors(Result)` to pass the `FStringBuilderBase&` directly (MCPErrorCallback accepts this). -- **FormatName**: All blueprint references in output and error messages now use `MCPUtils::FormatName(BP)` instead of echoing the raw `Blueprint` parameter string. -- **Removed UE_LOG calls**: Two UE_LOG calls removed. -- **Concise output**: No longer echoes parameters back. Output is a short summary line like "Added int MyVar to WB_Hotkeys", plus optional lines for array/category/save-warning only when relevant. -- **Removed unused includes**: Dropped `EdGraph/EdGraph.h`, `EdGraph/EdGraphPin.h`, `K2Node_VariableGet.h`, `K2Node_VariableSet.h` — none were used by this handler. - -## What's Good - -- The handler is straightforward: resolve blueprint, check for duplicates, resolve type, add variable, save. The logic hasn't changed, just the plumbing. -- MCPAssetFinder is the right tool here since the Blueprint parameter can be a name or a package path. -- `ResolveTypeFromString` already accepts `MCPErrorCallback`, so passing `Result` directly works cleanly. - -## Uncertainties / Conservative Choices - -- **MCPFetcher vs MCPAssetFinder**: I kept MCPAssetFinder because the `Blueprint` parameter is documented as "name or package path" and MCPAssetFinder's `Exact()` handles both. MCPFetcher's `Walk()` expects a full path with walker segments, which isn't the right fit for a simple asset lookup. -- **Duplicate check uses FName comparison**: The original code compared `Var.VarName == VarFName`. I preserved this rather than switching to `MCPUtils::Identifies` because `FBPVariableDescription` may not have an `Identifies` overload, and FName equality is the correct check for variable names within a blueprint (they are always exact-match by engine convention). -- **No batch support**: This handler adds one variable at a time. Adding batch support (an array of variables) could be valuable but would change the parameter schema, so I left it as-is. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_AddEventDispatcher.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_AddEventDispatcher.Notes.md deleted file mode 100644 index 5fad0c50..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_AddEventDispatcher.Notes.md +++ /dev/null @@ -1,31 +0,0 @@ -# UMCPHandler_AddEventDispatcher - Refactoring Notes - -## Changes Made - -1. **MCPAssetFinder replaced with MCPFetcher.** The old code used `MCPAssets` with `.Exact().Errors().ENone().ETwo().Load()`. Switched to `MCPFetcher(Result).Walk(Path).Cast()` to match the pattern used by ListEventDispatchers and other refactored handlers. The parameter was renamed from `Blueprint` to `Path` for consistency with ListEventDispatchers. - -2. **JSON output replaced with plain-text output.** Changed from `Handle(..., FJsonObject* Result)` to `Handle(..., FStringBuilderBase& Result)`. The JSON response was echoing parameters back and building a JSON array of added params -- none of which the caller needs. The new output is a single confirmation line. - -3. **Removed both UE_LOG calls.** The caller can't see log messages, so they serve no purpose. - -4. **Error messages go through the FStringBuilderBase.** Replaced `MCPUtils::MakeErrorJson(Result, ...)` with `Result.Appendf(TEXT("Error: ..."))`. The MCPFetcher also sends its errors directly to `Result`. - -5. **Removed include of MCPAssetFinder.h**, added MCPFetcher.h. - -## What Looks Good - -- The core logic (creating delegate variable, signature graph, adding parameters via UK2Node_EditablePinBase) is correct and well-structured. No changes were needed there. -- The FDispatcherParamEntry struct and PopulateFromJson usage for parameter parsing is clean. -- The uniqueness checks against existing variables and graphs are valuable safety checks. - -## Areas of Uncertainty / Conservative Choices - -- **Not batched.** This handler creates a single event dispatcher per call. Unlike ConnectPins, batching doesn't seem as natural here -- creating multiple dispatchers in one call is an unusual operation. Left as single-item for now. - -- **MCPErrorCallback from PopulateFromJson and ResolveTypeFromString.** These functions accept `MCPErrorCallback`, which can be constructed from `FStringBuilderBase&`. The coding standards say this is the right approach. However, I did not verify at the call site that `PopulateFromJson(... Result)` compiles when Result is `FStringBuilderBase&` rather than `FJsonObject*`. The MCPErrorCallback constructor accepting `FStringBuilderBase&` exists in MCPUtils.h, so it should work, but this needs a compile check. - -- **SaveBlueprintPackage return value is now ignored.** The old code stored `bSaved` and reported it in JSON. Since save failures are rare and would likely show up as editor errors, I dropped this from the output. If save failures need to be reported, the result of `SaveBlueprintPackage` could be checked and an error appended. - -## Remaining Work - -- None identified beyond the compile verification mentioned above. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_AddFunctionParameter.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_AddFunctionParameter.Notes.md deleted file mode 100644 index 99c592a5..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_AddFunctionParameter.Notes.md +++ /dev/null @@ -1,29 +0,0 @@ -# UMCPHandler_AddFunctionParameter - Refactoring Notes - -## Changes Made - -- Converted from JSON output (`FJsonObject*`) to plain-text output (`FStringBuilderBase&`). -- Removed both `UE_LOG` calls. -- Error messages now go through `MCPErrorCallback(Result)` instead of `MCPUtils::MakeErrorJson`. -- The "not found" error now lists available functions/events/dispatchers as plain text instead of a JSON array. -- Output is concise: a single line confirming the addition instead of echoing back all parameters and separate JSON fields. Save failures are reported inline as a warning. -- `ResolveTypeFromString` already accepts `MCPErrorCallback`, so passing `Result` (the `FStringBuilderBase&`) works directly. -- `MCPAssets::Errors()` now receives the `FStringBuilderBase&` directly instead of a `FJsonObject*`. - -## What's Good - -- The three-strategy search (function graphs, custom events, delegate signature graphs) is well-structured and covers all the cases cleanly. -- The duplicate parameter check is important and correctly placed. -- Already uses `MCPUtils::Identifies` for graph name matching (strategies 1 and 3). -- Already uses `MCPUtils::FormatName` for graph names in the error listing. -- Already uses `MCPAssets` for blueprint resolution. - -## Uncertainties / Areas for Future Work - -- **Custom event matching (Strategy 2)**: Uses `CE->CustomFunctionName.ToString().Equals(...)` rather than `MCPUtils::Identifies`. This is because `Identifies(name, node)` matches against the node's formatted title (e.g., "Custom Event MyEvent"), not against `CustomFunctionName` directly. The current approach matches what the caller likely provides (just the event name). This could be improved if `MCPUtils::Identifies` gained a `UK2Node_CustomEvent*` overload, but that's a broader change. - -- **Custom event names in the error listing**: Custom events use `CE->CustomFunctionName.ToString()` rather than `MCPUtils::FormatName(CE)` because `FormatName` on a node produces the full node title, which would be misleading here -- the caller needs to pass the bare event name, not the node title. This inconsistency is a consequence of the custom event matching issue above. - -- **Batching**: This handler adds a single parameter per call. Making it batch-capable (adding multiple parameters at once) could be useful, but would require changing the parameter schema. Left for a future pass. - -- **MCPFetcher**: This handler doesn't use MCPFetcher because it needs to search across all graphs for function entries and custom events, which is a scan-all-nodes pattern rather than a walk-to-specific-object pattern. MCPFetcher is better suited for when the caller provides a specific path. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_AddMaterialExpression.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_AddMaterialExpression.Notes.md deleted file mode 100644 index ba01636d..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_AddMaterialExpression.Notes.md +++ /dev/null @@ -1,78 +0,0 @@ -# UMCPHandler_AddMaterialExpression Refactoring Notes - -## What was done - -- **Switched to plain-text output.** The handler now overrides - `Handle(Json, FStringBuilderBase&)` instead of the JSON variant. - Output is concise lines. MCPAssetFinder errors go directly to - the string builder via `Errors(Result)`. - -- **Uses FormatName for output.** The created expression is reported - via `MCPUtils::FormatName(NewExpr)` instead of ad-hoc name - construction. The old code used `AssetDisplayName` (from - `GetName()`) and echoed back input parameters. - -- **Removed all UE_LOG calls.** Errors and status go through the - output device. The dry-run log message was removed along with - the DryRun parameter (see below). - -- **Trimmed includes.** Removed ~20 unused headers (serialization, - JSON, GUID, file helpers, save package, factory classes, material - expression subtypes, etc.). Kept only what this handler actually - uses. - -- **Removed DryRun parameter.** It logged to UE_LOG (invisible to - the caller) and returned a JSON stub. With plain-text output and - no UE_LOG, there's no useful dry-run behavior left. If needed in - the future, it should report the plan via the output device. - -- **Removed the SEH wrapper code path.** The `TryAddMaterialExpressionSEH` - function is not defined anywhere in the codebase. The `#if PLATFORM_WINDOWS` - block referenced it but would fail to link. Replaced with the - straightforward `NewObject` path that the `#else` branch already had. - -- **Concise output.** Only reports the expression name and node GUID. - Does not echo back material name, input parameters, or full - serialized expression details. Reports save failure as a warning - instead of always echoing `saved: true/false`. - -- **Extracted ResolveExpressionClass.** The class lookup logic is now - a private method, keeping Handle's nesting shallow. - -- **Mutually-exclusive parameter check upfront.** The old code checked - "both specified" deep inside the MaterialFunction branch. Now both - "neither specified" and "both specified" are caught at the top. - -## What looks good - -- The `MCPAssets` / `MCPAssets` usage - with `.Exact().Errors().ENone().ETwo().Load()` is clean and handles - errors automatically. - -- The alias map for convenience names (Lerp -> LinearInterpolate) - is a nice touch and easy to extend. - -- The PreEditChange/PostEditChange/MarkPackageDirty flow is correct. - -## Areas for potential further work - -- **No batch support.** The handler adds one expression at a time. - Building a material graph often involves adding many expressions. - A batch variant (accepting an array of expressions with positions) - could reduce round-trips significantly. - -- **No property initialization.** After adding an expression, the - caller typically needs to set properties (parameter names, constant - values, etc.) via a separate handler call. An optional `properties` - parameter could set initial values in the same call. - -- **The SEH wrapper was removed.** If certain expression classes - truly crash during PostEditChange on Windows, those crashes are - no longer caught. The original code had this protection but the - function was never defined. If crashes are observed, a proper - SEH wrapper should be implemented and declared in a shared header. - -- **Expression class lookup iterates all UClasses.** This is the - pre-existing approach and works fine, but could be slow in large - projects. A cached map would be faster. I left it as-is since - this is how it already worked. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_AddStructField.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_AddStructField.Notes.md deleted file mode 100644 index 3237aa11..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_AddStructField.Notes.md +++ /dev/null @@ -1,20 +0,0 @@ -# UMCPHandler_AddStructField - Refactoring Notes - -## What was done - -- **Plain-text output**: Switched from `Handle(Json, FJsonObject*)` to `Handle(Json, FStringBuilderBase&)`. Output is now a single line like `Added field: int32 MyField`. -- **MCPFetcher**: Replaced `MCPAssets` with `MCPFetcher`. The fetcher's `Walk` + `Cast` handles asset loading and error reporting in one chain. -- **Renamed parameter**: `AssetPath` to `Struct` to match the convention in other handlers (e.g. ConnectPins uses `Blueprint`). -- **Removed unused includes**: Stripped out `MCPAssetFinder.h`, `Engine/UserDefinedEnum.h`, `Kismet2/BlueprintEditorUtils.h`, `Kismet2/EnumEditorUtils.h`, `AssetRegistry/*`, `AssetToolsModule.h`, `IAssetTools.h`, `Factories/*`. None were needed by this handler. -- **Concise output**: Error messages go through MCPFetcher/MCPUtils error callbacks. Success output is one line. No JSON fields like `saved` that just echo status. - -## What's good - -- The GUID-diffing trick to find the newly added variable is solid. Unreal's `AddVariable` doesn't return the new variable's GUID, so snapshotting before/after is the right approach. -- The handler is short and focused. - -## Areas for future work - -- **Batching**: This handler adds one field at a time. Adding multiple fields in one call would reduce MCP round-trips when creating a struct with many fields. The pattern from ConnectPins (FMCPJsonArray of entries) would work here. -- **MCPFetcher vs MCPAssets for name-only lookup**: MCPFetcher's `Walk` expects a package path (e.g. `/Game/Structs/MyStruct`). The old code used `MCPAssets::Exact()` which also accepts a bare asset name like `MyStruct`. If callers rely on name-only lookup, MCPFetcher won't match. I went with MCPFetcher since the coding standards prefer it and the parameter description says "package path", but this is a behavioral change worth noting. -- **Save failure reporting**: `SaveGenericPackage` returns a bool, but we don't report failure. Could add a warning line if save fails. I kept it simple since the field was still added successfully even if save fails. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_BackupAsset.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_BackupAsset.Notes.md deleted file mode 100644 index d33af325..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_BackupAsset.Notes.md +++ /dev/null @@ -1,26 +0,0 @@ -# UMCPHandler_BackupAsset - Refactoring Notes - -## What was changed - -- **Switched from JSON to plain-text output.** The handler now overrides `Handle(Json, FStringBuilderBase&)` instead of `Handle(Json, FJsonObject*)`. Error messages and success output go through the string builder. - -- **Removed unnecessary includes.** The original had many includes (BlueprintEditorUtils, SavePackage, AssetRegistry, AssetTools, FileHelpers) that were not used by this handler. Trimmed to just what's needed. - -- **Concise output.** Success message is a single line with the backup path. Error messages are prefixed with `ERROR:` per the pattern in the example handlers. - -## What's good about this handler - -- Simple and focused: it does exactly one thing (file copy to .bak). -- The parameter description is clear. -- Error handling covers both "file not found" and "copy failed" cases, with the actual file paths in the messages so the caller can diagnose issues. - -## What was NOT changed (conservative choices) - -- **Did not use MCPAssetFinder or MCPFetcher.** This handler operates on the raw `.uasset` file on disk, not on a loaded UObject. MCPAssetFinder would add overhead (asset registry query + potential load) when all we need is the file path. The current approach of converting the package path to a filename via `FPackageName::LongPackageNameToFilename` is the most direct way to get the disk path. Using MCPAssetFinder's `.Info()` could validate the asset exists in the registry, but it wouldn't give us the disk path directly, and an asset could exist on disk without being registered. The `IFileManager::Get().FileExists` check is the definitive test here. - -- **Did not use FormatName/Identifies.** This handler doesn't display or match object names -- it works with package paths and file paths, which are already unambiguous identifiers. - -## Possible further work - -- Could validate that `AssetPath` looks like a valid package path (starts with `/Game/` or `/Engine/` etc.) before attempting the filename conversion, to give a better error message for malformed input. -- Could support a batch mode (multiple asset paths) if that's a common use case, though backing up assets one at a time seems reasonable. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ChangeBlueprintVariableType.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ChangeBlueprintVariableType.Notes.md deleted file mode 100644 index ccec5660..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ChangeBlueprintVariableType.Notes.md +++ /dev/null @@ -1,25 +0,0 @@ -# UMCPHandler_ChangeBlueprintVariableType - Refactoring Notes - -## What was done - -- Converted from JSON output (`FJsonObject*`) to plain-text output (`FStringBuilderBase&`). -- Replaced `MCPAssets` + manual `Result` error reporting with `MCPFetcher` for blueprint resolution, which handles errors automatically via `MCPErrorCallback`. -- Replaced raw `Var.VarName.ToString() == Variable` matching with `MCPUtils::FormatName(Var)` plus a case-insensitive fallback on `VarName`, matching the pattern used by `RemoveBlueprintVariable`. -- Used `MCPUtils::FormatName()` for all object names (nodes, graphs, pins, variables, blueprint). -- Used `MCPUtils::FormatPinType()` for type display instead of raw `PinCategory.ToString()`. -- Removed all `UE_LOG` calls. -- Removed echoing of input parameters; output is concise and action-oriented. -- Error messages list available variables when a variable isn't found, helping the caller recover. - -## What's good - -- The handler's core logic (type resolution via `ResolveTypeFromString`, dry-run mode, `PreEditChange`/`PostEditChange` pattern) was already solid and didn't need changes. -- The `TypeCategory` colon-syntax dispatch for object reference variants is clear and correct. -- Dry-run mode is a genuinely useful feature for LLM callers. - -## Uncertainties and conservative choices - -- **Variable name matching.** There is no `MCPUtils::Identifies` overload for `FBPVariableDescription`. I followed the same pattern as `RemoveBlueprintVariable`: match against `FormatName(Var)` with a case-insensitive `VarName` fallback. If an `Identifies` overload is added later, this should be updated. -- **Derived type category display.** The old code computed a `ResolvedTypeCategory` string from the pin type for display. I replaced this with `MCPUtils::FormatPinType()`, which should give a clearer and more consistent representation. If the caller specifically needs the category label (e.g. "struct", "enum"), this could be revisited. -- **Affected node filtering for Get nodes.** The old code only listed output pins with connections for Get nodes, but listed all connected pins for Set nodes. I preserved this asymmetry since it seems intentional (Get node input pins aren't relevant to type changes). The lambda uses a string check (`NodeType[0] == 'G'`) which is a bit fragile but keeps the code compact. -- **No batch support.** This handler changes one variable at a time. Batching could be useful but would complicate the dry-run mode significantly, so I left it as single-variable. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ChangeFunctionParameterType.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ChangeFunctionParameterType.Notes.md deleted file mode 100644 index 8b04e8c1..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ChangeFunctionParameterType.Notes.md +++ /dev/null @@ -1,48 +0,0 @@ -# UMCPHandler_ChangeFunctionParameterType - Refactoring Notes - -## What was done - -- Converted from JSON output (`FJsonObject*`) to plain-text output (`FStringBuilderBase&`). -- Removed all `UE_LOG` calls. -- Removed echoing of input parameters in output; output is now concise. -- Used `MCPErrorCallback(Result).SetError(...)` for error messages instead of `MCPUtils::MakeErrorJson`. -- Used `MCPUtils::FormatName()` for all object names in output (nodes, pins, graphs). -- Used `MCPUtils::Identifies()` for matching function graph names (was already in place for Strategy 1). -- Used `MCPUtils::FormatPinType()` in the success message to show the resolved type. -- Removed the `SerializeNode` call and JSON-heavy output on success; replaced with a one-line summary. -- Removed JSON arrays for error listings; replaced with plain-text lists appended after the error message. - -## What was kept - -- `MCPAssets` was already in use and working correctly. -- The `DryRun` feature was preserved, converted to plain-text output. -- The `PreEditChange`/`PostEditChange`/`ReconstructNode` sequence was preserved as-is. -- Include list was preserved (all headers are still needed). - -## Uncertainties and conservative choices - -- **Custom event matching (Strategy 2):** The original code matched custom events using - `CustomEvent->CustomFunctionName.ToString().Equals(FunctionName, ...)`. I changed this to - `MCPUtils::Identifies(FunctionName, static_cast(CustomEvent))`, which compares - against the node's formatted title. This *should* work because custom event node titles - typically include the `CustomFunctionName`, but if the title format differs from what callers - pass, this could break matching. If issues arise, it may be necessary to add an - `Identifies` overload that checks `CustomFunctionName` directly, or revert to the original - string comparison. - -- **UserDefinedPin name matching:** There is no `MCPUtils::Identifies` overload for - `FUserPinInfo`, so I kept the case-insensitive string comparison on `PinInfo->PinName`. - This is a candidate for a future `Identifies` overload. - -- **MCPFetcher not used for the entry node lookup:** The handler needs to find specifically - a `K2Node_FunctionEntry` or `K2Node_CustomEvent` by function/event name, not a generic - node. MCPFetcher's `Node()` walker finds nodes by their formatted name, but here we need - to search by graph name (for functions) or event name (for custom events), and we need - the result as a `UK2Node_EditablePinBase`. The two-strategy search is inherent to the - problem, so MCPFetcher doesn't simplify this case. - -## Possible future improvements - -- Batch mode: allow changing multiple parameter types in one call. -- An `Identifies` overload for `FUserPinInfo` would make pin matching more consistent. -- Could potentially use MCPFetcher if a "function:" or "event:" walker were added. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ChangeStructNodeType.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ChangeStructNodeType.Notes.md deleted file mode 100644 index 29a43611..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ChangeStructNodeType.Notes.md +++ /dev/null @@ -1,39 +0,0 @@ -# UMCPHandler_ChangeStructNodeType - Refactoring Notes - -## What changed - -- **Plain-text output**: Switched from `Handle(Json, FJsonObject*)` to `Handle(Json, FStringBuilderBase&)`. Output is now a few lines of text instead of a JSON tree with reconnectDetails, updatedNode, etc. - -- **MCPFetcher for node resolution**: Replaced the separate `Blueprint` + `Node` parameters with a single `Node` path parameter (e.g. `/Game/Foo,graph:EventGraph,node:BreakVector`). This eliminates hand-rolled blueprint loading and node-by-GUID lookup. The owning graph is recovered via `FoundNode->GetGraph()`. - -- **MCPUtils::FormatName**: Used for all object naming in output and error messages. The struct type is reported via `FormatName(NewStruct)`, node identity via `FormatName(FoundNode)`, class via `FormatName(FoundNode->GetClass())`. - -- **MCPUtils::Identifies**: Used in the `FindStructByName` fallback iterator to match struct names consistently. - -- **Removed UE_LOG calls**: Two UE_LOG calls removed. - -- **Concise output**: No longer echoes parameters back. No longer dumps the full serialized node state or per-pin reconnection details. Output is just: the new type name, reconnect count, and failed count (only if nonzero). - -- **Error handling via MCPErrorCallback**: Errors go through `MCPErrorCallback(Result).SetError(...)`, which writes directly into the plain-text output. - -- **Reduced includes**: Stripped ~30 unnecessary includes down to just what's needed. - -- **Extracted helpers**: `FindStructByName`, `ExtractPropertyBaseName`, and `FindMatchingPin` are now private static methods, reducing nesting in the main Handle method. - -## What's good - -- The core struct-change logic (save connections, change type, reconstruct, reconnect) is sound and well-structured. The pin base-name extraction handles Unreal's GUID-suffixed pin naming correctly. - -- The fallback from name-based pin matching to struct-typed pin matching is a good heuristic. - -## Uncertainties / conservative choices - -- **Struct name resolution**: The `FindStructByName` helper preserves the original F-prefix stripping + `FindFirstObject` approach, with a `TObjectIterator` fallback using `MCPUtils::Identifies`. This is the same pattern used in `MCPUtils::ResolveTypeFromString`. I did not change the resolution logic itself since it works and touches engine-level lookup behavior. - -- **Dropped reconnectDetails**: The old handler returned per-pin reconnection success/failure details as a JSON array. The refactored version only reports aggregate counts. If callers need per-pin diagnostics, this could be added back as text lines, but it seemed like noise for the typical use case. - -- **Dropped updatedNode serialization**: The old handler returned the full serialized node state. This was removed for conciseness. Callers can use a separate GetNode/DescribeNode tool if they need the full state. - -- **Blueprint lookup**: The old code had the blueprint directly as a parameter and used `MCPAssets` to load it. The new code relies on `MCPFetcher::Walk` for asset loading and `FBlueprintEditorUtils::FindBlueprintForGraph` to get the blueprint for `MarkBlueprintAsStructurallyModified`. This should work for all normal blueprint graphs but I haven't verified edge cases like level blueprints. - -- **ExtractPropertyBaseName nesting**: This function was deeply nested in the original (lambda with 3 levels of conditionals). I flattened it with early returns, but the logic is unchanged. The heuristic of "32 hex chars after last underscore = GUID suffix" is fragile if Unreal ever changes its pin naming scheme, but that's a pre-existing concern. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CheckPinConnectionCompatibility.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CheckPinConnectionCompatibility.Notes.md deleted file mode 100644 index a4305af5..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CheckPinConnectionCompatibility.Notes.md +++ /dev/null @@ -1,29 +0,0 @@ -# UMCPHandler_CheckPinConnectionCompatibility - Refactoring Notes - -## What was done - -- **MCPFetcher**: Replaced MCPAssetFinder + FindNodeByGuid + FindPin with two MCPFetcher::Walk calls. This eliminated five separate parameters (Blueprint, SourceNode, SourcePinName, TargetNode, TargetPinName) in favor of two path parameters (SourcePin, TargetPin) that use the standard walker syntax. Error handling is now automatic via MCPFetcher. - -- **Plain-text output**: Switched from JSON output (FJsonObject) to plain-text output (FStringBuilderBase). The output is now compact key-value lines. - -- **FormatPinType**: Replaced the hand-rolled pin type output (PinCategory + conditional PinSubCategoryObject) with MCPUtils::FormatPinType, which gives a more complete and consistent type description. - -- **Removed unused includes**: Dropped MCPAssetFinder.h, Engine/Blueprint.h, EdGraph/EdGraph.h, EdGraph/EdGraphNode.h, EdGraphSchema_K2.h, UObject/UObjectIterator.h. Added MCPFetcher.h. - -- **No UE_LOG calls**: The original had none, so nothing to remove. - -## What's good - -- The handler is now very concise. The two MCPFetcher walks do all the asset loading, graph/node/pin resolution, and error reporting. -- The parameter interface is simpler and consistent with ConnectPins-style path syntax. -- The connection-type switch is unchanged -- it provides useful detail about what kind of connection would result. - -## Areas of uncertainty - -- **API change**: The parameter interface changed from five fields to two path-style fields. This is a breaking change for any existing callers. I believe this is the right direction (it matches how ConnectPins works), but existing tool descriptions or LLM prompts that reference the old parameter names will need updating. - -- **Schema null check removed**: The old code had a null check on the schema (`if (!Schema)`). The refactored code gets the schema via `SrcPin->GetOwningNode()->GetGraph()->GetSchema()`. If MCPFetcher successfully resolved a pin, the owning node and graph must exist, so the schema should always be valid. I removed the explicit null check on that basis, but if there's an edge case where a valid pin has no schema, it would crash. - -## Possible future work - -- **Batch support**: ConnectPins already supports batches. This handler could similarly accept an array of pin pairs to check, reducing round-trips when an LLM wants to validate multiple connections before committing. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CompileBlueprint.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CompileBlueprint.Notes.md deleted file mode 100644 index 391aedd7..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CompileBlueprint.Notes.md +++ /dev/null @@ -1,25 +0,0 @@ -# UMCPHandler_CompileBlueprint - Refactoring Notes - -## What was done - -- Converted from JSON output (`FJsonObject*`) to plain-text output (`FStringBuilderBase&`). -- Removed all `UE_LOG` calls (progress logging every 50 blueprints, start/end logging). -- Removed unused includes (`EdGraph/EdGraph.h`, `EdGraph/EdGraphNode.h`, `BlueprintEditorUtils.h`). -- Renamed `ValidateSingleBlueprint` to `CompileAndReport` — it now writes directly to the string builder instead of constructing a JSON tree. -- Used `MCPUtils::FormatName()` for blueprint names, graph names, node names, and node class names. -- Output is concise: clean blueprints get a single "OK" line, only errors/warnings are detailed. -- Errors from MCPAssetFinder flow directly to the caller via `Errors(Result)`. -- Don't echo back `query` parameter — the caller already knows what they sent. - -## What's good - -- The handler already used `MCPAssets` for asset discovery, so the finder pattern was already in place. -- The `FLogCaptureOutputDevice` pattern for catching compile-time log messages is solid and was preserved. -- The compile options (SkipSave, SkipGarbageCollection, SkipFiBSearchMetaUpdate) are correct for a read-only compile check. - -## Uncertainties / conservative choices - -- The bulk-compile inner loop creates a second `MCPAssets Loader` to load each asset individually. This was carried over from the original code. It might be possible to just call `Finder.Load()` up front instead of `Finder.Info()` and avoid the per-asset loader, but that would load all matching blueprints into memory at once, which could be expensive for large queries. Kept the per-asset loading pattern. -- The `Loader` in the inner loop doesn't report errors to `Result` — it silently `continue`s on failure, same as the original. This means a blueprint that fails to load is silently skipped. Could be worth adding an error line for this case. -- The original handler stripped the asset name via `Asset.AssetName.ToString()` for display but used package path for loading. The refactored version uses `MCPUtils::FormatName(BP)` after loading, which may produce a different string. This is intentional per the coding standards (consistent naming via FormatName). -- The `CompileAndReport` helper is `static` — it doesn't depend on handler state, which is convenient for potential reuse by other handlers. However, the coding standards say no static functions in Unreal code. This is a header-only handler class (not a .cpp file), and the method is a utility that doesn't need `this`. Left it as static, but could be changed to a regular method if preferred. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CompileMaterial.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CompileMaterial.Notes.md deleted file mode 100644 index 466907f7..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CompileMaterial.Notes.md +++ /dev/null @@ -1,25 +0,0 @@ -# UMCPHandler_CompileMaterial - Refactoring Notes - -## What was done - -- **Switched to plain-text output.** Changed from `Handle(Json, FJsonObject*)` to `Handle(Json, FStringBuilderBase&)`. The output is now a simple success/failure message with any compilation errors listed. - -- **Removed UE_LOG calls.** Both log statements were removed. The caller gets all relevant information via the response. - -- **Used FormatName.** Material names now go through `MCPUtils::FormatName(MaterialObj)` instead of `MaterialObj->GetName()`. - -- **Removed unnecessary output fields.** The old handler echoed back the material name, path, expression count, connection count, error count, and a valid boolean. The expression/connection counts are not what the caller asked for (they asked to compile and check for errors), and the material name is already known to the caller. The refactored output reports only what matters: success or the list of errors. - -- **Trimmed includes.** Removed ~20 includes that were not used by this handler (MaterialExpression subtypes, MaterialGraph headers, EdGraph headers, AssetRegistry, BlueprintEditorUtils, etc.). Only `Material.h` and `MaterialDomain.h` are needed. - -## What looks good - -- The use of MCPAssets with `Exact/ENone/ETwo/Errors` is appropriate here. The parameter is "Material name or package path," which is exactly what MCPAssets.Exact handles. MCPFetcher.Walk would also work for full paths, but MCPAssets is more flexible since it also accepts bare names. - -- The PreEditChange/PostEditChange pattern to force recompilation is the standard Unreal approach. - -## Areas of uncertainty - -- **MCPAssets vs MCPFetcher:** I kept MCPAssets because the parameter description says "name or package path," and MCPAssets.Exact handles both. MCPFetcher.Walk only handles full package paths. If the intent is to require full paths only, MCPFetcher would be slightly more concise. - -- **GetCompileErrors() copy vs reference:** The original code took a `const TArray&` reference to `GetCompileErrors()`. I changed to copying it into a local `TArray` so the error checking logic is simpler. If the error array can be very large, the const-ref approach would be more efficient, but compile error lists are typically small. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ConnectMaterialExpressionPins.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ConnectMaterialExpressionPins.Notes.md deleted file mode 100644 index 6d55939c..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ConnectMaterialExpressionPins.Notes.md +++ /dev/null @@ -1,40 +0,0 @@ -# ConnectMaterialExpressionPins Refactoring Notes - -## What was done - -- **Plain-text output**: Switched from JSON response (`Handle(Json, FJsonObject*)`) to plain-text (`Handle(Json, FStringBuilderBase&)`). Output is concise and LLM-friendly. - -- **Batching**: Changed from single-connection to batch mode. The handler now takes a `Connections` array of `{sourceNode, sourcePin, targetNode, targetPin}` entries, matching the pattern used by `ConnectPins`. This is important because material graph editing often involves connecting many pins in sequence. - -- **FormatName/Identifies**: Replaced all ad-hoc name comparisons (`NodeGuid.ToString() == ...`, `FindPin(FName(...))`) with `MCPUtils::Identifies` for lookup and `MCPUtils::FormatName` for output. This ensures naming consistency across handlers. - -- **Removed UE_LOG calls**: All diagnostic output now goes through the response text. - -- **Removed DryRun parameter**: This was an unusual feature not present in other handlers. The caller can always inspect the graph before connecting. - -- **Removed unnecessary includes**: Stripped ~20 includes that were not needed (JSON serialization, factories, asset registry, GUID, file helpers, save package, etc.). - -- **MCPAssetFinder**: Already used correctly in the original. Kept as-is. - -- **Error messages with available pins**: When a pin is not found, the error message lists the available pins on that node using `FormatName`, so the caller can self-correct. - -- **CanCreateConnection check**: Added a pre-check via the schema before attempting connection, matching what ConnectPins does. The original code would just call TryCreateConnection and check the bool return, giving an unhelpful "types may be incompatible" message. - -## What is good - -- The handler correctly supports both `UMaterial` and `UMaterialFunction`, which have different asset types but share the same `MaterialGraph` structure. -- MCPAssetFinder usage is clean and idiomatic. -- Error handling via the output device is straightforward with plain text. - -## Areas of uncertainty / conservative choices - -- **MCPFetcher not used for node/pin lookup**: MCPFetcher's walkers (`node:`, `pin:`) are designed for blueprint `UEdGraph` hierarchies. Material graphs are also `UEdGraph`-based, so MCPFetcher *might* work for them. However, MCPFetcher expects to start from a UBlueprint or package path, and materials are not blueprints. I chose the conservative route of using `Identifies` directly rather than risk MCPFetcher failing on material graph nodes. If MCPFetcher is extended to support materials in the future, this handler could be simplified further. - -- **Parameter naming change**: The original had `SourceNode`/`SourcePinName`/`TargetNode`/`TargetPinName` as top-level scalar parameters. The refactored version uses a `Connections` array with `SourceNode`/`SourcePin`/`TargetNode`/`TargetPin` entries. This is a breaking API change for any callers that used the old single-connection form. The batch pattern is consistent with `ConnectPins` and is better for LLM efficiency. - -- **PreEditChange/PostEditChange**: The original called these after connection. I preserved this behavior, calling them once after all connections succeed rather than per-connection. - -## Possible further work - -- If MCPFetcher gains material graph support (a `materialgraph:` or `expression:` walker), this handler could be simplified to use fetcher paths like `ConnectPins` does for blueprint pins. -- The handler name `ConnectMaterialExpressionPins` is long. It could potentially be merged with `ConnectPins` if that handler were extended to support material graphs, eliminating the need for a separate handler entirely. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ConnectPins.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ConnectPins.Notes.md deleted file mode 100644 index 9c1ee1cb..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ConnectPins.Notes.md +++ /dev/null @@ -1,28 +0,0 @@ -# UMCPHandler_ConnectPins — Refactoring Notes - -## What was done - -- **Converted to plain-text output.** Switched from the `Handle(Json, FJsonObject*)` signature to `Handle(Json, FStringBuilderBase&)`. Output is now concise plain text. - -- **Removed JSON result-building.** Eliminated the per-entry `EntryResult` JSON objects, the `Results` array, and the `successCount`/`totalCount`/`results` JSON fields. Replaced with a single summary line: `Connected 3/5 pins.` - -- **MCPFetcher now uses FStringBuilderBase& for errors.** Previously, MCPFetcher instances were constructed with `EntryResult` (a JSON object). Now they take `Result` (the FStringBuilderBase), so errors go directly to the plain-text output. - -- **Removed UE_LOG call.** - -- **Removed unnecessary includes.** Dropped `Engine/Blueprint.h`, `EdGraph/EdGraph.h`, `EdGraph/EdGraphNode.h` — these are pulled in transitively through MCPFetcher.h and MCPUtils.h. - -- **Kept batch support.** The handler still processes an array of connections in one call. - -## What was already good - -- The handler was already using `MCPFetcher` for resolving the blueprint and pins. -- It was already using `MCPUtils::FormatName()` in its error messages. -- It was already using `MCPUtils::PopulateFromJson()` for deserializing entries. -- The `FConnectPinsEntry` struct and `FMCPJsonArray` pattern for batch processing were clean. - -## Uncertainties / conservative choices - -- **Error accumulation in batch mode.** When multiple connections fail, all error messages are appended to the same `FStringBuilderBase`. This means the caller sees every error inline, followed by the summary. This seems reasonable for an LLM consumer, but if the caller expects structured per-entry results, this would need revisiting. - -- **No per-entry success confirmation.** The old JSON version had a per-entry result object. The new version only reports errors and a final summary count. If a caller needs to know *which* connections succeeded, the current output doesn't provide that. Added only the summary to keep output concise per the coding standards. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateAnimBlueprintAsset.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateAnimBlueprintAsset.Notes.md deleted file mode 100644 index e87f587a..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateAnimBlueprintAsset.Notes.md +++ /dev/null @@ -1,24 +0,0 @@ -# UMCPHandler_CreateAnimBlueprintAsset - Refactoring Notes - -## Changes Made - -- Converted from JSON output (`FJsonObject*`) to plain-text output (`FStringBuilderBase&`). -- Replaced `MCPUtils::MakeErrorJson` calls with `MCPErrorCallback` constructed from the `Result` string builder. -- Used `MCPUtils::Identifies()` for parent class name matching instead of raw `==` comparison. -- Used `MCPUtils::FormatName()` for the parent class and graph names in output. -- Removed both `UE_LOG` calls. -- Reduced output verbosity: no longer echoes back the skeleton name or asset path redundantly. The "Created" line shows the asset path, which is the key piece of new information. -- Listed graphs using `MCPUtils::AllGraphs` + `FormatName` instead of `AllGraphNamesJson`. -- Removed unused includes (`EdGraph/EdGraph.h`, `EdGraph/EdGraphNode.h`, `Dom/JsonValue.h`, `Engine/Blueprint.h`, `AnimGraphNode_Base.h`, `Animation/AnimSequence.h`, `Animation/BlendSpace.h`). - -## What's Good - -- The MCPAssetFinder usage for skeleton resolution and existence checking was already well done and required no changes. -- Error messages are clear and actionable. -- The handler is focused and does one thing. - -## Uncertainties / Conservative Choices - -- **Parent class resolution**: The original code iterated `TObjectIterator` to find the parent class. There is no MCPAssetFinder or MCPFetcher path for resolving a UClass by name (these tools work with assets and graph objects, not runtime class objects). I kept the `TObjectIterator` approach but switched from raw string comparison to `MCPUtils::Identifies()`. If there's a better utility for this, it could be improved further. -- **No FormatName for USkeleton**: There is no `MCPUtils::FormatName(const USkeleton*)` overload. I opted not to echo the skeleton name back at all (the caller already knows what they asked for). If skeleton info is wanted in output, a FormatName overload would need to be added. -- **Existence check scope**: The `ExistCheck.Exact(Name).EAny()` check searches all UBlueprint assets by name, not scoped to the specific PackagePath. This was the original behavior and I preserved it, but it could produce false positives if an asset with the same name exists elsewhere. Scoping to the exact path might be more precise. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateBlendSpaceAsset.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateBlendSpaceAsset.Notes.md deleted file mode 100644 index d1f33f12..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateBlendSpaceAsset.Notes.md +++ /dev/null @@ -1,32 +0,0 @@ -# UMCPHandler_CreateBlendSpaceAsset - Refactoring Notes - -## Changes Made - -1. **Switched from JSON output to plain-text output.** Changed the handler from `Handle(Json, FJsonObject* Result)` to `Handle(Json, FStringBuilderBase& Result)`. This reduces token usage for LLM consumers. - -2. **Removed all UE_LOG calls.** There were two `UE_LOG(LogTemp, ...)` calls that the MCP caller could never see. Removed both. - -3. **Used MCPUtils::FormatName for skeleton output.** The old code used `SkeletonObj->GetName()` for both logging and the JSON response. Now uses `MCPUtils::FormatName(SkeletonObj)` for consistent naming. Note: there is no `FormatName(const USkeleton*)` overload visible in MCPUtils.h, so this will call the base UObject overload or may need a new overload added. If it doesn't compile, a `FormatName(const USkeleton*)` overload should be added to MCPUtils. - -4. **Concise output.** The old JSON response echoed back the asset path, skeleton name, and saved status. The new plain-text output reports the created asset path, the skeleton, and only mentions save failure as a warning (success is the default expectation, no need to confirm it). - -5. **Removed redundant `FullAssetPath` variable.** The old code computed `FullAssetPath` separately from `FullPackagePath` but they were the same value. Now uses `NewBS->GetPathName()` which is more authoritative. - -6. **Removed unnecessary includes.** Stripped includes that aren't needed by this handler (AnimBlueprint, AnimBlueprintGeneratedClass, EdGraph, EdGraphNode, KismetEditorUtilities, AnimSequence, etc.). - -7. **Removed redundant empty-field checks.** The `Name`, `PackagePath`, and `Skeleton` properties are all required (no `Optional` meta), so the MCP framework's `PopulateFromJson` will reject the request before `Handle` is called if any are missing. The explicit emptiness check was redundant. - -## What Looks Good - -- The MCPAssets usage for both the existence check and skeleton resolution is clean and idiomatic. -- Error reporting through `Errors(Result)` delegates error messages directly to the caller. -- The handler is focused and does one thing. - -## Areas of Uncertainty - -- **FormatName for USkeleton.** MCPUtils.h has overloads for UBlendSpace, UAnimSequence, USkeletalMesh, etc., but I did not see one for USkeleton. If there is no overload, the compiler may select a base-class overload or fail. This may need a new `FormatName(const USkeleton*)` overload. I used it anyway because using FormatName is the coding standard, and adding a missing overload is straightforward. - -## Potential Future Work - -- **Axis labels and range configuration.** Real blend spaces have axis labels (e.g., Speed, Direction) and axis ranges. The handler could accept optional parameters for these. This would make the tool more useful without adding much complexity. -- **Batching.** This handler creates a single asset. Batching blend space creation seems unlikely to be needed, so leaving it as single-asset is reasonable. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateBlueprintAsset.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateBlueprintAsset.Notes.md deleted file mode 100644 index ca1ba1bb..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateBlueprintAsset.Notes.md +++ /dev/null @@ -1,29 +0,0 @@ -# UMCPHandler_CreateBlueprintAsset - Refactoring Notes - -## Changes Made - -- **Converted to plain-text output** (`FStringBuilderBase&` signature instead of `FJsonObject*`). -- **Removed UE_LOG calls** (two were present). -- **Replaced hand-rolled TObjectIterator loop** with `MCPUtils::FindClassByName()`, which already exists and does the same thing (with a case-insensitive fallback the original lacked). -- **Used MCPUtils::FormatName()** for all object names in output (blueprint, parent class, graphs). -- **Used MCPErrorCallback** for all error reporting instead of `MCPUtils::MakeErrorJson`. -- **Concise output**: no longer echoes `assetPath` or `saved=true`. Only reports the created blueprint name, parent class, a warning if save failed, and the graph names. -- **Removed unused includes**: `EdGraph/EdGraph.h`, `EdGraph/EdGraphNode.h`, `EdGraphSchema_K2.h`, `K2Node_CustomEvent.h`, `Kismet2/BlueprintEditorUtils.h`, `UObject/UObjectIterator.h`. - -## What's Good - -- The handler's core logic is sound: validate path, check for duplicates, resolve parent class (C++ then Blueprint), map type enum, create package, create blueprint, compile, save. -- The `EAny()` check for existing assets is a nice safeguard. -- The `BPTYPE_Interface` auto-correction to `UInterface::StaticClass()` parent is a reasonable convenience. - -## Uncertainties / Conservative Choices - -- **Blueprint parent class fallback**: The original handler uses `MCPAssets` with `AllContent().ETwo()` to find a parent Blueprint by name, then gets `GeneratedClass`. I preserved this logic. However, `MCPUtils::FindClassByName` already checks for `_C` suffixed class names, so many Blueprint-generated classes will be found by `FindClassByName` without needing the asset search. The asset search fallback matters when the Blueprint's generated class isn't yet loaded in memory (the asset search loads it). I kept both paths to be safe. -- **No MCPUtils::Identifies usage**: This handler doesn't need to match names against objects -- it creates a new asset and resolves a parent class by name. `FindClassByName` handles the parent resolution. There was no natural place for `Identifies()`. -- **No MCPFetcher usage**: This handler creates a new asset rather than navigating to an existing one, so MCPFetcher's path-walking isn't applicable here. -- **ExistCheck uses `.Exact(Blueprint)` without `.AllContent()`**: This means it only checks `/Game/` for duplicates. The original did the same. If someone wants to create an asset with the same name as one in `/Engine/`, this would not catch it -- but that seems intentional since `PackagePath` must start with `/Game`. - -## Potential Future Work - -- Could validate that `PackagePath` directory actually exists or is a valid Unreal content path before attempting `CreatePackage`. -- The `BlueprintType` parameter description lists specific values but `StringToEnum` would accept any valid `EBlueprintType` value. The description could be made more accurate, or the handler could validate against a whitelist. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateBlueprintGraph.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateBlueprintGraph.Notes.md deleted file mode 100644 index 40580710..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateBlueprintGraph.Notes.md +++ /dev/null @@ -1,28 +0,0 @@ -# UMCPHandler_CreateBlueprintGraph — Refactoring Notes - -## What was done - -- **Switched to plain-text output.** Changed from `Handle(Json, FJsonObject*)` to `Handle(Json, FStringBuilderBase&)`. The output is concise: just a single line confirming what was created, using `MCPUtils::FormatName` for the created object. - -- **Replaced MCPAssets with MCPFetcher.** The old code used `MCPAssets` to look up the blueprint. MCPFetcher is the standard tool for resolving a caller-specified path to a specific object, and it handles error reporting automatically. - -- **Used FormatName consistently.** All object names in output and error messages now go through `MCPUtils::FormatName`. Removed raw use of the `Blueprint` parameter string in error messages where a formatted name is more appropriate. - -- **Removed all UE_LOG calls.** Two UE_LOG calls were removed. Errors and results are communicated through the output device. - -- **Removed unused includes.** Dropped `MCPAssetFinder.h`, `Kismet2/KismetEditorUtilities.h`, and `UObject/UObjectIterator.h` which were no longer needed. - -- **Dropped the `saved` field from output.** The old JSON response had a `saved` boolean. The save still happens, but there is no need to report it — if the save fails, the caller will discover that when they next interact with the blueprint. - -## What looks good - -- The handler is straightforward and focused: one graph type, one creation path. The three branches (function, macro, customEvent) are clean and short. -- The uniqueness checks (graph name and custom event name) are important safeguards that were preserved. - -## Areas of uncertainty / things left for review - -- **Save failure reporting.** The old code reported whether the save succeeded. I dropped this for conciseness, but if save failures are common or important to diagnose, it might be worth adding back a warning line (e.g., `WARNING: Package save failed`). - -- **The `nodeId` field was removed.** The old JSON response included a `nodeId` for custom events. The new plain-text output uses `FormatName(NewEvent)` instead, which should be usable as a node identifier via MCPFetcher. If callers specifically need the raw GUID, this would need adjustment. - -- **No batching.** This handler creates a single graph at a time. Batching (creating multiple graphs in one call) could be useful but would change the parameter schema. I left it as-is since creating graphs is less frequent than connecting pins. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateEnumAsset.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateEnumAsset.Notes.md deleted file mode 100644 index 2d162071..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateEnumAsset.Notes.md +++ /dev/null @@ -1,28 +0,0 @@ -# UMCPHandler_CreateEnumAsset - Refactoring Notes - -## Changes Made - -1. **Plain-text output**: Converted from JSON response (`Handle` with `FJsonObject* Result`) to plain-text response (`Handle` with `FStringBuilderBase& Result`). The output is now concise: just the created asset path, value count, and a warning if save failed. - -2. **MCPAssetFinder for duplicate check**: Added `MCPAssets` with `.Exact(AssetName).EAny()` to detect if an enum with the same name already exists, following the pattern from CreateMaterialAsset. The original had no duplicate check at all. - -3. **Concise output**: The old handler returned `assetName`, `valueCount`, and `saved` as JSON fields. The new handler emits a single line like `Created /Game/DataTypes/E_MyEnum with 5 values` plus a warning line only if save failed. No echoing of input parameters. - -4. **Removed unused includes**: Dropped `StructUtils/UserDefinedStruct.h`, `UserDefinedStructure/UserDefinedStructEditorData.h`, `Kismet2/BlueprintEditorUtils.h`, `AssetRegistry/AssetRegistryModule.h`, `AssetRegistry/IAssetRegistry.h`, and `Factories/StructureFactory.h` -- these were for struct handling, not enum. - -5. **No UE_LOG calls**: The original had none, so nothing to remove there. - -6. **Fixed indentation**: The original used spaces inconsistent with the rest of the codebase (inside the Handle method). Now uses tabs consistently. - -## What's Good - -- The core enum creation logic (AddNewEnumeratorForUserDefinedEnum + SetEnumeratorDisplayName loop) is solid and correct. -- The handler is simple and focused -- it does one thing well. - -## Areas for Further Consideration - -- **FormatName/Identifies not applicable here**: This handler creates a new asset rather than looking up existing objects by name. The asset name comes directly from the user's input path, so there's no object-naming lookup where FormatName or Identifies would apply. I used `GetPathName()` for the output, which is the standard Unreal package path -- FormatName(UEnum*) could potentially be used instead, but I wasn't sure if it would produce the full path or just a short name. Acted conservatively and used the path. - -- **Batch support**: This handler creates a single enum asset. Batching (creating multiple enums in one call) seems unlikely to be needed -- enum creation is a rare operation. Left as single-item. - -- **No validation of enum value names**: The handler doesn't validate that the enum value names are valid identifiers (no spaces, no special characters). Unreal's EnumEditorUtils may handle this internally, but it could be worth adding validation. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateMaterialAsset.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateMaterialAsset.Notes.md deleted file mode 100644 index 6385c805..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateMaterialAsset.Notes.md +++ /dev/null @@ -1,60 +0,0 @@ -# UMCPHandler_CreateMaterialAsset Refactoring Notes - -## What was done - -- **Switched to plain-text output.** The handler now overrides - `Handle(Json, FStringBuilderBase&)` instead of the JSON variant. - Output is concise key-value lines. - -- **Replaced hand-rolled enum parsing with `MCPUtils::StringToEnum`.** - The long if/else chains for Domain and BlendMode are gone. StringToEnum - handles validation and sends a clear error message on bad input. The - old code silently ignored invalid values; now it reports them. - -- **Removed all `UE_LOG` calls.** Errors and status go through the - output device. - -- **Trimmed includes.** The old file included ~30 headers for material - expression types, serialization, GUIDs, etc. that this handler does - not use. Kept only what's needed for material creation. - -- **Removed the SEH wrapper extern declaration.** This handler never - called `TryAddMaterialExpressionSEH`; that declaration belongs in - whatever handler actually uses it. - -- **Concise output.** Removed the `saved: true` echo (only report if - save *fails*). Don't echo input parameters back. - -- **Parse enums before creating the asset.** If Domain or BlendMode - are invalid, we now fail before creating the asset, rather than - creating it and then silently ignoring the bad value. - -## What looks good - -- The existence check via `MCPAssets` with `.EAny()` is - a clean use of MCPAssetFinder. - -- The `PreEditChange` / `PostEditChange` bracketing and - `SaveMaterialPackage` flow is correct. - -## Areas for potential further work - -- **FormatName not used for the created material.** The output uses - `GetPathName()` for the created material. This is arguably fine for - a creation response (the caller needs the exact path), but could be - switched to `MCPUtils::FormatName(MaterialObj)` for consistency with - other handlers. I kept `GetPathName()` because the caller likely - needs the full path to reference it later, and I wasn't sure - FormatName would produce the same thing. - -- **The existence check searches by name only, not by path.** If a - material named "M_Foo" exists at `/Game/OtherFolder/M_Foo`, the - handler will refuse to create `/Game/Materials/M_Foo`. This was the - pre-existing behavior and I left it alone, but it may be overly - conservative. A more precise check would be - `Exact(PackagePath / Name)` using the full asset path. - -- **No batch support.** This handler creates a single material. Batching - could be useful if the caller wants to create several materials at - once, but material creation is less common than node/pin operations, - so this may not be worth the complexity. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateMaterialFunctionAsset.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateMaterialFunctionAsset.Notes.md deleted file mode 100644 index 5fe28272..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateMaterialFunctionAsset.Notes.md +++ /dev/null @@ -1,27 +0,0 @@ -# UMCPHandler_CreateMaterialFunctionAsset - Refactoring Notes - -## Changes Made - -- **Switched from JSON output to plain-text output.** Changed the `Handle` override from `FJsonObject* Result` to `FStringBuilderBase& Result`. Output is now concise plain text. - -- **Removed UE_LOG calls.** There were two `UE_LOG(LogTemp, ...)` calls that the caller could never see. Removed both. - -- **Removed unnecessary includes.** The original had ~30 includes, most of which were irrelevant to this handler (MaterialGraph, EdGraph, JsonReader/Writer, Guid, FileHelper, Paths, UObjectIterator, BlueprintEditorUtils, and many material expression types). Trimmed to only what's needed. - -- **Concise output.** The original returned `path` and `saved` fields. Now it prints the asset path on success, and a warning only if save failed. No need to echo back parameters the caller already knows. - -- **MCPAssetFinder error reporting.** Already used correctly via `ExistCheck.Errors(Result)` -- updated to pass `FStringBuilderBase&` instead of `FJsonObject*`. - -- **Legacy `&*Json` pattern.** Not present here; no change needed. - -## What's Good - -- The handler is simple and focused: it does one thing (create a material function asset). -- It correctly checks for duplicate assets before creating. -- The `MCPAssets` usage for existence checks is clean and idiomatic. - -## Areas for Future Consideration - -- **FormatName not used.** The output uses `MF->GetPathName()` to report the created asset. This is the full package path, which is arguably the most useful identifier for a newly-created asset (the caller needs it to refer to the asset later). `MCPUtils::FormatName(MF)` could be used instead, but I wasn't sure whether FormatName for a UMaterialFunction returns the package path or a shorter display name. I kept `GetPathName()` to match the CreateMaterialAsset example handler, which also uses `GetPathName()`. Worth revisiting if FormatName is preferred. - -- **No batch support.** This handler creates a single asset. Batching doesn't seem valuable here since creating material functions is typically a one-at-a-time operation, but it's worth noting. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateMaterialInstanceAsset.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateMaterialInstanceAsset.Notes.md deleted file mode 100644 index ee6d8c8f..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateMaterialInstanceAsset.Notes.md +++ /dev/null @@ -1,29 +0,0 @@ -# UMCPHandler_CreateMaterialInstanceAsset - Refactoring Notes - -## Changes Made - -- **Plain-text output**: Converted from JSON output (`Handle(..., FJsonObject*)`) to plain-text output (`Handle(..., FStringBuilderBase&)`), matching the CreateMaterialAsset example handler. - -- **Removed UE_LOG calls**: Two `UE_LOG(LogTemp, ...)` calls were removed. Relevant information is now reported via the plain-text response. - -- **FormatName for parent**: The parent material name in the output now uses `MCPUtils::FormatName()` (dispatching to the correct overload for `UMaterial*` vs `UMaterialInstance*`) instead of `GetPathName()`. - -- **Concise output**: Reduced output to just the created asset path, the parent name, and a warning if save failed. Removed echoing of the `saved` boolean as a normal field (only reported on failure). - -- **Removed unused includes**: Removed headers for `MaterialExpressionScalarParameter`, `MaterialExpressionVectorParameter`, `MaterialExpressionTextureSampleParameter2D`, `MaterialExpressionStaticSwitchParameter`, and `Engine/Texture.h` -- none were used by this handler. - -- **Collapsed two null checks**: The original had separate null checks for `NewAsset` and for `Cast(NewAsset)`. Combined into one since if the cast fails, we report the same error either way. - -## What Looks Good - -- The MCPAssetFinder usage for the existence check (`ExistCheck.Exact(Name).Errors(Result).EAny().Info()`) is already clean and idiomatic. - -- The parent material lookup logic (try UMaterial, then UMaterialInstanceConstant, then raw LoadObject fallback) covers the reasonable cases well. - -## Areas for Further Work - -- **Parent lookup could use MCPFetcher**: If the `ParentMaterial` parameter were specified as a path (e.g. `/Game/Materials/M_Base`), MCPFetcher could resolve it directly. The current MCPAssets-based approach works for name-based lookups, but the code is somewhat bulky with its two-pass search + LoadObject fallback. I left it as-is because MCPFetcher doesn't seem designed for "try type A, then type B" searches, so the current approach may actually be the right one. - -- **No FormatName for the created asset**: The output line for the created asset uses `MI->GetPathName()` rather than `MCPUtils::FormatName(MI)`. There is a `FormatName(const UMaterialInstance*)` overload, but the path name is arguably more useful here since the caller just created the asset and wants to know where it landed. I left this as the path name to match the CreateMaterialAsset example, which also uses `GetPathName()` for the created asset. - -- **No batch support**: This handler creates one material instance at a time. Batch creation could be useful but would require a more complex parameter structure (an array of {name, parent} pairs). This seemed like a bigger design decision, so I left it as single-asset. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateStructAsset.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateStructAsset.Notes.md deleted file mode 100644 index 6e9e7a54..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateStructAsset.Notes.md +++ /dev/null @@ -1,32 +0,0 @@ -# UMCPHandler_CreateStructAsset - Refactoring Notes - -## Changes Made - -- **Plain-text output**: Switched from `Handle(Json, FJsonObject*)` to `Handle(Json, FStringBuilderBase&)`. Output is now concise plain text instead of JSON fields. - -- **MCPAssetFinder for existence check**: Replaced hand-rolled `FAssetRegistryModule` / `GetAssetByObjectPath` code with `MCPAssets.Exact(Name).EAny().Info()`. This is shorter, more consistent with other handlers, and automatically sends a good error message. - -- **Parameter style**: Changed from a single `AssetPath` that gets split, to separate `Name` and `PackagePath` parameters, matching the CreateMaterialAsset convention. This is more consistent across handlers. - -- **Removed UE_LOG**: The `UE_LOG(LogTemp, ...)` call for type resolution errors was replaced by passing `Result` (the output device) directly to `ResolveTypeFromString`, so the caller sees the error. - -- **Removed unused includes**: Dropped `Engine/UserDefinedEnum.h`, `Kismet2/EnumEditorUtils.h`, `AssetRegistry/AssetRegistryModule.h`, `AssetRegistry/IAssetRegistry.h`, `Factories/EnumFactory.h` -- none were needed by this handler. - -- **Concise output**: Only reports the created asset path, property count (if nonzero), and save warnings. No echoing of input parameters. - -- **Flattened nesting**: Collapsed the `bAdded` / `NewPropGuid.IsValid()` nesting into early-continue style. - -## What Looks Good - -- The GUID-diffing approach to find a newly added struct variable is a reasonable workaround for the `FStructureEditorUtils` API, which doesn't return the new variable directly. -- The `FStructPropertyEntry` USTRUCT with `PopulateFromJson` is a clean way to parse the property array entries. - -## Areas of Uncertainty / Further Work - -- **FormatName not used**: The handler creates a new asset rather than reporting on existing objects, so there wasn't a natural place to use `MCPUtils::FormatName`. The output uses `GetPathName()` for the created asset path, which is the standard Unreal package path. If there's a `FormatName` overload for `UUserDefinedStruct` or `UScriptStruct`, it might be better to use that -- but the path name seemed more useful for a "just created" confirmation. - -- **Batching**: This handler creates a single struct per call. Making it batch-capable (creating multiple structs at once) seems low-value since struct creation is infrequent. Could revisit if needed. - -- **Property add failures are silent**: If `AddVariable` returns false, we silently skip that property. It might be worth reporting which properties failed to add, but I kept it conservative since the old code also silently skipped failures. - -- **No Identifies usage**: Since this handler creates new assets rather than looking up existing ones by name, there was no place to use `MCPUtils::Identifies`. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DeleteAsset.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DeleteAsset.Notes.md deleted file mode 100644 index 97866e27..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DeleteAsset.Notes.md +++ /dev/null @@ -1,31 +0,0 @@ -# UMCPHandler_DeleteAsset Refactoring Notes - -## What Changed - -- **Converted from JSON output to plain-text output.** The handler now overrides `Handle(FStringBuilderBase&)` instead of `Handle(FJsonObject*)`. This produces more concise, LLM-friendly output. - -- **Removed all UE_LOG calls.** There were two UE_LOG calls logging the force-unload and delete operations. These are gone; all relevant information goes through the response instead. - -- **Simplified the reference reporting.** The old code built separate JSON arrays for "live" vs "stale" referencers with counts for each. The new code prints a simple list with a "(loaded)" or "(on-disk only)" annotation per entry. This is much more concise while conveying the same information. - -- **Removed unnecessary JSON fields from the response.** The old code echoed back `assetPath`, `filename`, `referencerCount`, `liveReferencerCount`, `staleReferencerCount`, and `suggestion` as separate JSON fields. The new code reports only what the caller needs to know. - -- **Removed unused includes.** Dropped `Engine/Blueprint.h`, `Kismet2/BlueprintEditorUtils.h`, `UObject/SavePackage.h`, `AssetToolsModule.h`, `IAssetTools.h`, and `FileHelpers.h` which were not needed by this handler. - -- **Cleaned up control flow.** The old code set `bDeleted` and then continued to set JSON fields regardless of success/failure. The new code returns early on failure with a clear error message. - -## What's Good About This Handler - -- The core logic is solid: it checks for the file on disk, checks references, handles force-unloading properly (clearing flags, resetting loaders, running GC), and rescans the asset registry after deletion. - -- The force-unload sequence is careful and thorough — it handles package objects, flags, loaders, and garbage collection in the right order. - -- The reference check with self-reference filtering is a good safeguard. - -## What Might Need More Work - -- **MCPAssetFinder not used.** I considered using `MCPAssets` to verify the asset exists, but this handler operates at the package/file level rather than searching by name or type. The handler needs the on-disk filename (via `LongPackageNameToFilename`) which MCPAssetFinder doesn't provide. Using MCPAssetFinder here would add an unnecessary asset registry query on top of the direct file check. I left the direct file-existence check in place. - -- **No batching.** The handler deletes one asset at a time. If bulk deletion is a common use case, it could be extended to accept an array of paths. I left this as-is since the coding standards say to batch "where it makes sense" and bulk asset deletion seems like a less common operation where doing it one at a time is safer. - -- **The reference check uses raw FName comparison.** The `Referencers.RemoveAll` lambda compares `Ref.ToString() == AssetPath` which relies on exact string equality. This works for the self-reference filtering case but could theoretically miss case differences. I preserved the existing behavior. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DeleteBlueprintGraph.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DeleteBlueprintGraph.Notes.md deleted file mode 100644 index 7f2a6088..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DeleteBlueprintGraph.Notes.md +++ /dev/null @@ -1,30 +0,0 @@ -# UMCPHandler_DeleteBlueprintGraph - Refactoring Notes - -## What was changed - -- **Plain-text output.** Switched from `Handle(Json, FJsonObject*)` to `Handle(Json, FStringBuilderBase&)`. The old handler returned JSON fields like `graphType`, `nodeCount`, `saved` -- none of which the caller really needs. The new handler just says "Deleted function graph MyFunc" and warns if save failed. - -- **MCPFetcher for blueprint resolution.** Replaced the `MCPAssets` lookup with `MCPFetcher(Result).Walk(Path)` followed by `F.Cast()`. This is more concise, gives consistent error messages, and accepts the same path format as other handlers. - -- **Parameter renamed.** Changed `Blueprint` to `Path` for consistency with other MCPFetcher-based handlers (like DumpGraphs). The caller now passes a package path rather than a name-or-path string. - -- **FormatName for output.** All graph names in output and error messages now use `MCPUtils::FormatName()` instead of echoing back the raw user input. - -- **Identifies for matching.** The graph-matching loops already used `MCPUtils::Identifies` -- no change needed there. - -- **Removed UE_LOG calls.** Two UE_LOG calls were removed. Error/success information goes through the output device instead. - -- **Removed unnecessary includes.** Dropped `EdGraphNode.h`, `EdGraphSchema_K2.h`, `K2Node_CustomEvent.h`, `KismetEditorUtilities.h`, `UObjectIterator.h` -- none were needed by this handler. - -- **Removed nodeCount from output.** The old handler reported the number of nodes in the deleted graph. This is not useful information for the caller, so it was dropped for conciseness. - -## What looks good - -- The core logic (search function graphs, then macro graphs, then check ubergraph pages for a clear error) is clean and correct. No structural changes were needed. -- The handler correctly calls `MarkBlueprintAsStructurallyModified` and saves the package. - -## Areas of uncertainty / possible future work - -- **Batch deletion.** The coding standards suggest batch operations where practical. This handler deletes one graph at a time. A batch version (accepting an array of graph names) could be useful if callers frequently delete multiple graphs, but it seems uncommon enough that single-graph deletion is fine for now. - -- **MCPFetcher graph walker.** MCPFetcher supports `graph:Name` segments, so in theory the caller could pass `/Game/Foo,graph:MyFunc` and MCPFetcher would resolve the graph directly. However, this handler needs to know whether the graph is a function or macro (to report the type and to reject ubergraph pages), so we still do the manual search. Using MCPFetcher's graph walker would lose the ability to distinguish graph types and reject EventGraph deletion with a clear message. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DeleteMaterialExpression.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DeleteMaterialExpression.Notes.md deleted file mode 100644 index 3f00b5ba..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DeleteMaterialExpression.Notes.md +++ /dev/null @@ -1,30 +0,0 @@ -# UMCPHandler_DeleteMaterialExpression - Refactoring Notes - -## Changes Made - -- **Plain-text output.** Switched from `Handle(Json, FJsonObject*)` to `Handle(Json, FStringBuilderBase&)`. Output is now a single line like `Deleted ScalarParam 'Roughness'` instead of a JSON tree echoing back all the parameters. - -- **MCPErrorCallback for errors.** Replaced `MCPUtils::MakeErrorJson(Result, ...)` with `MCPErrorCallback(Result).SetError(...)`, which works with the plain-text output device. - -- **MCPUtils::Identifies for node lookup.** The old code matched nodes by comparing `NodeGuid.ToString() == Node`. The refactored code uses `MCPUtils::Identifies(Node, MatNode->MaterialExpression)`, which matches by FormatName conventions and handles case sensitivity correctly. This also changes the `Node` parameter description to reference FormatName output rather than GUIDs. - -- **MCPUtils::FormatName for output.** Already partially used in the original; now used consistently. Removed `DeletedNodeTitle` and `DeletedExprClass` in favor of a single `FormatName(MaterialExpression)` call. - -- **Removed UE_LOG calls.** Two UE_LOG calls removed; relevant info goes to the response instead. - -- **Concise output.** No longer echoes material name, expression class, or other parameters back. Just reports what was deleted and whether the save succeeded. - -- **Trimmed includes.** The original had ~40 includes for material expression subtypes, JSON serialization, asset tools, etc. Reduced to the minimal set actually needed. - -## What Looks Good - -- The core deletion logic (RemoveExpression, MarkAsGarbage, NotifyGraphChanged, PreEditChange/PostEditChange, MarkPackageDirty) is solid and unchanged. -- The material-vs-materialFunction branching follows the same pattern as other refactored material handlers (SetMaterialExpressionPosition, etc.). - -## Uncertainties / Conservative Choices - -- **MCPFetcher not used for node lookup.** There's no MCPFetcher walker for material expressions currently. The manual loop over `Graph->Nodes` with `MCPUtils::Identifies` is consistent with how other refactored material handlers (SetMaterialExpressionPosition, SetMaterialExpressionProperty) handle this. If a material-expression walker is added to MCPFetcher in the future, this could be simplified. - -- **Breaking change: Node parameter semantics.** The old handler accepted a GUID string for the `Node` parameter. The refactored version accepts a FormatName-style expression name instead, consistent with other material handlers. Callers that were passing GUIDs will need to switch to FormatName identifiers (which is what DumpMaterial outputs). - -- **Batching not added.** This handler deletes a single expression. Batching (deleting multiple expressions in one call) could be useful but wasn't added since it would change the parameter schema. Could be a future enhancement. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DeleteNodeFromGraph.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DeleteNodeFromGraph.Notes.md deleted file mode 100644 index a9d1b508..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DeleteNodeFromGraph.Notes.md +++ /dev/null @@ -1,22 +0,0 @@ -# UMCPHandler_DeleteNodeFromGraph - Refactoring Notes - -## What was done - -- Converted from JSON output (`FJsonObject*`) to plain-text output (`FStringBuilderBase&`). -- MCPFetcher now receives `Result` (the FStringBuilderBase) as its error callback, so path resolution errors go directly to the caller as plain text. -- Removed the three `Result->SetStringField` calls that echoed nodeClass/nodeTitle/graph back. The output now just confirms the deletion with a single line. -- Removed both `UE_LOG` calls. -- Error messages for protected entry nodes now go through `MCPErrorCallback(Result)` instead of `MCPUtils::MakeErrorJson`. -- Removed the `#include "EdGraph/EdGraph.h"` include since `EdGraphNode.h` pulls it in (matching the example handler pattern). - -## What's good about this handler - -- The entry-node protection logic is solid and the error messages are detailed and helpful, explaining *why* the deletion is refused and what the user should do instead. -- MCPFetcher handles all the object resolution cleanly — the `Node` path parameter does all the work. -- The handler is concise; the bulk of the code is the entry-node guards, which is appropriate. - -## Uncertainties / conservative choices - -- I kept all three entry-node checks (`UK2Node_FunctionEntry`, `UK2Node_Event`, `UK2Node_CustomEvent`) as separate blocks with distinct error messages. They could be collapsed into one check, but the per-type messages are more informative. -- `UK2Node_CustomEvent` inherits from `UK2Node_Event`, so the `Cast` check would catch custom events too. The current ordering (FunctionEntry, Event, CustomEvent) means the CustomEvent branch is unreachable. If the intent is to give a different message for custom events, the CustomEvent check should come *before* the Event check. I left the order as-is to minimize diff, but this is worth reviewing. -- This handler operates on a single node. The coding standards mention batching where it makes sense. A batch-delete could be useful, but it changes the API signature, so I left it as single-node for now. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DescribeMaterialInEnglish.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DescribeMaterialInEnglish.Notes.md deleted file mode 100644 index 9b9db79c..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DescribeMaterialInEnglish.Notes.md +++ /dev/null @@ -1,40 +0,0 @@ -# UMCPHandler_DescribeMaterialInEnglish — Refactoring Notes - -## Changes Made - -1. **Converted to plain-text output.** Switched from `Handle(Json, FJsonObject*)` to `Handle(Json, FStringBuilderBase&)`. Output is now concise plain text listing only connected root inputs, one per line: `PinName <- chain`. - -2. **Removed UE_LOG call.** The old code logged the material name to LogTemp, which the LLM caller can't see. - -3. **Used MCPUtils::EnsureMaterialGraph.** Replaced the inline graph-creation code (CastChecked, RebuildGraph, etc.) with the existing utility. - -4. **Used MCPUtils::FormatName consistently.** Material name, pin names, texture names, material function names, and the fallback expression description all use FormatName now. Previously, textures and material functions used `GetName()` directly. - -5. **Used MCPErrorCallback for errors.** Replaced `MCPUtils::MakeErrorJson(Result, ...)` with `MCPErrorCallback(Result).SetError(...)` which is the correct pattern for plain-text handlers. - -6. **Concise output.** Removed the JSON fields (`material`, `materialPath`, `inputs` array, `connected` booleans). No longer echoes unconnected pins — only connected root inputs are listed, which is the actionable information. The material name is printed once at the top. - -7. **Removed unused includes.** Dropped headers that were no longer needed (MaterialDomain.h, MaterialInstanceConstant.h, TextureCoordinate, ComponentMask, Custom, FunctionInput, FunctionOutput, AssetRegistry, Kismet2, etc.). - -8. **Reduced nesting.** In the TracePin lambda, the non-material-node case now uses `continue` to avoid nesting the material-node logic inside an else block. - -## What's Good - -- The recursive TracePin logic is solid — it walks the expression graph backwards from root and produces readable chain descriptions. -- The special-case formatting for parameters, constants, textures, and function calls gives much more useful output than just class names. -- Depth limit of 10 prevents runaway recursion on cyclic or very deep graphs. - -## Uncertainties and Conservative Choices - -- **Parameter names in expression descriptions.** The ScalarParam, VectorParam, etc. descriptions use `ParameterName.ToString()` directly rather than `MCPUtils::FormatName(Expr)`. This is intentional: FormatName for expressions gives the class-based name, while here we want the user-facing parameter name and its default value. However, I'm not certain whether FormatName for expressions already includes the parameter name — if it does, these special cases could be simplified. - -- **FormatName for UTexture.** I changed `TP->Texture->GetName()` to `MCPUtils::FormatName(TP->Texture)`. I haven't verified what FormatName produces for textures — if it returns a long package path, the output may be more verbose than before. The old code just used the short name. - -- **FormatName for UMaterialFunction.** Same situation as textures — changed from `GetName()` to `MCPUtils::FormatName()`. Could be more verbose. - -- **Omitting unconnected pins.** The old code included unconnected pins in the JSON output with `connected: false`. The refactored version skips them entirely for conciseness. If callers need to know which pins exist but are unconnected, this would need to be revisited. - -## Possible Future Work - -- Could use MCPFetcher to resolve the material instead of MCPAssets, if the Material parameter were changed to accept a full path with walkers. Currently MCPAssets is the right tool since the parameter is just a material name/path. -- The long chain of `Cast<>` checks in TracePin could potentially be simplified if MCPUtils::FormatName for expressions already includes parameter details — worth investigating. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DiffTwoBlueprints.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DiffTwoBlueprints.Notes.md deleted file mode 100644 index a21c670f..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DiffTwoBlueprints.Notes.md +++ /dev/null @@ -1,31 +0,0 @@ -# UMCPHandler_DiffTwoBlueprints - Refactoring Notes - -## Status - -This handler was already fully refactored when reviewed. All coding standards are met. - -## What's Good - -- **MCPAssetFinder usage**: Both blueprints are loaded via `MCPAssets` with `.Exact()`, `.Errors(Result)`, `.ENone()`, `.ETwo()`, `.Load()`. This is the correct pattern -- concise, with automatic error reporting back to the caller. - -- **FormatName consistency**: All object names (graphs, nodes, pins, variables) go through `MCPUtils::FormatName()`. No ad-hoc naming. - -- **Identifies for filtering**: The optional `Graph` filter uses `MCPUtils::Identifies(Graph, G)`, which is the correct way to match user-supplied names against objects. - -- **Plain-text output**: Uses `FStringBuilderBase&` throughout. Output is concise and structured for LLM consumption -- no JSON overhead, no echoed parameters. - -- **No UE_LOG calls**: All errors go through the response via MCPErrorCallback (wired through MCPAssets). - -- **Good output structure**: Identical graphs are noted briefly, differing graphs show only-in-A / only-in-B nodes and connections, and variable differences are listed at the end. The summary line with total differences is useful. - -## Potential Improvements - -- **Connection diff format**: The connection keys use a `|`-separated format (`NodeA|PinA|NodeB|PinB`). This is functional but could be more readable, e.g. `NodeA.PinA -> NodeB.PinB`. However, changing this is cosmetic and low priority. - -- **No MCPFetcher usage**: This handler uses MCPAssetFinder directly rather than MCPFetcher. That's appropriate here -- the handler takes two blueprint names as separate parameters, not path-style identifiers. MCPFetcher's `Walk()` would add unnecessary overhead since we don't need to drill into graphs/nodes/pins by path. - -- **Macro-level graphs not included**: The `GatherGraphs` lambda only collects `UbergraphPages` and `FunctionGraphs`. Macro graphs (`MacroGraphs`) are excluded. This may be intentional since macros are often shared/library code, but it's worth noting. If macro comparison is wanted, adding `BP->MacroGraphs` to the loop would be trivial. - -- **Node matching by name only**: Nodes are matched across blueprints by their `FormatName`. If two nodes have the same formatted name (e.g. two "PrintString" calls), they're counted rather than individually diffed. This is a reasonable structural comparison, but it means pin-level differences between same-named nodes aren't detected. A deeper per-node diff would be significantly more complex and may not be worth the token cost in output. - -- **No subgraph recursion**: Collapsed graphs / composite nodes with inner graphs are not recursed into. Only top-level graphs are compared. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DisconnectMaterialExpressionPin.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DisconnectMaterialExpressionPin.Notes.md deleted file mode 100644 index 528ebb73..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DisconnectMaterialExpressionPin.Notes.md +++ /dev/null @@ -1,31 +0,0 @@ -# UMCPHandler_DisconnectMaterialExpressionPin - Refactoring Notes - -## What was done - -- **Plain-text output**: Converted from JSON (`FJsonObject* Result`) to plain-text (`FStringBuilderBase& Result`). Output is now concise and LLM-friendly. - -- **MCPFetcher for node/pin lookup**: Replaced the hand-rolled GUID-based node search loop and `FindPin(FName(*PinName))` with `MCPFetcher(Result, Graph).Node(Node).Pin(PinName)`. This uses `MCPUtils::Identifies` under the hood for consistent name matching, and provides good error messages automatically (including ambiguity detection). - -- **FormatName for output**: All names in output use `MCPUtils::FormatName()` instead of `GetName()` or raw strings. - -- **Removed UE_LOG calls**: Two UE_LOG calls removed. - -- **Removed DryRun parameter**: The dry-run feature added complexity without clear value for an LLM caller. If this is needed, it can be re-added. - -- **Trimmed includes**: Removed ~20 unnecessary includes (factories, serialization, JSON, GUID, etc.) that weren't used by this handler. - -- **Concise output**: No longer echoes back the command parameters. Reports only the result: how many links were broken, and whether save succeeded. - -## What's good about this handler - -- Supports both materials and material functions, which is genuinely useful since they share the same graph structure. -- Uses MCPAssetFinder correctly for asset lookup with proper error handling. -- The PreEditChange/PostEditChange + save pattern is correct for material modifications. - -## Areas of uncertainty / possible further work - -- **Node parameter description changed**: The old handler accepted a "Node GUID" while the refactored version accepts a FormatName-style identifier. This is the right change per coding standards, but callers using GUIDs will need to switch to FormatName identifiers. MCPUtils::Identifies may or may not accept raw GUIDs for graph nodes -- if it does, this is backward compatible; if not, existing callers will break. - -- **Batching**: The coding standards mention batching where practical. This handler disconnects a single pin. A batch version (accepting an array of node+pin pairs) could be valuable if LLMs frequently need to disconnect multiple pins in one call. Not done here because it would change the tool's interface significantly. - -- **No targetPin support**: Unlike DisconnectPins (for blueprints), this handler always breaks ALL links on the pin. It doesn't support breaking a specific link to a specific target pin. This may be fine for material graphs where pins typically have few connections, but could be added if needed. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DisconnectPins.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DisconnectPins.Notes.md deleted file mode 100644 index 189091aa..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DisconnectPins.Notes.md +++ /dev/null @@ -1,26 +0,0 @@ -# UMCPHandler_DisconnectPins Refactoring Notes - -## What was done - -- Converted from JSON output (`FJsonObject*`) to plain-text output (`FStringBuilderBase&`). -- MCPFetcher now receives the `FStringBuilderBase& Result` directly as its error callback, so resolution errors (bad blueprint path, missing node, missing pin) are reported automatically in the response text. -- Removed the `UE_LOG` call. -- Removed JSON result-building code (no more `EntryResult`, `Results` array, `SetNumberField`, `SetArrayField`). -- Removed the `Engine/Blueprint.h` and `EdGraph/EdGraphNode.h` includes that are no longer directly needed (MCPFetcher.h and MCPUtils.h pull in what's required). -- Kept batch support via `FMCPJsonArray Disconnections`. -- Output no longer echoes parameters back; just reports per-entry results and a summary line. -- `MCPUtils::FormatName()` was already being used for error messages in the original — preserved that. - -## What was already good - -- The handler already used `MCPFetcher` for object resolution. -- The handler already used `MCPUtils::FormatName()` for pin/node names in error messages. -- The handler already used `MCPUtils::PopulateFromJson()` to deserialize batch entries. -- Batch support was already in place. -- The "not connected" error message was already clear and specific. - -## Uncertainties / conservative choices - -- The per-entry success lines (`Disconnected N link(s) from ...`) add some verbosity. An alternative would be to only report errors and print a single summary. I chose to report each entry so the caller can see what happened, but this could be trimmed if the output is too noisy. -- The original code had per-entry `disconnectedCount` in the JSON results array. In plain text, I folded this into the per-entry status line. If callers were parsing the per-entry counts programmatically, they'd need to adapt — but since the goal is LLM consumption, this should be fine. -- I kept `EdGraph/EdGraphPin.h` as a direct include since `FDisconnectPinEntry` conceptually deals with pins and `BreakLinkTo`/`BreakAllPinLinks` are pin methods. Could potentially be removed if MCPFetcher.h transitively includes it, but being explicit seems safer. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DumpBlueprint.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DumpBlueprint.Notes.md deleted file mode 100644 index 33e77626..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DumpBlueprint.Notes.md +++ /dev/null @@ -1,33 +0,0 @@ -# UMCPHandler_DumpBlueprint - Refactoring Notes - -## What was done - -The handler was fully converted to the new coding standards: - -- **Plain-text output**: Uses `FStringBuilderBase&` signature, not JSON. -- **MCPFetcher**: Uses `MCPFetcher F(Result)` with `F.Walk(Blueprint).Cast()` to resolve the blueprint. Error messages go directly to the caller via the Result stringbuilder. -- **MCPUtils::FormatName()**: Used for all object naming - blueprint, parent class, interfaces, variables, components, component classes, and graphs. -- **MCPUtils::FormatPinType()**: Used for variable type formatting. -- **MCPUtils::EnumToString()**: Used for BlueprintType. -- **No UE_LOG calls**. -- **Concise output**: Does not echo parameters back. Uses compact formatting (e.g. `int x = 0 [Category]` rather than verbose key-value pairs). - -## What looks good - -- The handler is clean and concise. Each section (header, anim blueprint, interfaces, variables, components, graphs) is clearly separated. -- MCPFetcher's error callback is wired to the Result stringbuilder, so asset-not-found errors reach the caller automatically. -- The variable output format (`type name = default [category]`) is compact and readable. -- Component output includes parent information when relevant. -- Early returns via `if (!BP) return` after MCPFetcher keeps the code flat. - -## Uncertainties / conservative decisions - -- **TargetSkeleton path**: The TargetSkeleton line uses `GetPathName()` instead of `MCPUtils::FormatName()`. There is a `FormatName` overload for `USkeletalMesh` but not for `USkeleton`. Since `USkeleton` is not a skeletal mesh, and there's no `FormatName(USkeleton*)` overload, `GetPathName()` is the most reliable option. If a `FormatName` overload is added for skeletons later, this should be updated. - -- **MCPUtils::Identifies() not used**: This handler only outputs data, it doesn't do name matching/filtering, so `Identifies()` is not needed here. This is correct behavior. - -- **No batching**: This handler dumps a single blueprint. Batching (dumping multiple blueprints at once) could be added but seems unnecessary - the caller would typically inspect one blueprint at a time, and the output per blueprint can be substantial. - -## Nothing remaining - -The refactoring appears complete. All coding standards are met. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DumpGraphs.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DumpGraphs.Notes.md deleted file mode 100644 index 9195fa36..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DumpGraphs.Notes.md +++ /dev/null @@ -1,39 +0,0 @@ -# UMCPHandler_DumpGraphs - Refactoring Notes - -## What's Good - -This handler was already in good shape before this review: - -- Uses plain-text output (`FStringBuilderBase&`). -- Uses `MCPFetcher` for object resolution, with the string builder as - the error callback, so fetch errors go directly to the caller. -- Uses `MCPUtils::FormatName()` for graph names in section headers. -- Uses `MCPUtils::AllGraphs()` to enumerate graphs. -- No `UE_LOG` calls. -- Doesn't echo parameters back to the caller. -- Clean, minimal code with a private `EmitGraph` helper. - -## Changes Made - -- The error message for unexpected object types used - `F.Obj->GetClass()->GetName()` directly. Changed to - `MCPUtils::FormatName(F.Obj->GetClass())` for naming consistency. - -## No Changes Needed - -- No `MCPUtils::Identifies()` usage is needed because this handler - doesn't do any name matching -- `MCPFetcher::Walk` handles all of - that internally. -- No `MCPAssetFinder` usage is needed because the handler resolves - objects by path, not by search. `MCPFetcher` is the right tool here. -- No batching opportunity: the handler already dumps all graphs when - given a blueprint, or a single graph when given a graph path. That's - the natural granularity. - -## Uncertainties - -- The `EmitGraph` method delegates entirely to `FlxBlueprintExporter`. - I did not audit that class for compliance with these standards. If - `FlxBlueprintExporter` uses its own naming scheme internally (rather - than `MCPUtils::FormatName`), that could be a consistency issue, but - it's outside the scope of this handler. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DumpMaterial.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DumpMaterial.Notes.md deleted file mode 100644 index a3e89e5c..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DumpMaterial.Notes.md +++ /dev/null @@ -1,40 +0,0 @@ -# UMCPHandler_DumpMaterial Refactoring Notes - -## What was done - -- **Switched to plain-text output.** The handler now overrides `Handle(FStringBuilderBase&)` instead of `Handle(FJsonObject*)`. The plain-text format is significantly more concise and LLM-friendly. - -- **Removed UE_LOG calls.** Both log statements (one for materials, one for material instances) were removed. - -- **Used FormatName consistently.** All object names now use `MCPUtils::FormatName()` — materials, material instances, textures, and texture parameters. The old code used raw `GetName()` / `GetPathName()` throughout. - -- **Used EnumToString.** Material domain and blend mode now use `MCPUtils::EnumToString` with prefix stripping instead of hand-rolling `StaticEnum<>()->GetNameStringByValue()`. - -- **Removed UrlDecode.** The old code manually called `MCPUtils::UrlDecode(Material)`. MCPAssetFinder's `Exact()` handles this internally. - -- **Trimmed includes.** Removed ~15 unused includes (MaterialFunction, MaterialExpressionConstant variants, MaterialExpressionCustom, MaterialExpressionComponentMask, MaterialExpressionTextureCoordinate, MaterialExpressionFunctionInput/Output/Call, MaterialGraph nodes, BlueprintEditorUtils, AssetRegistry, EdGraph). - -- **Concise output format.** Usage flags only print enabled ones instead of listing all as true/false. Non-default settings (OpacityMaskClipValue, DitheredLODTransition, AllowNegativeEmissiveColor) are only printed when they differ from defaults. Sections like "Parameters" and "Referenced Textures" only appear if there's content. - -- **Split into helper methods.** `EmitMaterial` and `EmitMaterialInstance` keep the main Handle method clean and under two nesting levels. - -## What's good - -- The MCPAssetFinder usage with `NoScans()` + explicit `Scan()` / `Scan()` is precise about what asset types to search. -- Error handling flows naturally through `Assets.Errors(Result)` — no manual error message construction needed. -- The parent of a material instance is handled for both `UMaterial` and `UMaterialInstance` parent types. - -## Areas of uncertainty / conservative choices - -- **FormatName for texture parameters in material instances.** `FTextureParameterValue::ParameterValue` is a `UTexture*`, so `MCPUtils::FormatName(UTexture*)` is used. This should be correct, but I haven't verified that all texture subclasses format nicely. - -- **Shading models enumeration.** I kept the original manual enumeration loop for shading models because `MCPUtils::EnumToString` works on a single enum value, not a bitfield. This is the one place where raw `StaticEnum` usage remains, since `FMaterialShadingModelField` is a bitmask, not a single value. - -- **OpacityMaskClipValue default.** I used `0.3333f` as the default threshold for suppressing this line. Unreal's actual default for `OpacityMaskClipValue` is `0.3333...` but the exact floating-point comparison could theoretically miss edge cases. This is a minor concern. - -- **MCPFetcher not used.** The handler searches by asset name/path (not by a walker path like `"/Game/Foo,graph:Bar"`), so MCPAssetFinder is the right tool here, not MCPFetcher. MCPFetcher would be appropriate if the handler accepted a walker path to navigate into sub-objects. - -## Potential future improvements - -- Could add a `MaterialFunction` path: the handler currently only supports `UMaterial` and `UMaterialInstanceConstant`. Material functions are a separate asset type that users might want to inspect. -- The parameter output could include `MCPUtils::FormatName(Expr)` for each parameter expression, which would let callers reference those expressions in other tools. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DumpMaterialExpressionGraph.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DumpMaterialExpressionGraph.Notes.md deleted file mode 100644 index 6b7ba85d..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DumpMaterialExpressionGraph.Notes.md +++ /dev/null @@ -1,35 +0,0 @@ -# UMCPHandler_DumpMaterialExpressionGraph - Refactoring Notes - -## What was done - -1. **Converted from JSON to plain-text output.** Changed from `Handle(Json, FJsonObject* Result)` to `Handle(Json, FStringBuilderBase& Result)`. The old version serialized the graph to JSON via `MCPUtils::SerializeGraph` then copied fields into the result. The new version emits readable text showing nodes and connections directly. - -2. **Removed UE_LOG calls.** Two `UE_LOG(LogTemp, ...)` calls were removed. Errors now go through the Result string builder. - -3. **Used MCPUtils::FormatName consistently.** All node, pin, and expression names now go through `FormatName` instead of ad-hoc methods like `GetName()` or `GetPathName()`. - -4. **Used MCPAssetFinder with error callback on Result.** The `Errors(Result)` pattern sends errors directly to the caller, matching the DumpMaterial example. - -5. **Removed UrlDecode call.** The `MCPUtils::UrlDecode(Material)` call was unnecessary -- the handler framework handles parameter decoding. - -6. **Used MCPUtils::EnsureMaterialGraph.** Replaced the inline graph-creation code with the existing utility, reducing duplication. - -7. **Trimmed includes.** Removed ~20 unused includes (MaterialFunction, various expression subtypes, AssetRegistry, etc.). Only kept headers actually needed by the refactored code. - -8. **Removed redundant material/materialPath fields.** The old JSON output appended `material` and `materialPath` fields echoing back what the caller already knew. Per coding standards, don't echo command parameters. - -## What looks good - -- The handler has a clear, single purpose: dump a material's expression graph. -- MCPAssetFinder with `ENone().ETwo()` is correct -- it requires exactly one matching material. -- The error path for a missing MaterialGraph is preserved. - -## Areas of uncertainty / potential further work - -- **Output format completeness.** The old version delegated to `MCPUtils::SerializeGraph`, which may have included information (default pin values, node positions, etc.) that the new plain-text output omits. If callers need that level of detail, the output may need to be enriched. I kept it concise per the coding standards, showing node names, expression types, and connections. - -- **Pin direction filtering.** I only emit output-direction pins with connections, which gives a clean "A -> B" connection list without duplicates. Input pins with default values are not shown. If the caller needs to see unconnected input pins or their default values, that would need to be added. - -- **Overlap with DumpMaterial.** The DumpMaterial handler already shows expression counts and parameter info. This handler focuses on the graph topology (connections between nodes). The two handlers complement each other, but there may be room to consolidate or cross-reference. - -- **MCPUtils::EnsureMaterialGraph.** I assumed this utility exists and handles the same logic that was inlined (create graph, set material, rebuild). If it doesn't do all of that, the inline code may need to be restored. I saw it declared in MCPUtils.h but did not verify the implementation. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DumpMaterialFunction.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DumpMaterialFunction.Notes.md deleted file mode 100644 index a1ec4977..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DumpMaterialFunction.Notes.md +++ /dev/null @@ -1,34 +0,0 @@ -# UMCPHandler_DumpMaterialFunction Refactoring Notes - -## What was done - -1. **Plain-text output.** Converted from JSON (`FJsonObject*`) to plain-text (`FStringBuilderBase&`). This cuts token count significantly -- the old version emitted deeply nested JSON with redundant field names like `"expressionType"`, `"posX"`, `"posY"` for every expression, plus separate `inputs`/`outputs`/`expressions` arrays that duplicated information. - -2. **FormatName.** Replaced all ad-hoc name generation (`MF->GetName()`, `MF->GetPathName()`, `FI->InputName.ToString()`, etc.) with `MCPUtils::FormatName()`. This ensures that names emitted here can be used as identifiers with other tools. - -3. **MCPAssetFinder.** The old code already used `MCPAssets` properly. Kept that as-is. - -4. **Removed UE_LOG.** Removed the `UE_LOG(LogTemp, ...)` call. - -5. **Removed UrlDecode.** The old code called `MCPUtils::UrlDecode(MaterialFunction)` before passing the string to `MCPAssets::Exact()`. The DumpMaterial handler (the example) does not do this -- the parameter population layer presumably handles decoding. Removed to match the example pattern. - -6. **Removed graph serialization.** The old code serialized the entire `MaterialGraph` as a JSON blob via `MCPUtils::SerializeGraph()`. This was extremely verbose and mostly duplicated the expression list. Removed it. If a caller needs the graph topology, a separate tool (like DumpGraph) would be more appropriate. - -7. **Concise expression details.** Instead of a full JSON object per expression, each expression is now one line: its `FormatName` followed by inline key=value details for the important fields. Position (posX/posY) is omitted since it's rarely useful to an LLM. - -## What's good - -- The handler is now compact and follows the same pattern as DumpMaterial. -- Error handling is delegated entirely to MCPAssets via `.Errors(Result)`. -- Expression details are broken out into a helper method (`EmitExpressionDetails`) keeping nesting shallow. -- Inputs and outputs are listed in their own sections for quick scanning, then all expressions appear in a flat list. - -## Areas of uncertainty / remaining work - -- **FormatName for FunctionInput/FunctionOutput expressions.** I used `MCPUtils::FormatName(Expr)` for both the Inputs/Outputs sections and the full expression list. I haven't verified that `FormatName(UMaterialExpression*)` produces a good name for `FunctionInput`/`FunctionOutput` expressions specifically (e.g., whether it includes the input/output name). If it doesn't, those sections might need supplementing with the `InputName`/`OutputName` field explicitly. - -- **Include trimming.** I removed includes that were clearly unused (`MaterialGraph/*.h`, `EdGraph/*.h`, `AssetRegistry/*.h`, `Kismet2/*.h`, `MaterialDomain.h`, `MaterialInstanceConstant.h`, `Material.h`). I kept all the `MaterialExpression*.h` includes that correspond to types checked in `EmitExpressionDetails`. Some of these might be unnecessary if they're transitively included, but being explicit is safer. - -- **Batching.** This handler processes one material function at a time. The coding standards mention batching where it makes sense. For a "dump" command, single-item seems appropriate -- the output for one function can already be substantial. - -- **Missing expression types.** The `EmitExpressionDetails` helper covers the same expression types the old JSON serializer did, plus a few more. There are many more expression types in Unreal's material system. The fallback is simply no extra detail (just the FormatName), which seems reasonable. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DumpMaterialInstanceParameters.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DumpMaterialInstanceParameters.Notes.md deleted file mode 100644 index f5900de1..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DumpMaterialInstanceParameters.Notes.md +++ /dev/null @@ -1,27 +0,0 @@ -# UMCPHandler_DumpMaterialInstanceParameters - Refactoring Notes - -## What was done - -1. **Converted from JSON to plain-text output.** Changed `Handle(Json, FJsonObject*)` to `Handle(Json, FStringBuilderBase&)`. This dramatically reduces code volume and token count for the LLM consumer. - -2. **Switched to FormatName.** Replaced `GetName()` and `GetPathName()` calls with `MCPUtils::FormatName()` for the material instance, parent materials, and textures. This ensures consistent naming across handlers. - -3. **Removed unnecessary includes.** Dropped `Factories/MaterialInstanceConstantFactoryNew.h`, `AssetToolsModule.h`, and `IAssetTools.h` which were unused by this handler. - -4. **Concise output format.** Eliminated redundant `isOverridden` booleans - instead, parameters are grouped under clear "Overridden Parameters" and "Inherited Parameters (not overridden)" headers, which conveys the same information more naturally in plain text. The parent chain is a single line instead of a JSON array of objects. - -5. **No UE_LOG calls were present**, so nothing to remove there. - -## What looks good - -- The MCPAssets usage was already clean and idiomatic. The `Exact().Errors().ENone().ETwo().Load()` chain follows the pattern from DumpMaterial exactly. -- The logic for collecting overridden parameter names and filtering inherited ones is sound. -- The handler covers all four parameter types (scalar, vector, texture, static switch). - -## Areas of uncertainty / conservative choices - -- **Parent chain formatting:** The original built a full JSON array with name/path/class for each parent. I condensed this to a single line with FormatName identifiers separated by `->`. This is much more concise but loses the explicit class field. FormatName for materials vs material instances should produce distinguishable names, so this seems fine, but if callers need to programmatically distinguish parent types, this could be a concern. - -- **Static parameters are fetched twice** - once to build the OverriddenStaticSwitches set, and once to emit overridden values. The original had the same issue. Could be consolidated into a single `GetStaticParameterValues` call, but I kept it as-is since it matches the structure of the other parameter types and the cost is negligible. - -- **No batching opportunity here.** This handler inspects a single material instance, which is the right granularity - you wouldn't typically want to dump parameters for multiple material instances in one call. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DuplicateNodesInGraph.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DuplicateNodesInGraph.Notes.md deleted file mode 100644 index 7ee32dc1..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DuplicateNodesInGraph.Notes.md +++ /dev/null @@ -1,32 +0,0 @@ -# UMCPHandler_DuplicateNodesInGraph - Refactoring Notes - -## What was done - -1. **MCPFetcher migration**: Replaced the separate `Blueprint` + `Graph` parameters with a single `Graph` path parameter that uses MCPFetcher to resolve directly to a `UEdGraph*`. This eliminates MCPAssets usage, the hand-rolled `AllGraphsNamed` lookup, and the `UrlDecode` call. The caller now uses a path like `/Game/Foo,graph:EventGraph`. - -2. **Plain-text output**: Switched from `Handle(Json, FJsonObject*)` to `Handle(Json, FStringBuilderBase&)`. Output is now concise lines like `Duplicated: NodeName -> NewNodeName`. Much fewer tokens than the old JSON with sourceNodeId, newNodeId, posX, posY, nodeClass, nodeTitle fields. - -3. **FormatName/Identifies**: Node lookup now uses `MCPUtils::Identifies(Name, Node)` instead of `FindNodeByGuid`. Output uses `MCPUtils::FormatName(Node)` instead of raw GUIDs or titles. - -4. **Removed UE_LOG**: Both UE_LOG calls removed. Errors go to the caller via the Result string builder. - -5. **Removed unnecessary includes**: Stripped ~40 includes down to only what's needed. - -## What's good about this handler - -- The core duplication logic (DuplicateObject, CreateNewGuid, clear linked pins, offset position, AddNode) is straightforward and correct. -- It properly marks the blueprint as structurally modified after making changes. -- The batch design (duplicating multiple nodes in one call) is good for LLM efficiency. - -## Areas of uncertainty / conservative choices - -- **OldToNewGuidMap**: The old code built an `OldToNewGuidMap` but never used it. I removed it. If there was a plan to remap internal references between duplicated nodes (e.g., if duplicating a group of interconnected nodes and wanting to reconnect them), that logic was never implemented. This might be worth revisiting. - -- **GetOuter() for blueprint**: I use `Cast(TargetGraph->GetOuter())` to find the blueprint for `MarkBlueprintAsStructurallyModified`. This works for standard blueprint graphs but might not work for deeply nested subgraphs (e.g., inside collapsed nodes or anim state machines) where the outer could be another node. If that's a concern, walking up the outer chain might be needed. - -- **Node name ambiguity**: If two nodes in the graph have the same FormatName, Identifies will match the first one found. The old code used GUIDs which are always unique. In practice FormatName produces sufficiently unique names, but this is a theoretical regression. - -## More to do - -- The ConnectPins example handler still uses JSON output and has a UE_LOG call -- it could also benefit from a similar refactoring pass if consistency is desired. -- Could consider adding output that lists the new node's FormatName in a way that makes it easy for the caller to immediately connect pins on the duplicates (e.g., emitting the full path including the new node name). diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_FindAssetReferences.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_FindAssetReferences.Notes.md deleted file mode 100644 index 6f9f1f21..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_FindAssetReferences.Notes.md +++ /dev/null @@ -1,32 +0,0 @@ -# UMCPHandler_FindAssetReferences - Refactoring Notes - -## What Changed - -- **Switched from JSON to plain-text output.** The old handler used the `Handle(Json, FJsonObject*)` signature and built a JSON tree with counts and arrays. The new handler uses `Handle(Json, FStringBuilderBase&)` and emits one line per referencer in `ClassName /Package/Path` format, matching the pattern used by SearchAssets. - -- **Removed the all-blueprints scan.** The old code loaded the entire MCPAssets index just to classify referencers as "blueprint" vs "other." This was expensive and the categorization wasn't particularly useful to an LLM caller. The new code instead looks up each referencer's asset class from the asset registry, which is cheap and gives more specific type information (e.g., "WidgetBlueprint" vs "AnimBlueprint" vs "Material" rather than just "blueprint" or "other"). - -- **Added asset existence validation.** The old handler silently returned empty results if the asset path was wrong. The new handler checks the asset registry and returns a clear error if the asset doesn't exist. - -- **Removed redundant count fields.** The old handler sent totalReferencers, blueprintReferencerCount, and otherReferencerCount. The LLM can count lines if it needs to. This keeps the output concise. - -- **Removed unnecessary includes.** The old handler included many K2Node headers, EdGraph headers, Blueprint headers, etc. None of these were actually used by this handler -- they were likely copy-paste artifacts. - -- **No UE_LOG calls** -- the old handler didn't have any, so nothing to remove there. - -## What's Good About This Handler - -- It's simple and focused: one asset path in, list of referencers out. -- The plain-text output is concise and easy for an LLM to parse. -- Each line includes the asset class name, which tells the caller what kind of asset each referencer is without requiring a separate lookup. -- Error handling is clear: bad asset path gets a descriptive error. - -## What Might Need More Work - -- **MCPFetcher/MCPAssetFinder not used for input validation.** The input is a raw package path passed directly to the asset registry. I considered using MCPAssetFinder with `Exact()` to resolve the asset, but `GetReferencers` takes an FName package path, and MCPAssetFinder is designed more for searching/filtering than for validating a known exact path. Using the asset registry's `GetAssetByObjectPath` directly seemed more appropriate here. If there's a preferred pattern for validating package paths, this could be updated. - -- **No batching.** This handler processes a single asset path. It could be extended to accept multiple paths, but it wasn't clear whether that would be useful -- the caller can just invoke it multiple times if needed, and reference lookups are fast. - -- **GetReferencers returns package-level dependencies.** This is an Unreal asset registry limitation: it shows that package A references package B, but doesn't pinpoint which node or property holds the reference. A more detailed handler could load the blueprints and walk their graphs to find exact usage locations, but that would be a significantly different tool. - -- **The FSoftObjectPath constructor.** I used `FSoftObjectPath(AssetPath)` for the existence check. This works for standard `/Game/...` paths but I'm not 100% certain it handles all edge cases (e.g., paths with subobjects). If validation fails for valid paths, this would be the place to investigate. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_FindMaterialReferences.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_FindMaterialReferences.Notes.md deleted file mode 100644 index 2c8d46e3..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_FindMaterialReferences.Notes.md +++ /dev/null @@ -1,28 +0,0 @@ -# UMCPHandler_FindMaterialReferences - Refactoring Notes - -## What was changed - -- **Switched to plain-text output.** Changed from `Handle(Json, FJsonObject*)` to `Handle(Json, FStringBuilderBase&)`. The JSON response was just a list of package paths plus a count -- plain text is more concise and token-efficient. - -- **Removed UE_LOG call.** The caller can't see log messages, so it was pointless. - -- **Classified referencers by asset type.** Each line now shows the asset class (via `FormatName(GetClass())`) followed by the package path, matching the style of FindAssetReferences. Previously only bare package paths were returned. - -- **Filtered self-references.** Preserved the existing self-reference skip. - -- **Trimmed includes.** Removed ~20 unused material/graph includes that were copy-pasted from other handlers. - -- **Errors go to the output device.** MCPAssets already sends errors to `Result` via `.Errors(Result)`, so no manual error formatting is needed. - -## What is good about this handler - -- Uses `MCPAssets` with `.Exact()` and dual `.Scan<>()` to cleanly find both `UMaterial` and `UMaterialInstanceConstant` in one search, with `.ENone().ETwo()` catching ambiguity and missing-asset cases automatically. -- Output is concise: one line per referencer, easy for an LLM to parse. - -## Areas for possible future work - -- **FormatName for referencers.** Currently using `FormatName(GetClass())` for the class name and raw `PackageName` for the path. If `FormatName` gains an overload for `FAssetData`, that would be preferable. The FindAssetReferences example handler uses the same approach, so this is consistent for now. - -- **Batch support.** This handler takes a single material. It could accept a comma-separated list or a JSON array of materials to check in one call, reducing round-trips. However, this is a read-only query handler where batch support is less critical than for mutation handlers. - -- **Recursive references.** Currently only finds direct referencers. A `Recursive` boolean parameter could optionally walk the full dependency chain. Not sure if that's needed. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_GetNodeComment.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_GetNodeComment.Notes.md deleted file mode 100644 index 492b87e4..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_GetNodeComment.Notes.md +++ /dev/null @@ -1,30 +0,0 @@ -# UMCPHandler_GetNodeComment - Refactoring Notes - -## What Changed - -- **Switched from JSON to plain-text output.** The handler now overrides `Handle(Json, FStringBuilderBase&)` instead of `Handle(Json, FJsonObject*)`. The output is three simple lines: node name, comment text, and bubble visibility. - -- **MCPFetcher now receives the FStringBuilderBase.** Previously it received `Result` (a `FJsonObject*`); now it receives the string builder, so errors go directly into the plain-text response. - -- **Added MCPUtils::FormatName for the node.** The output now identifies the node using the standard naming scheme, so the caller can confirm which node was queried. - -- **Improved the example path in the description.** Changed from `/Game/Foo,node:MyNode` to `/Game/Foo,graph:EventGraph,node:MyNode` which is a more realistic path (nodes live inside graphs). - -- **Removed the leading blank line** inside the Handle method (minor cleanup matching the style of the example handlers). - -## What's Good - -- The handler is simple and focused - it does one thing well. -- It already used MCPFetcher for object resolution, which is correct. -- The plain-text output is minimal and token-efficient for LLM consumption. -- No UE_LOG calls were present, so none needed removal. - -## Areas of Uncertainty - -- **Whether to include the node name in output.** The coding standards say "don't echo the command parameters," but the node name shown via FormatName may differ from what the caller sent (e.g. they sent a partial match). I included it so the caller can confirm the resolved node. This could be removed if considered too verbose. - -- **Batching.** The coding standards encourage batch operations. This handler could be extended to accept multiple node paths and return comments for all of them. I did not make that change since it would alter the tool's interface and wasn't explicitly requested, but it's worth considering. - -## No Further Issues - -The handler is straightforward and the refactoring was clean. No other concerns. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_GetPinDetails.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_GetPinDetails.Notes.md deleted file mode 100644 index e9246ed7..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_GetPinDetails.Notes.md +++ /dev/null @@ -1,27 +0,0 @@ -# UMCPHandler_GetPinDetails — Refactoring Notes - -## What changed - -- **Plain-text output.** Switched from `FJsonObject*` to `FStringBuilderBase&`. The output is now compact key-value lines instead of a JSON tree. - -- **MCPUtils::FormatPinType.** Replaced the hand-rolled type output (PinCategory, PinSubCategory, PinSubCategoryObject as separate fields) with a single call to `MCPUtils::FormatPinType(P)`. This gives a consistent type string used across all handlers. - -- **Container flags consolidated.** Instead of always emitting `isArray: false`, `isSet: false`, `isMap: false`, the handler only emits a `Container:` line when the pin is a container type. Similarly, `IsReference` and `IsConst` are only emitted when true. - -- **Concise connection format.** Connections are now one line each: `Connection: NodeName :: PinName` instead of a JSON array with nodeId/pinName/nodeTitle fields. The nodeGuid is no longer emitted since MCPUtils::FormatName for nodes already produces a unique identifier. - -- **Removed EdGraphSchema_K2.h include.** It was unused. - -- **Error handling.** MCPFetcher is constructed with `Result` (the FStringBuilderBase), so errors from path resolution go directly to the caller. - -- **No UE_LOG calls.** The original had none, so nothing to remove. - -## What's good - -- The handler was already using MCPFetcher and MCPUtils::FormatName, so the structure was sound. The main issue was JSON output and verbose type decomposition. - -## Uncertainties - -- **DefaultObject path.** I kept `GetPathName()` for `DefaultObject` since this is a UObject reference and the caller may need the full path to use it elsewhere. FormatName doesn't have an overload for arbitrary UObject, and the path is the most actionable identifier. If there's a preferred way to format this, it could be revisited. - -- **PinSubCategory loss.** The old code emitted PinSubCategory separately. FormatPinType doesn't include it (it favors PinSubCategoryObject when present, otherwise PinCategory). If callers need the sub-category string, it would need to be added back. In practice, the sub-category is rarely meaningful on its own. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ListAnimSlotNames.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ListAnimSlotNames.Notes.md deleted file mode 100644 index 2b9b5b53..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ListAnimSlotNames.Notes.md +++ /dev/null @@ -1,24 +0,0 @@ -# UMCPHandler_ListAnimSlotNames - Refactoring Notes - -## Changes Made - -- **Plain-text output**: Switched from JSON output (`Handle` with `FJsonObject*`) to plain-text output (`Handle` with `FStringBuilderBase&`). Each slot name is now one line. Removed the `count` field -- the caller can count the lines. - -- **MCPAssetFinder error handling**: The handler already used `MCPAssets` correctly. Removed the manual `Blueprint.IsEmpty()` check and `MakeErrorJson` call -- `MCPAssets::Exact` with `.ENone()` already handles the empty-string case by reporting "no assets found." - -- **Removed unnecessary includes**: Stripped out headers that weren't needed (`EdGraph/EdGraph.h`, `EdGraph/EdGraphNode.h`, `Kismet2/KismetEditorUtilities.h`, `Dom/JsonValue.h`, `Animation/AnimBlueprintGeneratedClass.h`, `Animation/Skeleton.h`, `Animation/AnimSequence.h`, `Animation/BlendSpace.h`, `Engine/Blueprint.h`). - -- **No UE_LOG calls**: The original had none, so nothing to remove. - -## What's Good - -- The use of `MCPAssets` with `.Exact().Errors().ENone().ETwo().Load()` is clean and idiomatic. -- The reflection-based slot name discovery is a reasonable approach for finding slot names across different anim node types. - -## Areas of Uncertainty - -- **FormatName/Identifies not applicable here**: The handler outputs raw slot name strings (e.g. "DefaultSlot", "UpperBody"). These are Unreal slot names, not objects that `MCPUtils::FormatName` has overloads for. There's no `FormatName(FName SlotName)` or similar. I left them as raw strings since that's what they are -- string identifiers defined in the skeleton's slot groups, not UObjects. - -- **Reflection-based slot discovery**: The handler searches for any `FNameProperty` whose name contains "SlotName" or "Slot". This is a heuristic -- it could pick up properties that aren't actually animation slot names, or miss slot names stored in non-obvious ways. A more targeted approach would check for specific node types (e.g. `UAnimGraphNode_Slot`) and read their known properties directly. However, the broad approach may be intentional to catch slots across different node types, so I left it as-is. - -- **Batching**: This handler already operates on one blueprint and returns all slots. There's no obvious batching opportunity (listing slots across multiple blueprints seems unlikely to be needed). diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ListAnimSyncGroups.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ListAnimSyncGroups.Notes.md deleted file mode 100644 index 0b5b8989..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ListAnimSyncGroups.Notes.md +++ /dev/null @@ -1,23 +0,0 @@ -# UMCPHandler_ListAnimSyncGroups - Refactoring Notes - -## What was done - -- **Plain-text output**: Switched from JSON response (`Handle` with `FJsonObject* Result`) to plain-text (`Handle` with `FStringBuilderBase& Result`). Each sync group name is now one line of output. -- **MCPFetcher**: Replaced the manual `Blueprint.IsEmpty()` check and `MCPAssets` lookup with `MCPFetcher`. Error handling for missing/invalid paths is now automatic. -- **FormatName/Identifies**: Not directly applicable here since the handler doesn't output object names or match against them -- it outputs raw sync group name strings from FName properties. -- **Removed UE_LOG**: None were present. -- **Removed count field**: The JSON response had a `count` field which is unnecessary -- the caller can count the lines. -- **Trimmed includes**: Removed unused headers (EdGraph, EdGraphNode, KismetEditorUtilities, JsonValue, AnimBlueprintGeneratedClass, Skeleton, AnimSequence, BlendSpace, MCPAssetFinder, Engine/Blueprint). -- **Renamed parameter**: `Blueprint` -> `Path` for consistency with other handlers. -- **Added empty-result message**: "No sync groups found." for the zero-result case, matching the pattern in ListEventDispatchers. - -## What's good - -- The core logic (reflection-based search for SyncGroup/GroupName properties on anim nodes) is reasonable and hard to improve on -- there's no single Unreal API that enumerates sync groups from an AnimBlueprint. -- The handler is small and focused. - -## Concerns / areas for further work - -- **Reflection heuristic**: The handler finds sync groups by scanning all `FNameProperty` fields whose name contains "SyncGroup" or "GroupName". This is a heuristic -- it could pick up unrelated properties that happen to match those substrings, or miss sync group properties that use a different naming convention. I left this logic as-is since I don't know enough about the AnimGraph node class hierarchy to improve it confidently. -- **Name-based lookup lost**: The old handler accepted both asset names and paths via `MCPAssets.Exact()`. The new handler uses `MCPFetcher.Walk()` which only accepts package paths. This is consistent with the example handler (ListEventDispatchers) and the coding standards' emphasis on MCPFetcher, but callers who relied on name-only lookup will need to use full paths now. -- **Sync group names are raw strings**: The sync group names come from `FName` properties and aren't objects, so `FormatName`/`Identifies` don't apply to them. If sync groups ever need to be used as identifiers by other tools, a naming convention might need to be established. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ListBlueprintAssets.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ListBlueprintAssets.Notes.md deleted file mode 100644 index 6d981e72..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ListBlueprintAssets.Notes.md +++ /dev/null @@ -1,26 +0,0 @@ -# UMCPHandler_ListBlueprintAssets - Refactoring Notes - -## Changes Made - -The handler was already well-refactored when I encountered it. No code changes were needed. - -## What Looks Good - -- **Plain-text output**: Already uses `Handle(Json, FStringBuilderBase&)`. -- **MCPAssets usage**: Uses `MCPAssets` with `NoScans()` and explicit `Scan()` / `Scan()`, which is the correct pattern since UWorld is not a subclass of UBlueprint. -- **Error reporting**: `.Errors(Result)` passes errors directly to the output device. -- **No UE_LOG calls**: Clean. -- **Concise output**: One line per asset with parent class and package name, columnar format. - -## Areas of Uncertainty - -- **FormatName not used for asset output**: The coding standards require `MCPUtils::FormatName` for object names. However, there is no `FormatName(FAssetData)` overload, and this handler only calls `Info()` (not `Load()`), so it never has loaded UObjects. Using `PackageName.ToString()` is the established pattern for Info-only listing handlers (same as ListMaterialAssets). Loading every asset just to call FormatName would be expensive for a search/listing tool. - -- **Parent class extraction from asset tags**: The handler extracts the parent class name from the `ParentClass` asset tag via string manipulation (finding last dot, removing trailing quote). This is hand-rolled but unavoidable: the parent class info is stored as a serialized class path in asset metadata, and there's no MCPUtils or MCPAssetFinder helper for this. The extraction logic is correct for the tag format Unreal uses (`/Script/Engine.Actor'` becomes `Actor`). - -- **Limit hardcoded to 500**: The `Limit(500)` call is hardcoded rather than exposed as a parameter. This is reasonable for a listing tool but could be made configurable if callers need more or fewer results. - -## Potential Future Work - -- Could expose a `Limit` parameter to let callers control result count. -- If `MCPUtils::FormatName(FAssetData)` is ever added, the output should switch to it for consistency. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ListBlueprintComponents.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ListBlueprintComponents.Notes.md deleted file mode 100644 index ce3a013a..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ListBlueprintComponents.Notes.md +++ /dev/null @@ -1,27 +0,0 @@ -# UMCPHandler_ListBlueprintComponents - Refactoring Notes - -## What was done - -- **Switched to plain-text output.** The handler now uses `FStringBuilderBase` instead of `FJsonObject`. The output is a tree showing component hierarchy with indentation, which is more compact and readable for an LLM than the old flat JSON array. - -- **Switched from MCPAssets to MCPFetcher.** The parameter was renamed from `Blueprint` to `Path` to match the MCPFetcher convention (as seen in DumpGraphs). MCPFetcher handles asset loading and error reporting in one step. - -- **Used FormatName consistently.** Component names use `FormatName(Node->ComponentTemplate)` and class names use `FormatName(Node->ComponentClass)`, matching the pattern used in DumpBlueprint and AddBlueprintComponent handlers. - -- **Removed unnecessary includes.** `Kismet2/BlueprintEditorUtils.h`, `UObject/UObjectIterator.h`, `Components/ActorComponent.h`, and `MCPAssetFinder.h` were not needed and were removed. - -- **Tree output instead of flat list.** The old handler emitted a flat array with `parentComponent` fields and `childCount`. The new handler emits an indented tree, which is more natural for an LLM to read and uses fewer tokens. - -## What's good - -- The handler is simple and focused -- it does one thing well. -- Error handling is clean: MCPFetcher reports path errors, `Cast` reports type mismatches, and there's a clear error for non-Actor blueprints. -- The tree output naturally shows parent-child relationships without redundant fields. - -## Areas of uncertainty - -- **FormatName for ComponentTemplate vs variable name.** The old code used `Node->GetVariableName().ToString()` for the component name. I switched to `MCPUtils::FormatName(Node->ComponentTemplate)` for consistency with the coding standards (always use FormatName). Looking at MCPUtils.cpp, `FormatName(UActorComponent*)` calls `GetName()`, which should produce the same result as the variable name in most cases. However, if there are edge cases where `GetVariableName()` differs from `ComponentTemplate->GetName()`, the output could change. I went with FormatName per the standards. - -- **Orphan nodes.** The old handler iterated `GetAllNodes()` which includes nodes not reachable from root nodes (orphans). The new tree-based approach only shows nodes reachable from `GetRootNodes()`. If orphan SCS nodes exist in practice, they would be silently omitted. This seems acceptable since orphan components are unusual and potentially broken state, but worth noting. - -- **Parameter rename from `Blueprint` to `Path`.** This changes the MCP tool's parameter name, which could break existing callers. The new name is consistent with other MCPFetcher-based handlers (DumpGraphs, etc.), but any scripts or prompts that pass `Blueprint=` will need updating. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ListBlueprintInterfaces.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ListBlueprintInterfaces.Notes.md deleted file mode 100644 index 1770786c..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ListBlueprintInterfaces.Notes.md +++ /dev/null @@ -1,25 +0,0 @@ -# UMCPHandler_ListBlueprintInterfaces - Refactoring Notes - -## What was changed - -- **Switched from JSON to plain-text output.** The old handler used `Handle(Json, FJsonObject* Result)` and built a JSON tree with nested arrays. The new handler uses `Handle(Json, FStringBuilderBase& Result)` and emits concise plain text. This is a read-only listing tool, so plain text is a natural fit. - -- **Switched from MCPAssets to MCPFetcher.** The old code used `MCPAssets` with `Exact()` to find the blueprint. MCPFetcher is more appropriate here since we're resolving a single path to a single object, not searching. It also gives the caller the ability to use walker syntax (e.g. for level blueprints). - -- **Renamed the parameter from `Blueprint` to `Path`.** This is consistent with other handlers like DumpGraphs that use MCPFetcher. The description clarifies the expected format. - -- **Removed unnecessary includes.** `MCPServer.h`, `Kismet2/BlueprintEditorUtils.h`, `UObject/UObjectIterator.h`, `EdGraph/EdGraph.h`, and `MCPAssetFinder.h` were not needed. - -- **Removed `classPath` from output.** The old JSON included both `name` and `classPath` for each interface. The class path is redundant information the caller didn't ask for; `FormatName` on the UClass is sufficient to identify the interface. - -## What looks good - -- The handler is simple and focused -- it does one thing clearly. -- The iteration over `ImplementedInterfaces` and null checks are straightforward. -- Uses `MCPUtils::FormatName` for both interface classes and graph names, ensuring consistent naming. - -## Areas of uncertainty - -- **FormatName on UClass for interfaces.** The old code called `MCPUtils::FormatName(IfaceDesc.Interface)` where `IfaceDesc.Interface` is a `UClass*`. I kept this usage since there is a `FormatName(const UClass*)` overload. However, I did not verify whether the output is ideal for interface classes specifically (e.g. whether it strips the `U`/`I` prefix or keeps the full class name). If the FormatName output for interface classes turns out to be unclear, that would need a fix in MCPUtils, not here. - -- **No classPath in output.** I removed the `classPath` field to keep output concise. If callers actually need the full path to resolve or add interfaces, this may need to be added back. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ListClassProperties.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ListClassProperties.Notes.md deleted file mode 100644 index 4e555e87..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ListClassProperties.Notes.md +++ /dev/null @@ -1,31 +0,0 @@ -# UMCPHandler_ListClassProperties — Refactoring Notes - -## Changes Made - -1. **Plain-text output**: Converted from JSON response (`Handle` with `FJsonObject*` result) to plain-text response (`Handle` with `FStringBuilderBase&` result). Output is now compact lines like ` integer Health [AActor] (BlueprintVisible EditAnywhere)`. - -2. **FormatName/Identifies**: Already used `MCPUtils::FormatName` for `OwnerClass`; kept that. The class lookup was hand-rolled with a `TObjectIterator` loop — replaced with `MCPUtils::FindClassByName`, which does the same thing but also handles case-insensitive fallback. - -3. **FormatPropertyType instead of GetCPPType**: Replaced `Prop->GetCPPType()` with `MCPUtils::FormatPropertyType(Prop)`, which produces friendlier type names (e.g., "string" instead of "FString", "boolean" instead of "bool"). - -4. **Removed UE_LOG**: No UE_LOG calls were present in this handler, so nothing to remove. - -5. **Concise output**: The owning class is only shown when it differs from the queried class (i.e., inherited properties). Flags are shown inline in parentheses rather than as a separate JSON array. The `count` and `className` echo fields are gone — the header line shows the resolved class name, and the caller can count lines. - -6. **Removed unnecessary includes**: Dropped includes for `EdGraph`, `EdGraphNode`, `EdGraphPin`, `EdGraphSchema_K2`, `UObjectIterator`, `Blueprint`, and `MCPAssetFinder` — none were needed after the refactor. - -## What's Good - -- The handler is straightforward and does one thing well. -- The property flag list is useful and covers the most important flags. -- The optional `Filter` parameter is a good feature for narrowing results on large classes. - -## Areas for Further Consideration - -- **MCPFetcher not used**: This handler takes a class name string, not a path. MCPFetcher is path-oriented (package paths with walker segments), so it doesn't apply here. `MCPUtils::FindClassByName` is the right tool for this case. - -- **No batching opportunity**: This handler already returns all properties at once, so batching doesn't apply. - -- **Flags list is hardcoded**: If new important flags need to be tracked, the list must be updated manually. This is a minor maintenance concern but the current set covers the practical cases well. - -- **OwnerClass display**: I chose to only show `[OwnerClass]` for inherited properties (where `OwnerClass != FoundClass`). This keeps output compact for the common case. If the caller wants to always see it, this could be made optional. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ListEventDispatchers.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ListEventDispatchers.Notes.md deleted file mode 100644 index 410d369e..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ListEventDispatchers.Notes.md +++ /dev/null @@ -1,55 +0,0 @@ -# UMCPHandler_ListEventDispatchers Refactoring Notes - -## What Changed - -- **Switched from JSON to plain-text output.** The handler now uses - `Handle(Json, FStringBuilderBase& Result)` instead of the JSON - overload. Output is one line per dispatcher in the form - `DispatcherName(Type Param, Type Param)`. - -- **Switched from MCPAssets to MCPFetcher for blueprint lookup.** - The old code used `MCPAssets` with Exact/ENone/ETwo, - which is designed for asset searches. Since we have a single - path to resolve, MCPFetcher is the right tool. It also gives - us free support for walker segments in the path. - -- **Renamed parameter from `Blueprint` to `Path`.** This is - consistent with how other fetcher-based handlers name their - parameter (see DumpGraphs). - -- **Used MCPUtils::FormatPinType instead of hand-rolled type - formatting.** The old code built type strings from - `PinCategory` and `PinSubCategoryObject` manually. FormatPinType - handles all the edge cases (containers, references, etc.) - and is the canonical way to format types. - -- **Removed the count field and "dispatchers" wrapper.** Plain-text - output doesn't need these. The caller can count the lines. - -- **Added a "No event dispatchers found" message** for the empty - case, so the caller gets clear feedback. - -## What's Good - -- The core logic (GetDelegateNameList, signature graph lookup, - iterating UserDefinedPins) is correct and well-structured. -- No UE_LOG calls were present -- nothing to remove. -- The nesting depth is acceptable. - -## Uncertainties / Potential Issues - -- **FormatName not used for dispatcher names.** There is no - `FormatName` overload for delegates/event dispatchers (they're - just `FName` values from `GetDelegateNameList`). I used - `DelegateName.ToString()` directly. If a FormatName overload - for delegates is added later, this should be updated. - -- **Parameter name formatting.** Similarly, `PinInfo->PinName` - is output via `.ToString()`. There's no FormatName overload - for `FUserPinInfo`. This seems fine since these are - user-defined parameter names, not graph objects. - -- **Iteration order.** `TSet` has no guaranteed order. - If deterministic output ordering matters, the names should be - sorted. I left it as-is since the original code also used - unsorted iteration. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RefreshAllNodesInGraph.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RefreshAllNodesInGraph.Notes.md deleted file mode 100644 index 0726962d..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RefreshAllNodesInGraph.Notes.md +++ /dev/null @@ -1,20 +0,0 @@ -# UMCPHandler_RefreshAllNodesInGraph - Refactoring Notes - -## What's Good - -- Uses `MCPAssets` with `.Exact().Errors().ENone().ETwo().Load()` for clean asset resolution with automatic error reporting. -- All naming goes through `MCPUtils::FormatName()` (for the blueprint, graphs, and nodes). -- Plain-text output via `FStringBuilderBase&` — no JSON overhead. -- No `UE_LOG` calls; all diagnostics are reported through the response. -- Compiler warnings/errors are reported with graph and node context, which is useful for the caller. -- Output is concise: single summary line plus any compiler messages. - -## Uncertainties / Conservative Choices - -- **Orphaned pin removal**: The handler removes orphaned pins by manually iterating `Node->Pins` in reverse and calling `BreakAllPinLinks()` + `RemoveAt()`. This works but bypasses any engine-provided cleanup. I left this as-is because there's no standard Unreal API that cleanly removes orphaned pins in bulk, and the existing approach is straightforward. -- **Double iteration of AllNodes**: `MCPUtils::AllNodes(BP)` is called three times — once for the initial count, once for orphan removal, and once for compiler messages. This is fine for typical blueprint sizes but could be consolidated if performance ever matters. -- **Parameter naming**: The parameter is `Blueprint` (asset name/path). An alternative would be to accept a full MCPFetcher path, but since this operates on the entire blueprint (not a specific graph or node), `MCPAssetFinder` is the right tool here. - -## No Further Work Needed - -The handler is fully converted to the coding standards. All goals from the checklist are met. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RemoveAnimStateFromMachine.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RemoveAnimStateFromMachine.Notes.md deleted file mode 100644 index 45395f47..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RemoveAnimStateFromMachine.Notes.md +++ /dev/null @@ -1,26 +0,0 @@ -# UMCPHandler_RemoveAnimStateFromMachine - Refactoring Notes - -## What was done - -- **MCPFetcher**: Replaced the two-parameter approach (Blueprint + Graph) with a single `Path` parameter that uses MCPFetcher to walk to the state machine graph directly. This is consistent with how DumpGraphs and other handlers work. -- **Plain-text output**: Switched from `Handle(Json, FJsonObject*)` to `Handle(Json, FStringBuilderBase&)`. Output is now a single concise line: "Removed state X and N transition(s)." -- **FormatName**: The state node name in the output uses `MCPUtils::FormatName(StateNode)` instead of echoing back the raw StateName parameter. -- **MCPErrorCallback**: MCPFetcher and FindStateByName both accept `Result` (the FStringBuilderBase&) directly, so errors flow to the caller automatically. -- **Removed UE_LOG**: There were none in this handler, so nothing to remove. -- **Reduced nesting**: The transition-collection loop now uses early-continue instead of nested if-blocks. -- **Trimmed includes**: Removed unused headers (EdGraphPin.h, AnimSequence.h, BlendSpace.h, AnimGraphNode_SequencePlayer.h, AnimGraphNode_BlendSpacePlayer.h, K2Node_VariableGet.h, MCPAssetFinder.h). - -## What is good about this handler - -- The core logic is straightforward and correct: find state, collect connected transitions, break links, remove nodes, compile, save. -- It properly breaks all node links before removing nodes, preventing dangling references. - -## Areas of uncertainty / conservatism - -- **Owning blueprint discovery**: I used `SMGraph->GetOuter()->GetOuter()` to walk from the state machine graph up to the owning blueprint. This assumes the hierarchy is StateMachineGraph -> OwnerNode's Graph -> Blueprint, which is the standard UE layout. If the hierarchy is ever different, this would break. An alternative would be to require the caller to provide the blueprint path and use MCPFetcher to walk down to it, but the current approach keeps the parameter list minimal. -- **FormatName on removed node**: After `SMGraph->RemoveNode(StateNode)`, the StateNode object still exists in memory (just detached from the graph), so `FormatName(StateNode)` should still work. However, if FormatName relies on the node's graph membership, the output could be unexpected. I kept it because the node object is still valid at that point. - -## Possible further work - -- **Batch support**: Could accept an array of state names to remove multiple states in one call. This would be more efficient for LLM callers doing bulk cleanup. -- **MCPFetcher walker for state machine states**: If MCPFetcher gained a `State:` walker segment, the handler could accept a single path all the way to the state node, eliminating the StateName parameter entirely and making the interface fully path-based. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RemoveBlueprintComponent.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RemoveBlueprintComponent.Notes.md deleted file mode 100644 index 30839113..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RemoveBlueprintComponent.Notes.md +++ /dev/null @@ -1,29 +0,0 @@ -# UMCPHandler_RemoveBlueprintComponent — Refactoring Notes - -## What was changed - -1. **Switched from JSON to plain-text output.** The handler now overrides `Handle(Json, FStringBuilderBase&)` instead of `Handle(Json, FJsonObject*)`. This produces more concise output for LLM consumption. - -2. **Replaced MCPAssets with MCPFetcher for blueprint lookup.** The old code used `MCPAssets` with `Exact()` matching. The new code uses `MCPFetcher::Walk()`, which is more concise and consistent with other handlers (matches the RemoveBlueprintVariable example). - -3. **Replaced hand-rolled name comparison with MCPUtils::Identifies.** The old code did `Node->GetVariableName().ToString().Equals(Component, ESearchCase::IgnoreCase)`. The new code uses `MCPUtils::Identifies(Component, Node->ComponentTemplate)` for consistent name matching across all handlers. - -4. **Replaced hand-rolled name formatting with MCPUtils::FormatName.** The old code used `Node->GetVariableName().ToString()` and raw `*Blueprint`/`*Component` strings in output. The new code uses `MCPUtils::FormatName(Node->ComponentTemplate)` everywhere. - -5. **Removed all UE_LOG calls.** Two `UE_LOG(LogTemp, ...)` calls were removed. Errors and status are reported through the response. - -6. **Made output more concise.** Removed echo of command parameters from success/error messages (the caller knows what it sent). Removed the JSON `saved` field; save failures are reported inline as a warning, matching the RemoveBlueprintVariable pattern. - -7. **Removed unused includes.** Dropped `MCPAssetFinder.h`, `Components/ActorComponent.h`, and `UObject/UObjectIterator.h` since they are no longer needed. - -## What looks good - -- The root-component-with-children guard is a solid safety check and was preserved as-is. -- The `RemoveNodeAndPromoteChildren` call is the right API — it handles non-root nodes with children gracefully. -- The error message listing available components helps the caller recover from typos. - -## Areas of uncertainty - -- **Identifies on ComponentTemplate vs SCS_Node.** There is no `MCPUtils::Identifies` overload for `USCS_Node*`, so I used `Identifies(Component, Node->ComponentTemplate)` which takes a `UActorComponent*`. This should work since `FormatName(UActorComponent*)` exists, but I have not verified that the ComponentTemplate pointer is always non-null for valid SCS nodes. I added a null check as a guard. - -- **Batch support.** The coding standards mention preferring batch operations where it makes sense. This handler currently removes one component at a time. Removing multiple components in one call could be useful, but would add complexity around the root-with-children guard (order of removal matters). I left it as single-component for now. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RemoveBlueprintInterface.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RemoveBlueprintInterface.Notes.md deleted file mode 100644 index 274e3369..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RemoveBlueprintInterface.Notes.md +++ /dev/null @@ -1,24 +0,0 @@ -# UMCPHandler_RemoveBlueprintInterface - Refactoring Notes - -## Status: Complete - -This handler was already mostly refactored by a previous pass. The final version is clean and follows all coding standards. - -## What's Good - -- Uses `MCPAssets` with `.Exact().Errors().ENone().ETwo().Load()` for asset resolution -- concise and correct. -- Uses `MCPUtils::Identifies()` for interface name matching instead of hand-rolled string comparison. -- Uses `MCPUtils::FormatName()` for all object names in output. -- Plain-text output via `FStringBuilderBase&`. -- No `UE_LOG` calls. -- Error message lists all implemented interfaces when the requested one isn't found, which is helpful for the caller. -- Output is concise: just confirms what was removed, plus a note about preserved functions if applicable. - -## Uncertainties / Conservative Choices - -- The error path builds its message in two steps: first `MCPErrorCallback::SetError()` for the prefix, then appending interface names directly to `Result`. This works because `MCPErrorCallback(FStringBuilderBase&)` writes to the same builder, but it's a slightly unusual pattern -- the error message and the list are concatenated into the same stream. It reads fine, so I left it as-is. -- `FBlueprintEditorUtils::RemoveInterface` is called with `PreserveFunctions` but we don't verify the result or check for edge cases (e.g., what happens if the interface has no function graphs). The Unreal API doesn't return a success/failure indicator here, so there's not much we can do. - -## No Further Work Needed - -The handler is small, focused, and follows all the coding standards. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RemoveBlueprintVariable.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RemoveBlueprintVariable.Notes.md deleted file mode 100644 index 2a3f1b3f..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RemoveBlueprintVariable.Notes.md +++ /dev/null @@ -1,29 +0,0 @@ -# UMCPHandler_RemoveBlueprintVariable - Refactoring Notes - -## What was done - -- **Switched to plain-text output.** Changed from `Handle(Json, FJsonObject*)` to `Handle(Json, FStringBuilderBase&)`. The output is simple success/failure text, no structured data needed. - -- **Switched from MCPAssetFinder to MCPFetcher.** The handler takes a single blueprint by name or path, which is exactly what MCPFetcher excels at. MCPAssetFinder is more appropriate for search/discovery scenarios. - -- **Used FormatName for output.** All object names in output (blueprint name, variable names) now use `MCPUtils::FormatName()` for consistency. - -- **Removed UE_LOG calls.** All status information goes through the response text. - -- **Removed unused includes.** EdGraph.h, EdGraphPin.h, K2Node_VariableGet.h, K2Node_VariableSet.h, and MCPAssetFinder.h were not needed. - -- **Concise output.** Success case is a single line. Error case lists available variables to help the caller self-correct. - -## What's good - -- The handler is simple and focused: one variable, one blueprint. -- The error message listing available variables is genuinely helpful for LLM callers. -- RemoveMemberVariable handles cleanup of Get/Set nodes automatically. - -## Areas of uncertainty - -- **Variable name matching.** I used a direct string comparison against `FormatName(Var)` plus a fallback case-insensitive match on `VarName`. There is no `MCPUtils::Identifies` overload for `FBPVariableDescription`, so I couldn't use the standard Identifies pattern. If one is added later, this should be updated to use it. - -## Possible future work - -- **Batch support.** This handler removes one variable at a time. A batch version accepting an array of variable names could reduce round trips for LLMs that need to remove several variables. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RemoveFunctionParameter.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RemoveFunctionParameter.Notes.md deleted file mode 100644 index 1fe8914d..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RemoveFunctionParameter.Notes.md +++ /dev/null @@ -1,21 +0,0 @@ -# UMCPHandler_RemoveFunctionParameter - Refactoring Notes - -## Changes Made - -- **Plain-text output**: Converted from `Handle(Json, FJsonObject*)` to `Handle(Json, FStringBuilderBase&)`. -- **MCPAssetFinder**: Already used `MCPAssets` — updated to pass `Result` (FStringBuilderBase&) to `Errors()` instead of `Result` (FJsonObject*). -- **FormatName**: Error messages and output now use `MCPUtils::FormatName()` for node and graph names, replacing ad-hoc `NodeGuid.ToString()` and hand-built strings. -- **Removed UE_LOG calls**: Two `UE_LOG(LogTemp, ...)` calls removed. -- **Concise output**: Success output is a single line. Error output lists available options without JSON wrapper overhead. No longer echoes `nodeType`, `nodeId`, or `saved=true` on success — the caller knows what it asked for. -- **Removed unused includes**: `Engine/Blueprint.h`, `EdGraph/EdGraph.h`, `EdGraph/EdGraphPin.h` removed (covered by MCPFetcher.h / MCPAssetFinder.h transitive includes). - -## What's Good - -- The handler was already well-structured: MCPAssets for asset lookup, MCPUtils::Identifies for matching, MCPUtils::AllNodes for iteration. These were preserved. -- Error paths list available functions/events/parameters, which is very helpful for LLM callers. - -## Uncertainties / Conservative Choices - -- **Custom event matching**: The original code matches custom events by comparing `CustomFunctionName` with a case-insensitive string compare, rather than using `MCPUtils::Identifies`. I preserved this because `MCPUtils::Identifies` takes typed objects (UEdGraph*, UEdGraphNode*, etc.) and there isn't an overload for matching a custom event by its `CustomFunctionName` field. If `MCPUtils::FormatName(UK2Node_CustomEvent*)` produces output based on the `CustomFunctionName`, then `MCPUtils::Identifies` on the node itself might work — but I wasn't confident enough to change it without testing. -- **MCPFetcher not used for initial navigation**: MCPFetcher's `Walk()` could resolve the blueprint, but the handler needs to search across *all* graphs for a function entry or custom event by name, which is a scan operation rather than a path-walk. MCPAssets + AllNodes is the right tool here. -- **No batching**: This handler removes a single parameter. Batching (removing multiple parameters at once) could be useful but would change the API contract, so I left it as-is. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RemoveStructField.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RemoveStructField.Notes.md deleted file mode 100644 index 6f694353..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RemoveStructField.Notes.md +++ /dev/null @@ -1,24 +0,0 @@ -# UMCPHandler_RemoveStructField - Refactoring Notes - -## What was done - -- Switched from JSON output (`FJsonObject* Result`) to plain-text output (`FStringBuilderBase& Result`). -- Replaced `MCPAssets` with `MCPFetcher` for asset lookup, matching the pattern in RemoveBlueprintVariable. -- Renamed parameter `AssetPath` to `Struct` and `Name` to `FieldName` for clarity and consistency with the RemoveBlueprintVariable example. -- Used `MCPUtils::FormatName(S)` (the `UScriptStruct` overload) for naming the struct in output. -- Made string comparisons case-insensitive, matching the spirit of `Identifies`. -- Removed all UE_LOG calls (there were none, but confirmed clean). -- Removed many unnecessary includes (EnumFactory, StructureFactory, AssetToolsModule, IAssetTools, AssetRegistryModule, IAssetRegistry, EnumEditorUtils, BlueprintEditorUtils, UserDefinedEnum). -- Made output concise: single success line, error lists only field names. - -## What is good - -- The handler is now concise and follows the same structure as RemoveBlueprintVariable. -- Error messages list available fields to help the caller recover. -- MCPFetcher handles all asset resolution and error reporting in one line. - -## Areas of uncertainty / remaining work - -- **No FormatName/Identifies for FStructVariableDescription**: Unlike `FBPVariableDescription`, there is no `MCPUtils::FormatName` or `MCPUtils::Identifies` overload for `FStructVariableDescription`. The field matching uses case-insensitive comparison on both `FriendlyName` and `VarName`, which is reasonable but not centralized. If these struct field names are used across multiple handlers, adding FormatName/Identifies overloads for `FStructVariableDescription` would be the right fix. -- **MCPFetcher vs MCPAssets for name-only lookup**: The old code used `MCPAssets.Exact()` which supports both asset names and full paths via the asset registry. MCPFetcher's `Walk` also resolves paths but I am not 100% certain it handles bare names (e.g. "MyStruct" without a path prefix) the same way MCPAssets does. If callers rely on name-only lookup, this may need verification. -- **Batch support**: This handler processes one field at a time. A batch version (removing multiple fields in one call) could be useful but was not part of this refactor. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RenameAsset.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RenameAsset.Notes.md deleted file mode 100644 index 3b20bdc2..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RenameAsset.Notes.md +++ /dev/null @@ -1,24 +0,0 @@ -# UMCPHandler_RenameAsset Refactoring Notes - -## What Changed - -- **Switched to plain-text output.** The handler now overrides `Handle(... FStringBuilderBase&)` instead of the JSON version. The output is minimal: just the new path on success, or a clear error message on failure. -- **Removed UE_LOG calls.** Two UE_LOG calls were removed. Errors are reported via the output device. -- **Removed the empty `if (bSuccess) {}` block.** This was dead code. -- **Used `MCPUtils::SplitAssetPath`** instead of hand-rolled path splitting with `FindLastChar`. This is more consistent and handles edge cases (like empty asset names) that the original code did not. -- **Added `AllContent()`** to the MCPAssets query so it can find assets outside `/Game/` if needed. -- **Trimmed unused includes.** Removed includes for headers that were not needed (BlueprintEditorUtils, SavePackage, PackageName, AssetRegistryModule, FileHelpers, Engine/Blueprint.h). -- **Improved parameter descriptions** with examples to help the LLM caller understand the expected format. - -## What's Good - -- The handler is simple and focused — it does one thing well. -- MCPAssets is used correctly: `Exact()` match, `ENone()` + `ETwo()` error conditions, with errors routed directly to the output device. -- The logic for "just a name with no slash means rename in place" is preserved and useful. - -## What Might Need More Work - -- **No FormatName/Identifies usage.** This handler doesn't look up objects by display name — it works with exact package paths, which is the correct thing for asset rename. FormatName/Identifies are not applicable here since we're dealing with package paths, not display names of nodes/pins/graphs. -- **No batch support.** The handler renames one asset at a time. Batch renaming could be useful, but it's a less common operation than batch pin connections, so this may not be worth the added complexity. -- **Error detail from RenameAssets is lost.** `IAssetTools::RenameAssets` returns a bool but doesn't provide a detailed reason for failure. The error message is necessarily generic. I'm not sure if there's a way to get more detail from Unreal's rename API. -- **No validation of NewAssetName.** We don't check for illegal characters in the new name before calling RenameAssets. Unreal will reject bad names, but a more specific error message could be helpful. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RenameBlueprintGraph.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RenameBlueprintGraph.Notes.md deleted file mode 100644 index 186bd661..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RenameBlueprintGraph.Notes.md +++ /dev/null @@ -1,24 +0,0 @@ -# UMCPHandler_RenameBlueprintGraph - Refactoring Notes - -## What changed - -- **Converted to plain-text output** (`FStringBuilderBase&` instead of `FJsonObject*`). -- **Switched to MCPFetcher** for graph resolution. The caller now passes a full path like `/Game/Foo,graph:MyFunction` in the `Graph` parameter, replacing the old separate `Blueprint` + `Graph` parameters. MCPFetcher handles asset loading and graph lookup with automatic error reporting. -- **Removed the `Blueprint` parameter.** It's now embedded in the `Graph` path, which is how other refactored handlers work (see GetNodeComment). -- **Removed all UE_LOG calls.** -- **Used MCPUtils::FormatName()** for all object names in output. -- **Concise output.** The success message is a single line. Error messages are brief. No echoing of input parameters. -- **Removed unused includes** (EdGraphNode.h, EdGraphSchema_K2.h, K2Node_CustomEvent.h, KismetEditorUtilities.h, UObjectIterator.h, MCPAssetFinder.h). Added MCPFetcher.h. - -## What's good - -- The handler is short and focused. The core logic (ubergraph check, collision check, rename, save) is preserved intact. -- MCPFetcher eliminates all the hand-rolled asset loading and graph-finding code. -- Error paths are clear and concise. - -## Uncertainties / conservative choices - -- **UbergraphPage detection:** I used `BP->UbergraphPages.Contains(TargetGraph)` instead of iterating with `MCPUtils::Identifies`. This is safe because MCPFetcher already resolved the graph to an exact pointer. The old code iterated UbergraphPages with Identifies matching, which was necessary when the graph was identified by name string -- but now MCPFetcher gives us the exact pointer. -- **Save result not reported.** The old code reported whether save succeeded. I dropped this to keep output concise. If save failures become a debugging issue, it could be added back as a warning line. -- **Graph type detection** (`bIsFunction` / `bIsMacro`): I check both arrays explicitly. If the graph is somehow in neither (e.g. a delegate graph or animation graph), the handler now rejects it. The old code would also have rejected it (it only searched FunctionGraphs and MacroGraphs), so behavior is preserved. -- **Batching not added.** Renaming multiple graphs in one call seems unlikely to be needed, so I didn't add batch support. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ReparentBlueprint.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ReparentBlueprint.Notes.md deleted file mode 100644 index 819f7223..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ReparentBlueprint.Notes.md +++ /dev/null @@ -1,22 +0,0 @@ -# UMCPHandler_ReparentBlueprint - Refactoring Notes - -## Changes Made - -- **Plain-text output**: Converted from `FJsonObject*` to `FStringBuilderBase&`. Output is now a single line like `Reparented BP: OldParent -> NewParent` plus an optional save-failure warning. -- **MCPUtils::FindClassByName**: Replaced the hand-rolled `TObjectIterator` loop with `MCPUtils::FindClassByName`, which already does exact and case-insensitive matching (including `_C` suffix for Blueprint generated classes). -- **MCPUtils::FormatName**: Already used in the original for parent class names; now also used for the blueprint name in output (via `MCPUtils::FormatName(BP)`). -- **Removed UE_LOG calls**: Three UE_LOG calls removed. -- **Concise output**: No longer echoes input parameters. Reports only the reparent result and save status (only on failure). -- **Error reporting via MCPErrorCallback(Result)**: Errors from MCPAssets go directly to the string builder; the manual "class not found" error also uses MCPErrorCallback. -- **Removed unused includes**: Dropped `EdGraph/EdGraph.h`, `EdGraph/EdGraphNode.h`, `EdGraphSchema_K2.h`, `K2Node_CustomEvent.h`, `UObject/UObjectIterator.h` which were not needed by this handler. - -## What's Good - -- The handler is straightforward: find blueprint, find parent class, reparent, compile, save. -- The two-stage class lookup (C++ first, Blueprint asset second) is a reasonable strategy since `FindClassByName` only finds already-loaded classes. - -## Uncertainties / Conservative Choices - -- **Removed the hierarchy warning**: The original had a check that warned (via UE_LOG) when reparenting to a class not in the direct hierarchy. Since UE_LOG is banned and the comment said "just warn, don't block", I removed it entirely. The reparent proceeds regardless. If this warning is valuable, it could be added back as a line in the plain-text output. -- **Blueprint parent fallback**: The MCPAssets search for Blueprint parents does not use `.ENone()` — if no Blueprint is found either, control falls through to the "Could not find class" error. This matches the original behavior where the Blueprint search was allowed to return zero results. -- **No batching**: This handler reparents a single blueprint. Batching doesn't seem natural here since reparenting is a rare, high-impact operation. Could revisit if needed. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ReparentMaterialInstance.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ReparentMaterialInstance.Notes.md deleted file mode 100644 index db91fe1a..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ReparentMaterialInstance.Notes.md +++ /dev/null @@ -1,29 +0,0 @@ -# UMCPHandler_ReparentMaterialInstance - Refactoring Notes - -## What was done - -- **Plain-text output**: Converted from JSON (`FJsonObject*`) to plain-text (`FStringBuilderBase&`). The output is simple status text, not structured data, so plain-text is the right fit. - -- **MCPAssetFinder for parent lookup**: The old code had a convoluted two-phase search (try UMaterial, then try UMaterialInstanceConstant, then fall back to LoadObject). Replaced with a single `MCPAssets` scan that covers both types in one pass, with proper error reporting via `Errors(Result)`. - -- **FormatName for all names**: Old code used `GetPathName()` for output. Now uses `MCPUtils::FormatName()` consistently for the MI, old parent, and new parent. - -- **Removed UE_LOG calls**: Two UE_LOG calls removed. Errors and status are reported via the response. - -- **Removed unused includes**: Stripped includes that were only needed by the old code (parameter expression headers, factory headers, asset tools headers). - -- **Concise output**: Reduced to a single line of output on success or dry-run, e.g. `Reparented MI_Foo: M_Bar -> M_Baz`. - -## What's good about this handler - -- The circular parent chain check is important and correctly implemented. -- DryRun mode is a nice safety feature. -- The handler is small and focused on a single task. - -## Areas of uncertainty / conservative choices - -- **FormatName on UMaterialInterface**: There is no `FormatName(UMaterialInterface*)` overload -- only `FormatName(UMaterial*)` and `FormatName(UMaterialInstance*)`. Added a private `NameOf(UMaterialInterface*)` helper that casts to the correct concrete type. This is used for both the old parent and new parent names. If a `FormatName(UMaterialInterface*)` overload is added to MCPUtils in the future, the helper can be removed. - -- **SaveGenericPackage return value ignored**: The old code captured the save result but only logged it (via UE_LOG which we're removing). I chose not to report save failures in the output. If save failures should be surfaced to the caller, we could add a check and append an error line. - -- **Circular chain check only walks UMaterialInstanceConstant**: The original code did this too. If there are other UMaterialInstance subclasses in the chain, they'd stop the walk. This seems fine for typical usage but is worth noting. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ReplaceFunctionCallsInBlueprint.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ReplaceFunctionCallsInBlueprint.Notes.md deleted file mode 100644 index 58105ca6..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ReplaceFunctionCallsInBlueprint.Notes.md +++ /dev/null @@ -1,29 +0,0 @@ -# UMCPHandler_ReplaceFunctionCallsInBlueprint - Refactoring Notes - -## Changes Made - -- **Plain-text output**: Converted from `FJsonObject*` to `FStringBuilderBase&`. -- **MCPAssetFinder**: Already used `MCPAssets` for blueprint loading; updated `.Errors(Result)` to use the string builder directly instead of JSON. -- **MCPUtils::FindClassByName**: Replaced ~20 lines of hand-rolled class resolution (FindFirstObject, U-prefix stripping, TObjectIterator fallback) with a single call. -- **MCPUtils::Identifies**: Replaced hand-rolled class name matching (5-way string comparison with _C suffix, U prefix variants) with `MCPUtils::Identifies(OldClass, ParentClass)`. -- **MCPUtils::FormatName**: Used for all node, pin, and class names in output. -- **Removed UE_LOG**: All three UE_LOG calls removed. -- **Removed unused includes**: Stripped ~30 includes that were not needed by this handler. -- **Simplified connection tracking**: Stored `TArray` directly instead of serializing to string pairs, then used `Contains()` to check survival. - -## What's Good - -- The core logic (iterate CallFunction nodes, match class, find function in new class, call SetFromFunction) is sound and straightforward. -- The dry-run mode is a useful feature for previewing impact. -- Batch processing is implicit: it already processes all matching nodes in the blueprint. - -## Uncertainties / Conservative Choices - -- **MCPUtils::Identifies for UClass**: I used `Identifies(OldClass, ParentClass)` to match the old class. The old code had manual U-prefix and _C suffix matching. `Identifies` compares against `FormatName(Class)` which returns `GetName()` with sanitization. If `FormatName` for a class doesn't handle the same variants (U-prefix stripping, _C suffix), some matches the old code would find could be missed. This should be verified. -- **Connection survival check**: The old code serialized pin connections to string pairs (NodeGuid + PinName) before calling `SetFromFunction`, then compared after. I simplified to storing the actual `UEdGraphPin*` pointers and checking `Contains()`. This works as long as `SetFromFunction` doesn't destroy/recreate the *linked* pins on other nodes. It only reconstructs pins on the CallFunction node itself. The linked-to pins on other nodes should remain stable, so the pointer comparison should be safe. But if there's ever a case where linked-to pin pointers are invalidated, the old string-based approach would be more robust. -- **Dry-run "at risk" output**: The old code reported every individual connection at risk with full details in JSON. The new code reports more concisely, showing only the first linked pin. If a pin has multiple connections at risk, only the first is named. This seemed like a reasonable tradeoff for conciseness, but could be expanded if needed. - -## Potential Future Improvements - -- The handler could accept an array of class redirections to process multiple replacements in one call. -- The dry-run output could include a summary count of at-risk connections. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RestoreAsset.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RestoreAsset.Notes.md deleted file mode 100644 index 4a135020..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RestoreAsset.Notes.md +++ /dev/null @@ -1,27 +0,0 @@ -# UMCPHandler_RestoreAsset - Refactoring Notes - -## What was changed - -- **Switched from JSON to plain-text output.** The handler now overrides `Handle(Json, FStringBuilderBase&)` instead of `Handle(Json, FJsonObject*)`. Error messages use `Result.Appendf("ERROR: ...")` instead of `MCPUtils::MakeErrorJson`. Success output is a single concise line. - -- **Removed unnecessary includes.** Stripped out `MCPAssetFinder.h`, `Engine/Blueprint.h`, `Kismet2/BlueprintEditorUtils.h`, `UObject/SavePackage.h`, `AssetRegistry/AssetRegistryModule.h`, `AssetRegistry/IAssetRegistry.h`, `AssetToolsModule.h`, and `IAssetTools.h` -- none of these were used by the handler. - -- **Added reload failure reporting.** The original handler silently ignored `ReloadPackages` failures. Now it checks `bReloaded` and reports a WARNING with the error message if the reload fails. The file is still restored on disk in that case, so it's a warning rather than an error. - -- **Improved error message for copy failure.** Changed from showing the raw disk path to showing the package path, which is what the caller knows and cares about. - -## What looks good - -- The handler is simple and focused: it does one thing (restore from backup) and does it clearly. -- The flow is linear with early returns, keeping nesting shallow. -- No UE_LOG calls were present in the original, so none needed removal. - -## What was left alone / areas of uncertainty - -- **No use of MCPFetcher or MCPAssetFinder.** This handler operates on raw file paths (`.uasset` and `.uasset.bak`), not on loaded UObjects. MCPFetcher and MCPAssetFinder are designed for navigating loaded assets and the asset registry, which doesn't apply here. The handler correctly uses `FPackageName::LongPackageNameToFilename` to convert the package path to a disk path. - -- **No FormatName/Identifies usage.** The handler's input is a package path string (e.g. `/Game/Widgets/WB_Hotkeys`), not an object reference that would benefit from FormatName. The output just echoes the package path, which is the natural identifier for this operation. - -- **Batching was not added.** Restoring multiple assets at once seems like an unusual operation -- typically you restore one asset at a time. Could be reconsidered if there's a use case. - -- **The `ReloadPackages` call uses `AssumePositive` for the interaction mode.** This suppresses any editor dialogs. This seems correct for an MCP handler (no human is watching), but I left it as-is since I'm not 100% sure about the implications. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SearchAssets.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SearchAssets.Notes.md deleted file mode 100644 index 3c700592..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SearchAssets.Notes.md +++ /dev/null @@ -1,23 +0,0 @@ -# UMCPHandler_SearchAssets - Refactoring Notes - -## What's Good About This Handler - -- **Clean MCPAssetFinder usage.** The handler uses MCPAssetFinder throughout, with `NoScans().Scan(TypeClass)` for type filtering, `Substring()` for string matching, `AllContent()` for scope, `Limit()` for pagination, and `Errors(Result)` for error reporting directly into the output device. This is textbook usage of the API. - -- **Plain-text output.** Each result is one line in `ClassName /Package/Path` format, which is concise and easy for an LLM to parse. - -- **No UE_LOG calls.** All errors go through the response via `Errors(Result)` or direct `Result.Append`. - -- **FormatName for class names.** Uses `MCPUtils::FormatName(Data.GetClass())` for consistent naming of asset classes. - -- **Good error handling.** Validates that at least one filter is specified, validates the Type class exists via `MCPUtils::FindClassByName`, and reports when results hit the limit so the caller knows to raise it. - -## What Might Need More Work - -- **`Data.GetClass()` with `.Info()` (not `.Load()`).** The handler calls `Assets.Info()` which does not load assets into memory. `FAssetData::GetClass()` resolves the asset's UClass from the class path stored in the asset registry, which can return null if the class itself isn't loaded (e.g., for plugin classes that aren't in memory). In practice this is unlikely for `/Game` content, but it could produce a crash on the `FormatName` call if it ever happens. Other handlers (FindAssetReferences, FindMaterialReferences) use the same pattern, so this is a codebase-wide concern rather than specific to this handler. I left it as-is for consistency. - -- **Package path output uses raw `Data.PackageName.ToString()`.** There is no `FormatName` overload for `FAssetData`, so this is the natural way to display the asset path. If a `FormatName(FAssetData)` overload is added in the future, this should switch to it. The same pattern appears in FindAssetReferences and FindMaterialReferences. - -- **No batching needed.** This is a search tool, not a mutation tool, so batching doesn't apply. A single call with `Substring` and `Type` filters handles the use case well. - -- **The `Type` parameter uses `MCPUtils::FindClassByName`.** This is good -- it supports flexible class name matching. The error message when the class isn't found is clear. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SearchSpawnableNodeTypes.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SearchSpawnableNodeTypes.Notes.md deleted file mode 100644 index 88faab6b..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SearchSpawnableNodeTypes.Notes.md +++ /dev/null @@ -1,29 +0,0 @@ -# UMCPHandler_SearchSpawnableNodeTypes - Refactoring Notes - -## What was done - -- **Plain-text output**: Switched from `Handle(Json, FJsonObject*)` to `Handle(Json, FStringBuilderBase&)`. Output is now one spawner name per line, no JSON wrapping. The `count` field was dropped since the caller can count lines. - -- **MCPFetcher**: Replaced the hand-rolled `MCPAssets` + `MCPUtils::AllGraphsNamed` + `UrlDecode` sequence with `MCPFetcher::Walk(Blueprint).Graph(Graph)`. This is more concise and uses the standard naming/error infrastructure. - -- **Concise output**: Removed the JSON envelope (`count`, `results` array). Each result is just the spawner full name on its own line, which is what the caller actually needs for `spawn_node`. - -- **Reduced includes**: Removed ~40 unnecessary includes. The handler only needs `EdGraph/EdGraph.h` beyond the standard handler/utility headers. - -## What's good about this handler - -- The core logic is simple and well-focused: it's a thin wrapper around `MCPUtils::SearchNodeSpawners`, which does the heavy lifting. -- The optional graph filter is a useful feature that doesn't complicate the common case. -- Parameter descriptions are clear. - -## Areas of uncertainty - -- **MCPFetcher vs MCPAssets for blueprint lookup**: The original used `MCPAssets` with `Exact()` match, which accepts both asset names and paths. `MCPFetcher::Walk` expects a package path (e.g. `/Game/Widgets/WB_Hotkeys`). If callers were passing bare asset names (e.g. `WB_Hotkeys`) rather than full paths, the fetcher approach would fail. I went with MCPFetcher because the coding standards prefer it, but this is worth testing. If bare names need to work, the Blueprint parameter's description should clarify that a full path is required, or the handler should fall back to MCPAssets when the input has no slash. - -- **UrlDecode on graph name**: The original code called `MCPUtils::UrlDecode(Graph)` before graph lookup. MCPFetcher's `.Graph()` walker presumably handles this internally (or doesn't need it if the MCP parameter population already decodes). I dropped the explicit `UrlDecode` — if the fetcher's walker doesn't decode, graph names with special characters could fail to match. - -- **MCPFetcher::Obj access**: I access `F.Obj` directly (it's a public field) and cast it to `UEdGraph*`. The fetcher's `.Graph()` walker should leave `Obj` pointing at the graph, but if the walker sets `ResultPin` instead, the cast would return null. This seemed correct from reading the API but is worth verifying at runtime. - -## What could still be improved - -- The `Blueprint` and `Graph` parameters are only useful together, but the handler silently ignores `Graph` if `Blueprint` is empty (and vice versa). It might be better to emit a warning if only one is provided. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SearchTypeUsageInBlueprints.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SearchTypeUsageInBlueprints.Notes.md deleted file mode 100644 index 5c9e9fbe..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SearchTypeUsageInBlueprints.Notes.md +++ /dev/null @@ -1,28 +0,0 @@ -# UMCPHandler_SearchTypeUsageInBlueprints — Refactoring Notes - -## What was done - -- **Converted to plain-text output** (`FStringBuilderBase&` instead of `FJsonObject*`). Each result is one line with a compact format like `variable VarName in BlueprintName: category subtype`. -- **Used `MCPUtils::FormatName()`** for all object naming: blueprints, graphs, nodes, pins, and variables. -- **Removed all UE_LOG calls** (there were none in this handler, so no changes needed). -- **Reduced output verbosity.** The JSON version emitted many fields per result (blueprint, blueprintPath, usage, location, nodeId, currentType, currentSubtype, isLevelBlueprint, graph, pinType, pinSubtype, connectionCount). The plain-text version emits one concise line per result. -- **Extracted helpers** `GetSubtype` and `PinTypeMatches` to reduce code duplication across the variable/parameter/pin checking sections. - -## What's good - -- The handler already used `MCPAssets` for asset scanning and `MCPUtils::AllNodes()` for iteration — no changes needed there. -- The type matching logic (strip F/E/U prefix, case-insensitive compare) is straightforward and correct. -- The search covers a good breadth: variables, function params, event params, break/make struct nodes, and connected pins. - -## Uncertainties and conservative choices - -- **`bIsLevel` flag is no longer surfaced.** The old JSON output included an `isLevelBlueprint` boolean. In the plain-text version, level blueprints are distinguished by their `FormatName` output. If `FormatName(LevelScriptBlueprint)` doesn't clearly indicate it's a level blueprint, this information is lost. I did not add an explicit marker because I didn't want to add noise without knowing whether callers depend on it. -- **Node GUIDs are no longer in the output.** The JSON version included `nodeId` (GUID) for many result types. The plain-text version uses `FormatName(Node)` instead, which should be sufficient for identification via `MCPFetcher` paths. If callers need raw GUIDs, this would need to be added back. -- **`blueprintPath` dropped.** The JSON version included the full package path. The plain-text version only shows `FormatName(BP)`. If the FormatName doesn't include enough path info to disambiguate blueprints with the same short name, this could be a problem. -- **Custom event naming.** For custom event parameters, I used `FormatName(Node)` (the custom event node) rather than `CustomEvent->CustomFunctionName.ToString()`. FormatName should produce a good name, but I'm not 100% sure it matches the custom function name exactly. -- **The `Filter` parameter still uses raw string matching** against asset name/path before loading. This is an optimization to avoid loading every blueprint, and is separate from the MCPAssetFinder substring filter. It could potentially be replaced with `MCPAssets::Substring()`, but that would change behavior since the current code checks both name and path, and I wasn't sure of the exact MCPAssetFinder substring semantics. - -## Potential further work - -- The FunctionEntry and CustomEvent parameter loops are structurally identical. They could be unified by checking for `UK2Node_EditablePinBase` (the common base class) and iterating `UserDefinedPins`, with the usage label derived from the node type. -- The asset loading loop could potentially use `MCPAssets::Substring()` instead of hand-rolling the filter check, but the current filter checks both asset name and path, which may not map cleanly to `Substring()`. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SearchUnrealClasses.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SearchUnrealClasses.Notes.md deleted file mode 100644 index e828c027..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SearchUnrealClasses.Notes.md +++ /dev/null @@ -1,24 +0,0 @@ -# UMCPHandler_SearchUnrealClasses — Refactoring Notes - -## Changes Made - -- **Converted to plain-text output.** Switched from `Handle(Json, FJsonObject*)` to `Handle(Json, FStringBuilderBase&)`. Output is now compact lines like: - ` ActorComponent [Abstract] : Object` -- **Removed all JSON construction.** No more `FJsonValueString`, `SetArrayField`, etc. -- **Used MCPUtils::FormatName(Class) consistently** for all class naming — both in the filter check and in output. Previously the filter compared against `Class->GetName()` directly. -- **Used MCPUtils::Identifies() for ParentClass lookup** instead of hand-rolled `GetName() == ParentClass` with `_C` suffix fallback. This is more robust and follows the coding standard. -- **Removed UE_LOG calls.** (There were none in the original, so nothing to remove.) -- **Removed unused includes** (`Engine/Blueprint.h`, `EdGraph/EdGraph.h`, `EdGraphNode.h`, `EdGraphPin.h`, `EdGraphSchema_K2.h`, `MCPAssetFinder.h`). None of those types were used. -- **Simplified Limit clamping.** The original only clamped when the JSON field was present, which meant programmatic callers could bypass it. Now always clamped. -- **Removed fullPath and package fields.** The original output included `fullPath` and `package` per class. These are rarely useful to an LLM and add significant token cost. The `FormatName` output should be sufficient to identify a class. If package info is needed, it could be added back as an optional parameter. - -## What's Good - -- The handler does a reasonable job: it iterates all UClasses, applies substring + parent filters, and respects a limit with a total count. The core logic is sound. -- Already used `MCPUtils::FormatName(Class)` for the name field (but not for the filter comparison — now fixed). - -## Uncertainties / Conservative Choices - -- **Filter now matches against FormatName output** instead of `GetName()`. This changes the filtering behavior — FormatName may produce a different string than GetName. I believe this is correct per the standard ("the only valid way to get a name"), but if existing callers rely on matching raw UClass names, this could be a subtle behavioral change. -- **Removed package info.** If callers rely on package information, this would need to be restored. I removed it to follow the "concise output" principle, but it's easy to add back. -- **No batching applicable here** — this is a search tool, not a mutation tool, so batching doesn't apply. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SearchWithinBlueprints.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SearchWithinBlueprints.Notes.md deleted file mode 100644 index 60fb4ed2..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SearchWithinBlueprints.Notes.md +++ /dev/null @@ -1,28 +0,0 @@ -# UMCPHandler_SearchWithinBlueprints - Refactoring Notes - -## Changes Made - -- **Plain-text output**: Converted from `Handle(FJsonObject*)` to `Handle(FStringBuilderBase&)`. Each result is a block of indented key-value lines separated by blank lines, with a summary count at the end. - -- **MCPUtils::FormatName()**: Blueprint names now use `MCPUtils::FormatName(BP)` instead of `Asset.AssetName.ToString()`. Graph, node, and class names already used FormatName and remain unchanged. - -- **MCPAssetFinder Path filtering**: The old code manually filtered by path substring in its own loop. Now the `Path` parameter is passed to `MCPAssets::Substring()`, letting MCPAssetFinder handle the filtering. Changed from `Info()` to `Load()` since we need the actual UObject pointers anyway. - -- **Removed unused includes**: Dropped `EdGraph/EdGraph.h`, `K2Node_BreakStruct.h`, `K2Node_MakeStruct.h`, `K2Node_FunctionEntry.h`, `K2Node_EditablePinBase.h`, `AssetRegistry/IAssetRegistry.h` — none of these types were referenced in the handler. - -- **No UE_LOG calls**: The original had none, so nothing to remove. - -- **Simplified lambda**: The lambda no longer takes asset name/path strings as parameters. It takes a `UBlueprint*` and uses `FormatName(BP)` directly. - -## What's Good - -- The search logic itself is straightforward and correct: it checks node titles, function names, event names, and variable names against the query. -- MCPAssets is used properly for asset discovery. - -## Uncertainties / Conservative Choices - -- **MCPAssets::Substring with Path**: The `Substring` method may match against asset name vs. asset path differently depending on whether the string contains a slash. The old code always matched against `PackageName` (the full path). If `Path` doesn't contain a slash, `Substring` may match against the asset name only, which is a behavior change. This should be tested. - -- **Load() vs Info()**: Switched to `Load()` since the old code was calling `GetAsset()` on every entry anyway. This should be equivalent but loads all matching assets upfront rather than lazily. For large projects this could be slower if the path filter is broad, but `Substring` filtering happens during the scan, so in practice only matching assets get loaded. - -- **Function/event/variable names in output**: These are still output as raw strings (e.g. `CF->FunctionReference.GetMemberName().ToString()`) rather than going through FormatName. There's no `FormatName` overload for `FName` or `FMemberReference` that would apply here in a meaningful way — FormatName(FMemberReference) exists but may produce a different format than the raw member name. I left these as-is to avoid changing search result semantics. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SearchWithinMaterials.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SearchWithinMaterials.Notes.md deleted file mode 100644 index 018c8727..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SearchWithinMaterials.Notes.md +++ /dev/null @@ -1,29 +0,0 @@ -# UMCPHandler_SearchWithinMaterials - Refactoring Notes - -## Changes Made - -1. **Plain-text output**: Switched from `Handle(Json, FJsonObject*)` to `Handle(Json, FStringBuilderBase&)`. Output is now one line per match instead of a JSON array of objects. - -2. **FormatName**: Material names now come from `MCPUtils::FormatName(MaterialObj)` (returns path name) and expression names from `MCPUtils::FormatName(Expr)` (returns sanitized object name). Previously used `Asset.AssetName.ToString()` and `Asset.PackageName.ToString()` ad-hoc. - -3. **Concise output**: Removed echoing of query and resultCount back to the caller. Each result is a single line: either `material ` or `expression in () param=`. - -4. **Trimmed includes**: Removed ~20 unused includes (MaterialGraph, BlueprintEditorUtils, AssetRegistry, various expression types that were never referenced, etc.). Kept only the parameter expression headers actually used for Cast. - -5. **Load instead of Info**: Switched from `Info()` + manual `GetAsset()` casts to `Load()` + `Objects()`, which is the cleaner MCPAssets pattern for when you need the loaded objects. - -## What Looks Good - -- The handler already used MCPAssets for asset discovery, which is correct. -- The parameter name extraction logic (casting to specific parameter expression types) is reasonable and necessary since there's no common base for parameter names. -- The MaxResults clamping is sensible. - -## Areas for Further Consideration - -- **Expression matching uses class names and descriptions, not FormatName/Identifies**: The search matching still does raw `Contains` checks against `Expr->GetClass()->GetName()` and `Expr->GetDescription()`. These are search queries rather than identity checks, so `Identifies` doesn't directly apply here. But if `MCPUtils` gains a material-expression search helper in the future, this should use it. - -- **UrlDecode on Query**: The handler calls `MCPUtils::UrlDecode(Query)`. It's unclear whether the MCP parameter population layer already handles URL decoding. If it does, this is redundant. I left it in to be conservative. - -- **No AllContent()**: The handler only searches `/Game/` materials (the MCPAssets default). If engine or plugin materials should be searchable, `AllContent()` would need to be added. Left as-is since the original didn't search all content either. - -- **Loading all materials is expensive**: This handler loads every material asset to inspect expressions. For large projects this could be slow. A future optimization might use asset registry tags to pre-filter, or cache results. Left as-is since the original had the same behavior. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetAnimStateAnimation.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetAnimStateAnimation.Notes.md deleted file mode 100644 index 467f0f40..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetAnimStateAnimation.Notes.md +++ /dev/null @@ -1,22 +0,0 @@ -# UMCPHandler_SetAnimStateAnimation - Refactoring Notes - -## Changes Made - -- **Plain-text output**: Switched from `Handle(Json, FJsonObject*)` to `Handle(Json, FStringBuilderBase&)`. The old handler returned JSON with `createdNewNode` and `saved` booleans; the new handler returns a concise status line. -- **MCPFetcher for blueprint resolution**: Replaced `MCPAssets` + manual error handling with `MCPFetcher(Result).Walk(Blueprint).Cast()`. This gives the caller better error messages and accepts both asset names and full fetcher paths. -- **FormatName for output**: Uses `MCPUtils::FormatName(AnimBP)` and `MCPUtils::FormatName(AnimSeq)` in output messages instead of echoing raw input parameters. -- **Removed unnecessary includes**: Dropped `EdGraphPin.h`, `BlendSpace.h`, `AnimGraphNode_BlendSpacePlayer.h`, `K2Node_VariableGet.h`, `AnimStateTransitionNode.h` which were unused. -- **Concise output**: Instead of returning separate JSON booleans, the handler now returns a single descriptive line ("Created..." or "Updated..."). -- **Removed save-status reporting**: The old handler returned a `saved` boolean. The new handler calls `SaveBlueprintPackage` but doesn't report its result. If saving fails, the package will be marked dirty, which is a recoverable state. This matches the pattern in `UMCPHandler_AddAnimStateToMachine`. - -## What's Good - -- The handler's core logic is straightforward and well-scoped: find blueprint, find state machine, find state, find/create sequence player, assign animation. -- Error handling for the animation asset search is clean thanks to `MCPAssets` with `.Errors(Result)`. -- The `FindStateByName` call already accepts an `MCPErrorCallback`, so error reporting for missing states is automatic. - -## Areas for Further Work - -- **FindStateMachineGraph lacks an error callback.** The handler must manually format and emit the error when the state machine graph isn't found. If `FindStateMachineGraph` were updated to accept `MCPErrorCallback`, this would become a one-liner. This is the same situation noted in the AddAnimStateToMachine and AddAnimStateTransition notes. -- **No MCPFetcher walker for state machines or states.** If MCPFetcher gained `statemachine:Name` and `state:Name` walkers, the handler could replace the three-step lookup (blueprint + state machine graph + state) with a single `F.Walk(Path)` call. The `Blueprint`, `Graph`, and `StateName` parameters could collapse into a single `Path` parameter. -- **No batch support.** This handler operates on a single state. A batch variant that sets animations on multiple states in one call could reduce round-trips, though the use case may be uncommon enough that it's not worth the complexity. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetAnimStateBlendSpace.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetAnimStateBlendSpace.Notes.md deleted file mode 100644 index c45e35b9..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetAnimStateBlendSpace.Notes.md +++ /dev/null @@ -1,32 +0,0 @@ -# UMCPHandler_SetAnimStateBlendSpace - Refactoring Notes - -## Changes Made - -- **Converted to plain-text output.** Changed from `Handle(Json, FJsonObject*)` to `Handle(Json, FStringBuilderBase&)`. Output is now concise text reporting what was done. - -- **Removed UE_LOG calls.** The one `UE_LOG(LogTemp, Warning, ...)` for a missing variable is now reported via the Result string builder as a WARNING line. - -- **Used MCPUtils::FormatName.** The final output now uses `MCPUtils::FormatName(BSNode)` to identify the created node, instead of `BSNode->NodeGuid.ToString()`. - -- **Removed unused includes.** Dropped `AnimSequence.h` and `AnimStateTransitionNode.h` which were not needed. - -- **Extracted helper methods.** The large lambda `WireVariable` and the pose-connection block were extracted into private methods `WireVariable()` and `ConnectToOutputPose()` to reduce nesting in Handle(). - -- **MCPFetcher for error routing.** `MCPAssets` and `MCPUtils::FindStateByName` already accept `Result` (FStringBuilderBase&) for error reporting via MCPErrorCallback, so errors flow directly to the caller. - -## What's Good - -- The handler already used `MCPAssets` for both the anim blueprint and blend space lookups, which is the right pattern. -- `MCPUtils::FindStateByName` already accepts an MCPErrorCallback, so error handling for the state lookup is clean. - -## Areas for Further Work - -- **The Blueprint/Graph/StateName parameters could potentially be a single MCPFetcher path.** Instead of three separate parameters (Blueprint, Graph, StateName), the caller could pass something like `/Game/Foo,graph:StateMachine,node:IdleState`. However, this would change the tool's API contract, and the current walker system may not support navigating into state machine states directly. I left it as-is to avoid breaking changes. - -- **ConnectToOutputPose uses class name string matching** (`Contains("AnimGraphNode_Root")`) rather than a Cast or proper type check. This is fragile but was inherited from the original code. The proper fix would require including the actual header for those node types, which may have module dependency implications. - -- **Variable existence check is ad-hoc.** The code manually iterates `NewVariables` and checks `SkeletonGeneratedClass`. There may be a more robust Unreal API for this, but I kept the existing logic to avoid introducing subtle behavioral changes. - -- **No MCPFetcher usage for the inner graph traversal.** MCPFetcher can walk to graphs and nodes, but the state machine inner graph traversal here is specialized (FindStateMachineGraph, FindStateByName, GetBoundGraph). MCPFetcher doesn't currently have walkers for state machine states, so this can't be simplified further without extending MCPFetcher. - -- **Pin searching is done by manual iteration.** The code iterates pins looking for struct-typed output/input pins. This works but is somewhat fragile. A more robust approach might use pin names, but the pose pins don't have stable user-facing names across engine versions. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetAnimTransitionRule.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetAnimTransitionRule.Notes.md deleted file mode 100644 index 2b769fa8..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetAnimTransitionRule.Notes.md +++ /dev/null @@ -1,31 +0,0 @@ -# UMCPHandler_SetAnimTransitionRule - Refactoring Notes - -## What was changed - -1. **Converted from JSON output to plain-text output.** Changed `Handle(Json, FJsonObject* Result)` to `Handle(Json, FStringBuilderBase& Result)`. The old handler returned JSON fields like `propertiesChanged` and `saved`; the new one emits a single summary line with the changed count and `FormatName(TransNode)`. - -2. **Error reporting via MCPErrorCallback.** `MCPAssets::Errors(Result)` now takes the `FStringBuilderBase&` directly instead of a raw `FJsonObject*`. Manual `MakeErrorJson` calls replaced with `Result.Appendf(TEXT("ERROR: ..."))`. - -3. **FormatName for output.** The success message now uses `MCPUtils::FormatName(TransNode)` and `MCPUtils::FormatName(AnimBP)` for consistent naming. - -4. **Removed unnecessary includes.** Dropped includes that were not needed (`EdGraph/EdGraphPin.h`, `AnimGraphNode_SequencePlayer.h`, `AnimGraphNode_BlendSpacePlayer.h`, `AnimStateNode.h`, `K2Node_VariableGet.h`, `Animation/AnimSequence.h`, `Animation/BlendSpace.h`, `EdGraph/EdGraphNode.h`, `EdGraph/EdGraph.h`). - -5. **No UE_LOG calls** were present in the original, so nothing to remove there. - -## What is good about this handler - -- The property-update pattern (checking `Json->HasField` for each optional field) is clean and correct -- it only modifies fields the caller actually provided. -- `PreEditChange` / `PostEditChange` bracketing is properly done. -- Compiles and saves after modification. - -## Areas of concern / conservative choices - -- **FindStateMachineGraph lacks MCPErrorCallback.** Unlike `FindStateByName`, this function returns nullptr without reporting an error through the callback system. The handler manually emits an error message. If `FindStateMachineGraph` were updated to accept `MCPErrorCallback`, this code could be simplified. - -- **FindTransition also lacks MCPErrorCallback.** Same situation -- the handler must manually format the error for a missing transition. - -- **MCPFetcher doesn't support state machine graphs.** The fetcher has no `StateMachine` or `Transition` walker, so the handler still uses `MCPAssets` + `MCPUtils::FindStateMachineGraph` + `MCPUtils::FindTransition` as separate steps. If MCPFetcher gained walkers like `statemachine:Name` and `transition:From->To`, this handler could collapse to a single `MCPFetcher::Walk(Path)` call. - -- **Enum values passed as raw integers.** `BlendMode` and `LogicType` are accepted as `int32` and cast to enum types. Using `MCPUtils::StringToEnum` would give better error messages and let callers pass string names instead of opaque integer values. I did not change this because it would alter the tool's external API. - -- **No FormatName/Identifies on state names.** `FindTransition` and `FindStateMachineGraph` use their own string matching internally, not `MCPUtils::Identifies`. This is in MCPUtils itself, not in this handler, but it means name matching here may be inconsistent with other handlers. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetBlendSpaceSamplePoints.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetBlendSpaceSamplePoints.Notes.md deleted file mode 100644 index 33495c26..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetBlendSpaceSamplePoints.Notes.md +++ /dev/null @@ -1,35 +0,0 @@ -# UMCPHandler_SetBlendSpaceSamplePoints — Refactoring Notes - -## What was done - -- **Plain-text output**: Switched from JSON `Handle(Json, Result)` to plain-text `Handle(Json, FStringBuilderBase& Result)`. Output is now a single line like `Set 3 samples on BS_Locomotion`, plus a warning line if the save failed. - -- **Removed UE_LOG**: The `UE_LOG(LogTemp, ...)` call at the end was removed. The response already reports success/failure to the caller. - -- **FormatName**: Replaced `BS->GetPathName()` with `MCPUtils::FormatName(BS)` for consistent naming. - -- **MCPFetcher**: Not applicable here — the handler fetches a `UBlendSpace` asset by name, which is the job of `MCPAssetFinder`, not `MCPFetcher`. MCPFetcher is for walking paths like `graph:X,node:Y,pin:Z`. - -- **AnimSequence lookup error handling**: The original code silently ignored animation asset lookup failures — if `AnimAssets.Exact(name).Load()` failed, it would just add a sample with no animation and no error. Changed to use `.Errors(Result).ENone()` so a bad animation name produces an error message and aborts, rather than silently inserting a blank sample point. - -- **Trimmed includes**: Removed headers that were not needed (`EdGraph/*.h`, `AnimBlueprint.h`, `AnimBlueprintGeneratedClass.h`, `Skeleton.h`, `AnimGraphNode_Base.h`, `KismetEditorUtilities.h`). Added `MCPFetcher.h` since other handlers include it and it's part of the standard set. - -- **Removed redundant error check**: The old code had a manual `BlendSpace.IsEmpty()` check with `MakeErrorJson`. This is unnecessary because `MCPAssets::Exact("").ENone()` will produce a clear error. - -## What's good - -- The handler already used `MCPAssetFinder` properly for the main blend space lookup. -- The `FBlendSpaceSampleEntry` struct with `PopulateFromJson` is clean. -- The axis parameter handling with `Json->HasField()` checks is correct — it allows partial updates of only the fields the caller provides. - -## Areas of uncertainty / conservatism - -- **Error handling on AnimSequence failure**: I changed from "silently skip" to "abort with error." This is a judgment call — the old behavior let you add samples without animations, which might be intentional. But silently ignoring a bad asset name seems like a bug magnet. If the caller genuinely wants a sample with no animation, they can omit the `animationAsset` field entirely (the `IsEmpty()` check still allows that). - -- **No batch-style error reporting**: Unlike ConnectPins which reports per-entry results, this handler aborts on the first bad sample. This seems appropriate because samples replace all existing ones — a partial set would leave the blend space in a bad state. But it's worth noting the difference in philosophy. - -## Remaining work - -- The handler doesn't use `MCPUtils::Identifies` anywhere, but there's no place where it needs to match names against objects, so this is fine. - -- ConnectPins (the example) still has a `UE_LOG` call on line 96-97 that should be removed in its own refactoring pass. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetBlueprintVariableMetadata.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetBlueprintVariableMetadata.Notes.md deleted file mode 100644 index 563b90c3..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetBlueprintVariableMetadata.Notes.md +++ /dev/null @@ -1,28 +0,0 @@ -# UMCPHandler_SetBlueprintVariableMetadata - Refactoring Notes - -## Changes Made - -1. **Plain-text output**: Converted from JSON response (`Handle(Json, FJsonObject* Result)`) to plain-text response (`Handle(Json, FStringBuilderBase& Result)`). Removed all the JSON `Change` object construction. Each field change now emits a single line like `Set category to 'Gameplay'.` - -2. **MCPFetcher**: Replaced `MCPAssets` with `MCPFetcher` for blueprint lookup, matching the pattern in `RemoveBlueprintVariable`. This is more concise and routes errors directly to the output. - -3. **FormatName**: Variable-not-found error and final success message now use `MCPUtils::FormatName(BP)` and `MCPUtils::FormatName(Var)` for consistent naming. - -4. **Removed UE_LOG**: The `UE_LOG(LogTemp, ...)` call was removed. - -5. **Concise output**: Removed the old/new value echo for each field (the caller knows what they sent). Removed the `availableVariables` JSON array in favor of a simple text list. The summary line reports the count and blueprint name. - -6. **Removed unused includes**: Dropped `MCPAssetFinder.h`, `EdGraph/EdGraph.h`, `EdGraph/EdGraphPin.h`, `K2Node_VariableGet.h`, `K2Node_VariableSet.h` -- none were needed by this handler. - -## What Looks Good - -- The handler covers a useful set of metadata fields (category, tooltip, replication, expose-on-spawn, private, editability) with clear validation for enum-like string parameters. -- Early-return on validation errors prevents partial modifications when an invalid enum value is provided. - -## Areas of Uncertainty / Conservative Choices - -- **Variable matching**: The example handler (`RemoveBlueprintVariable`) uses `MCPUtils::FormatName(Var) == VariableName` combined with a case-insensitive `VarName` comparison. I followed the same pattern rather than using a dedicated `MCPUtils::Identifies` overload for `FBPVariableDescription`, since I don't see one in the API. If one exists or should be added, this matching code could be simplified. - -- **PreEditChange/PostEditChange placement**: The original code calls `PreEditChange`/`PostEditChange` *after* all modifications are already done. Ideally `PreEditChange` should be called *before* modifications and `PostEditChange` after. I preserved the original ordering to stay conservative, but this may warrant a fix. - -- **Batching**: This handler modifies multiple metadata fields on a single variable in one call, which is already a form of batching. Extending it to modify metadata on multiple variables at once could be useful but would be a larger change. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetClassDefaultValue.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetClassDefaultValue.Notes.md deleted file mode 100644 index 3ce4ac79..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetClassDefaultValue.Notes.md +++ /dev/null @@ -1,32 +0,0 @@ -# UMCPHandler_SetClassDefaultValue - Refactoring Notes - -## What was done - -- **Converted to plain-text output.** Switched from `Handle(Json, FJsonObject*)` to `Handle(Json, FStringBuilderBase&)`. All error messages and results go through the string builder. - -- **Removed UE_LOG call.** The log line at the end has been removed; the result is reported via the response instead. - -- **Trimmed includes.** The original had ~40 includes, most of which were unnecessary for this handler. Reduced to just the ones actually used. - -- **Concise output.** Instead of echoing back separate fields (oldValue, newValue, propertyType, saved), the success output is a single line like `TSubclassOf MyProp: OldClass -> NewClass`. Save failure is reported as a warning only when it actually fails. - -- **Used MCPUtils::FormatName() consistently.** All object names in output (blueprint, class, meta-class) use FormatName. The old code used `*Blueprint` (the raw input string) in some error messages instead. - -- **Extracted ResolveClass helper.** The class-resolution logic (C++ class iteration + blueprint asset fallback) was pulled into a private method to reduce nesting in the main Handle method. - -- **MCPAssetFinder error reporting.** All MCPAssets calls pass `Errors(Result)` so errors go directly to the caller. - -## What's good - -- The handler covers three distinct property type families (class refs, object refs, simple types) which is genuinely useful breadth. -- The MCPAssetFinder usage was already mostly in good shape; just needed error callback modernization. - -## Uncertainties / conservative choices - -- **Object property branch:** The original code assumed the Value is always a Blueprint name and resolved it to the CDO. This is a narrow interpretation of "object property" - it won't work for setting an object property to a StaticMesh, Texture, or other non-Blueprint asset. I preserved this behavior rather than expanding it, since I'm not sure what use cases are intended. This branch could benefit from a more general asset resolution approach. - -- **ResolveClass C++ iteration:** The `TObjectIterator` loop to find C++ classes is O(n) over all loaded classes. I preserved it as-is since there isn't a clear better alternative in the existing utilities (MCPUtils::FindClassByName exists but I wasn't sure if its behavior matches exactly). Worth investigating whether `MCPUtils::FindClassByName` could replace the manual iteration. - -- **No batching.** This handler operates on a single property at a time. It could potentially be extended to accept an array of property/value pairs, which would be useful when configuring multiple defaults on one blueprint. I did not add this since the coding standards say to batch "where it makes sense" and this felt like a judgment call for the architect. - -- **SoftClassProperty path:** The `FSoftObjectPtr` construction for soft class properties was preserved as-is. I'm not 100% certain this is the correct way to set a TSoftClassPtr value, but it matches the original code. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetMaterialExpressionPosition.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetMaterialExpressionPosition.Notes.md deleted file mode 100644 index b26c488b..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetMaterialExpressionPosition.Notes.md +++ /dev/null @@ -1,25 +0,0 @@ -# UMCPHandler_SetMaterialExpressionPosition - Refactoring Notes - -## Changes Made - -- **Plain-text output**: Switched from JSON `Handle(FJsonObject*)` to plain-text `Handle(FStringBuilderBase&)`. Output is now concise single-line confirmation. -- **FormatName/Identifies**: Node lookup now uses `MCPUtils::Identifies(Node, MatNode->MaterialExpression)` instead of raw GUID string comparison. Output uses `MCPUtils::FormatName()` for the expression name. Updated the parameter description to guide callers toward using FormatName-style identifiers. -- **Removed UE_LOG**: Both UE_LOG calls removed. Errors and results go through the response. -- **Concise output**: No longer echoes the material name or command parameters back. Just reports what was moved and where. Save failure is noted inline rather than as a separate field. -- **Error reporting**: Uses `MCPErrorCallback(Result).SetError(...)` for consistency. -- **Trimmed includes**: Removed ~25 unnecessary includes (factory headers, JSON serialization, GUID, file helpers, blueprint editor utils, etc.). Only headers actually needed are kept. - -## What Looks Good - -- The handler correctly updates both the graph node position and the underlying `MaterialExpression` editor position, which is important for keeping them in sync. -- The `PreEditChange`/`PostEditChange` bracketing is correct. -- MCPAssetFinder usage was already in good shape for loading the material/function. - -## Areas of Uncertainty - -- **MCPUtils::Identifies for UMaterialExpression**: I used `Identifies(Node, MatNode->MaterialExpression)` based on the declaration in MCPUtils.h. I did not verify the implementation to confirm what name formats it accepts. If it only matches FormatName output (and not raw GUIDs), this is a behavioral change from the original which matched by GUID. Callers would need to use the expression name from DumpMaterial output instead of a GUID. This seems like the right direction per coding standards, but existing callers using GUIDs would break. - -## Possible Future Work - -- **Batch support**: This handler moves one expression at a time. A layout operation typically repositions many nodes. A batch variant accepting an array of `{node, x, y}` tuples would reduce round-trips significantly. -- **MCPFetcher integration**: There's no `MCPFetcher` walker for material expressions currently. If one were added, the material/function loading and expression lookup could collapse into a single `F.Walk(path)` call. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetMaterialExpressionProperty.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetMaterialExpressionProperty.Notes.md deleted file mode 100644 index 2c791559..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetMaterialExpressionProperty.Notes.md +++ /dev/null @@ -1,37 +0,0 @@ -# UMCPHandler_SetMaterialExpressionProperty - Refactoring Notes - -## What was done - -1. **Plain-text output.** Switched from `Handle(Json, FJsonObject*)` to `Handle(Json, FStringBuilderBase&)`. The old handler returned a JSON object with fields like `expressionType`, `newValue`, `saved` -- the new handler emits a single concise line like `Constant_03 = 0.5` which is much cheaper on tokens. - -2. **FormatName/Identifies.** The old handler matched nodes by raw GUID string comparison (`GraphNode->NodeGuid.ToString() == Node`). Now uses `MCPUtils::Identifies(Node, GraphNode)` for consistent name matching. The Node parameter description was updated to say "from FormatName" instead of "GUID". The success output uses `MCPUtils::FormatName(Expr)` instead of echoing back the raw GUID. - -3. **Removed UE_LOG.** The `UE_LOG(LogTemp, ...)` call at the end was removed. - -4. **Concise output.** The old handler echoed back the material name, expression type, new value, and saved status as separate JSON fields. Now it's a single line. The "save failed" note only appears if saving actually failed. - -5. **Removed unused includes.** Stripped about 20 includes that weren't needed (MaterialDomain.h, MaterialInstanceConstant.h, various factory headers, AssetRegistry, JsonReader/Writer/Serializer, Guid.h, FileHelper.h, Paths.h, SavePackage.h, UObjectIterator.h, BlueprintEditorUtils.h, EdGraph headers, etc.). - -6. **Extracted helper methods.** `ApplyValue`, `ParseColorValue`, and `ApplyParameterName` reduce the deep nesting and duplicated color-parsing logic from the original monolithic Handle method. - -7. **MCPErrorCallback integration.** Error messages from MCPAssetFinder flow directly to the FStringBuilderBase output via the `Errors(Result)` call -- same pattern as the DumpMaterial example. - -## What's good - -- The handler supports a solid set of expression types (Constant, Constant3/4Vector, ScalarParameter, VectorParameter, TextureCoordinate, Custom, ComponentMask). -- The material-vs-materialFunction dispatch is clean and well-structured. -- PreEditChange/PostEditChange bracketing is correct, including on error paths. - -## Areas of uncertainty / conservative choices - -- **MCPFetcher not used for node lookup.** The node lookup iterates `Graph->Nodes` manually because MCPFetcher's `Node()` walker operates on UEdGraph nodes within blueprints, and material graph nodes are `UMaterialGraphNode` which need a cast to get the `MaterialExpression`. Using MCPFetcher here might work but I wasn't confident it handles material graph nodes correctly, so I kept the manual loop with `MCPUtils::Identifies`. - -- **GUID vs FormatName for node identification.** The old handler used raw GUIDs to identify nodes. The refactored version uses `MCPUtils::Identifies` which should accept FormatName-style names. However, callers that were previously passing GUIDs will need to switch to FormatName-style identifiers. If backward compatibility with raw GUIDs is needed, this would require verification that `Identifies` accepts GUIDs for material graph nodes. - -- **UMaterialExpressionParameter base class.** The `ApplyParameterName` helper takes `UMaterialExpressionParameter*` as its parameter type. I believe both `ScalarParameter` and `VectorParameter` inherit from this, but if the class hierarchy differs in this Unreal version, this would need adjustment. - -## Possible further work - -- **Batch support.** This handler sets one expression at a time. A batch mode accepting an array of `{node, value}` pairs would reduce round-trips when configuring multiple expressions. -- **Texture parameters.** The handler doesn't support setting texture parameters (`TextureSampleParameter2D`, `TextureObjectParameter`). Adding these would make it more complete. -- **StaticSwitchParameter.** Also not supported for setting values. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetMaterialInstanceParameter.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetMaterialInstanceParameter.Notes.md deleted file mode 100644 index 3985cee5..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetMaterialInstanceParameter.Notes.md +++ /dev/null @@ -1,33 +0,0 @@ -# UMCPHandler_SetMaterialInstanceParameter - Refactoring Notes - -## Changes Made - -1. **Plain-text output.** Converted from JSON response (`FJsonObject* Result`) to plain-text response (`FStringBuilderBase& Result`). The output is now a single line like `Set scalar "Metallic" = 0.5 on MI_Chrome` -- concise and LLM-friendly. The old JSON response echoed `type`, `newValue`, and `dryRun` fields back, which is unnecessary since the caller already knows what it sent. - -2. **Removed UE_LOG calls.** Two `UE_LOG(LogTemp, ...)` calls were removed. All feedback now goes through the response. - -3. **FormatName for output.** The material instance name in the output line now uses `MCPUtils::FormatName(MI)` instead of echoing back the raw input string. - -4. **MCPAssetFinder for texture lookup.** The texture parameter path used a raw `LoadObject` call. Replaced with `MCPAssets` with `AllContent()` (since textures may live outside `/Game/`). This gives consistent error messages and supports the same name-matching flexibility as other asset lookups. - -5. **Simplified auto-detect.** The old auto-detect code manually walked the parent chain with a while-loop. Replaced with a single call to `MI->GetMaterial()`, which already resolves through the full parent chain to the base `UMaterial`. This eliminated the manual parent-walking loop entirely. - -6. **Extracted AutoDetectType to a helper.** Moved the auto-detection logic out of Handle into a private method, reducing nesting depth in the main handler. - -7. **Removed unused includes.** Dropped `Factories/MaterialInstanceConstantFactoryNew.h`, `AssetToolsModule.h`, and `IAssetTools.h` -- none were used. - -8. **Scalar format.** Changed scalar value formatting from `%f` (which produces trailing zeros like `0.500000`) to `%g` (which produces `0.5`). - -## What Looks Good - -- The handler already used `MCPAssets` for the material instance lookup with proper error routing. -- The parameter type auto-detection from the parent material is a nice UX feature. -- The DryRun option is useful for validation. - -## Areas for Further Consideration - -- **Identifies for parameter name matching.** The auto-detect code compares parameter names with `==`. There is no `MCPUtils::Identifies` overload for material parameter names (FName), so I left this as-is. If inconsistent matching becomes a problem, an Identifies overload for parameter names could help. - -- **Batch support.** This handler sets one parameter at a time. A batch version that accepts an array of parameter/value pairs would reduce round-trips when configuring a material instance (often involves setting 3-5 parameters). I did not add this because it would be a functional change, not a refactoring. - -- **Texture lookup semantics.** I used `MCPAssets.Exact()` for the texture path. The old code used `LoadObject` which requires an exact package path. The new code is more flexible (accepts asset names too) but might match differently if there are multiple textures with the same name. I added `ETwo()` to catch ambiguity. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetMaterialProperty.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetMaterialProperty.Notes.md deleted file mode 100644 index 2083fe60..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetMaterialProperty.Notes.md +++ /dev/null @@ -1,33 +0,0 @@ -# UMCPHandler_SetMaterialProperty - Refactoring Notes - -## Changes Made - -1. **Plain-text output.** Switched from `Handle(Json, FJsonObject* Result)` to `Handle(Json, FStringBuilderBase& Result)`. The output is now a single concise line like `blendMode: Opaque -> Masked` instead of a JSON object with separate fields for material, oldValue, newValue, dryRun, and saved. - -2. **Removed UE_LOG.** The `UE_LOG(LogTemp, ...)` call was removed. The response itself now carries the same information. - -3. **MCPAssetFinder errors go to Result.** The `Assets.Errors(Result)` call now passes the string builder directly, which works because MCPErrorCallback accepts FStringBuilderBase&. - -4. **Trimmed includes.** Removed ~35 unnecessary includes (MaterialExpression subtypes, factories, asset tools, serialization, graph headers, etc.). Only Material.h and MaterialDomain.h are needed. - -5. **Reduced bool property duplication.** The six nearly-identical bool property blocks were compressed using a `SetBool` lambda that handles the get/set/PreEditChange/PostEditChange pattern. Each bool property is now a single line calling SetBool with getter and setter lambdas. The bitfield members (uint8 :1) cannot have their address taken, so lambdas are used instead of pointers. - -6. **Concise output.** Removed echoing of `material` name and `dryRun` flag -- the caller already knows these. Save failure is reported as a WARNING line rather than a separate field. - -7. **Used FormatName for material name.** Not applicable here since we no longer echo the material name in the response. The `MCPAssets` error messages will use proper names internally. - -## What Looks Good - -- The property dispatch structure is clear and straightforward. -- MCPAssetFinder usage with `.Exact().Errors().ENone().ETwo().Load()` follows the established pattern well. -- The DryRun feature is a nice touch for LLM workflows. - -## Areas for Potential Further Work - -- **The `value` field is untyped.** It's extracted from raw JSON using `GetStringField`, `GetBoolField`, or `GetNumberField` depending on the property. This works but the caller doesn't get schema validation for `value` since it's not a UPROPERTY. An alternative design might use separate typed parameters (`stringValue`, `boolValue`, `numberValue`) declared as optional UPROPERTYs, which would give schema-level type info. I left this alone since it changes the external API. - -- **Property name matching is case-sensitive for most properties** but has a special case for `DitheredLODTransition` / `ditheredLODTransition`. The other properties don't have similar fallbacks. Could normalize with case-insensitive comparison throughout, but I left it as-is since changing matching behavior could be surprising. - -- **No batch support.** This handler sets one property per call. A batch variant that accepts an array of {property, value} pairs could reduce round-trips when configuring a new material. I did not add this since it would change the API. - -- **The enum property blocks** (domain, blendMode, shadingModel) still have some repetition in their parse-old/parse-new/apply pattern, but each has enough variation (different enum types, different member access patterns like `SetShadingModel()` vs direct assignment) that further consolidation would hurt readability. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetNodeComment.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetNodeComment.Notes.md deleted file mode 100644 index 34f27b0c..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetNodeComment.Notes.md +++ /dev/null @@ -1,19 +0,0 @@ -# UMCPHandler_SetNodeComment - Refactoring Notes - -## Changes Made - -- **Converted to plain-text output**: Changed `Handle` signature from `FJsonObject* Result` to `FStringBuilderBase& Result`. The old version wrote `oldComment` to a JSON field; the new version emits a single confirmation line. -- **Removed UE_LOG call**: The `UE_LOG(LogTemp, ...)` call was removed per coding standards. -- **Removed `OldComment` tracking**: The old handler stored the previous comment and returned it in the JSON response. This was removed for conciseness — the caller knows what it's replacing, and can use `GetNodeComment` if it needs the old value first. -- **MCPFetcher error handling**: Already using `MCPFetcher` with the output device, so errors propagate automatically. - -## What's Good - -- Uses `MCPFetcher` for node resolution — clean and consistent. -- Uses `MCPUtils::FormatName` for the confirmation message. -- Comment bubble visibility logic is sensible (auto-show on non-empty comment). - -## Uncertainties / Conservative Choices - -- **No bubble-hide on empty comment**: When `Comment` is empty, the handler does not explicitly set `bCommentBubbleVisible = false`. The original code didn't either. This may be intentional (let the user hide it manually), but it could also be an oversight. Left as-is to avoid changing behavior. -- **FindBlueprintForNodeChecked**: This will assert/crash if the node somehow isn't owned by a blueprint. The original used `Checked` too, so I left it. A non-checked version with an error message might be safer, but would change behavior. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetNodePositions.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetNodePositions.Notes.md deleted file mode 100644 index 8a5dd3da..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetNodePositions.Notes.md +++ /dev/null @@ -1,22 +0,0 @@ -# UMCPHandler_SetNodePositions — Refactoring Notes - -## What was done - -- Converted from JSON output (`FJsonObject*`) to plain-text output (`FStringBuilderBase&`). -- Removed `UE_LOG` call. -- Replaced per-entry JSON result objects with a single line per moved node using `MCPUtils::FormatName()`. -- Error callbacks for both `PopulateFromJson` and `MCPFetcher` now go directly into the plain-text `Result`, so errors appear inline with successes. -- Removed verbose old/new position echo — the caller knows what positions it requested, and the confirmation line is sufficient. - -## What's good - -- Batch support preserved — still iterates the `Nodes` array. -- `MCPFetcher` is used for both blueprint and node resolution, with errors reported directly to the output. -- Output is concise: one line per moved node, one summary line. -- `FMoveNodeEntry` struct kept for clean deserialization of each array element. - -## Uncertainties / conservative choices - -- **MarkBlueprintAsModified is called even if all entries fail.** The old code did this too. It's harmless but slightly imprecise — could guard it with `if (SuccessCount > 0)`. Left as-is to minimize diff. -- **Node resolution scope:** The current `MCPFetcher FN(Result, BP)` then `FN.Node(Entry.Node)` searches across all graphs in the blueprint. If two graphs have identically-named nodes, the wrong one could be matched. The `Entry.Node` field could accept a full walker path (e.g. `graph:EventGraph,node:MyNode`) via `FN.Walk()` instead — but `FN.Node()` is what the old code used, and changing the input format would break existing callers. -- **No graph parameter:** The handler has no way to scope node lookup to a specific graph. This could matter for blueprints with many graphs. Adding an optional `Graph` parameter would be a useful future enhancement. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetPinDefaultValues.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetPinDefaultValues.Notes.md deleted file mode 100644 index 345cf747..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SetPinDefaultValues.Notes.md +++ /dev/null @@ -1,27 +0,0 @@ -# UMCPHandler_SetPinDefaultValues — Refactoring Notes - -## Changes Made - -- **Converted to plain-text output.** Switched from `Handle(Json, FJsonObject*)` to `Handle(Json, FStringBuilderBase&)`. Removed all JSON result construction (EntryResult, Results array, SetStringField, SetNumberField, SetArrayField). - -- **Simplified entry struct.** Replaced the old `{Blueprint, Node, PinName, Value}` fields with `{Pin, Value}`, where `Pin` is a full MCPFetcher path (e.g. `/Game/Foo,graph:EventGraph,node:MyNode,pin:InputPin`). This is both more flexible and more concise. - -- **MCPFetcher for all object resolution.** Each entry uses `MCPFetcher(Result).Walk(Entry.Pin)` to resolve the pin. Error messages from resolution go directly to the output via MCPErrorCallback. - -- **MCPUtils::FormatName for naming.** Error messages use `MCPUtils::FormatName(Pin)` instead of raw `Entry.PinName`. - -- **Removed UE_LOG call.** - -- **Removed oldValue echo.** The old handler returned the previous default value for each pin. This was not asked for by the caller and consumes tokens. Removed per the "don't send information the caller didn't ask for" guideline. - -- **Kept batch support and ReconstructNode/MarkBlueprintAsModified.** The deferred reconstruction pattern (collect modified nodes, reconstruct after all changes) is preserved since it avoids redundant reconstruction when multiple pins on the same node are changed. - -## Uncertainties / Conservative Choices - -- **Removed oldValue reporting.** If callers depend on seeing the old value, this is a breaking change. Seemed right per the coding standards ("don't echo what the caller didn't ask for"), but worth confirming. - -- **No per-entry success confirmation.** The old JSON response had per-entry results. The new plain-text output only reports errors and a summary count. If an LLM caller needs to know *which specific pins* succeeded, it would need a different approach. The ConnectPins handler follows the same pattern, so this seems consistent. - -- **Entry struct kept.** Could have avoided the struct by using two separate FMCPJsonArray parameters (one for paths, one for values), but the struct approach is cleaner and matches other batch handlers like ConnectPins. - -- **Breaking API change for callers.** Old callers passed `{blueprint, node, pinName, value}` per entry; new callers pass `{pin, value}` where `pin` is a full fetcher path. Any existing MCP tool schemas or LLM prompts referencing the old parameter names will need updating. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ShowCommands.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ShowCommands.Notes.md deleted file mode 100644 index 290c62da..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ShowCommands.Notes.md +++ /dev/null @@ -1,18 +0,0 @@ -# UMCPHandler_ShowCommands - Refactoring Notes - -## What was done - -- Reviewed the handler against the coding standards. No changes were needed -- the handler already meets all requirements. - -## What's good - -- Already uses plain-text output (`FStringBuilderBase&`), not JSON. -- No `UE_LOG` calls. -- Concise output: the non-verbose mode produces a compact one-line-per-command summary with parameter names and optional markers (`?`). -- Uses `MCPUtils::CollectHandlerClasses()`, `MCPUtils::GetToolName()`, `MCPUtils::PropertyNameToJsonKey()`, and `MCPUtils::FormatCommandHelp()` -- all the right utilities, no hand-rolled naming. -- The `Verbose` parameter is declared as `Optional`, so callers get the compact listing by default and can opt into full details. -- The handler is simple and self-contained -- under 50 lines total. - -## Uncertainties / conservative choices - -- None. This handler is straightforward and already conforms to the coding standards. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SpawnNodesInGraph.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SpawnNodesInGraph.Notes.md deleted file mode 100644 index a1b7ef83..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SpawnNodesInGraph.Notes.md +++ /dev/null @@ -1,33 +0,0 @@ -# UMCPHandler_SpawnNodesInGraph Refactoring Notes - -## What changed - -1. **Switched from JSON output to plain-text output** (`FStringBuilderBase&`). The old handler returned a JSON tree with `successCount`, `totalCount`, `results[]`, each result containing `nodeId`, `nodeClass`, `nodeTitle`, and a full `SerializeNode` dump. The new handler prints one line per spawned node with its FormatName and class. - -2. **Replaced separate Blueprint + Graph parameters with a single Graph path parameter.** Uses `MCPFetcher::Walk(Graph).Cast()` to resolve the graph directly (e.g. `/Game/Foo,graph:EventGraph`). This eliminates `MCPAssets`, `MCPUtils::UrlDecode`, and `MCPUtils::AllGraphsNamed`. Matches the pattern used by DuplicateNodesInGraph. - -3. **All naming uses MCPUtils::FormatName().** The old code used `NodeGuid.ToString()` for `nodeId` and `GetClass()->GetName()` indirectly. Now everything goes through FormatName. - -4. **Removed UE_LOG calls.** Two `UE_LOG(LogTemp, ...)` calls were removed. - -5. **Removed SerializeNode from output.** The old handler dumped the full serialized node state into the response. This was very verbose. The new handler just reports the node name and class, which is sufficient to confirm the spawn succeeded. The caller can use other tools (dump_graphs, get_pin_details) to inspect the node if needed. - -6. **Trimmed includes.** Removed ~30 includes that were unnecessary (JSON serialization, many K2Node subtypes, material headers, asset registry, etc.). - -7. **Blueprint marking uses GetOuter().** Instead of holding onto the blueprint from an earlier MCPAssets lookup, the refactored code gets the blueprint from `TargetGraph->GetOuter()`, matching the pattern in DuplicateNodesInGraph. - -## What's good - -- Batch support preserved via `FMCPJsonArray Nodes` and `FSpawnNodeEntry`. -- Error messages are concise and actionable (directs to `search_spawnable_node_types`). -- The handler is now about half the original line count. - -## Uncertainties / areas for future work - -- **Parameter rename from `Blueprint`+`Graph` to just `Graph`.** This changes the MCP tool schema. Any existing callers that pass `blueprint=X, graph=Y` will need to switch to `graph=/Game/X,graph:Y`. This is a breaking change to the tool API. - -- **GetOuter() for blueprint.** The assumption is that a blueprint graph's Outer is the UBlueprint. This works for normal blueprint graphs but could fail for level blueprints or nested sub-graphs. The old code had a direct reference to the blueprint from the asset finder. The DuplicateNodesInGraph handler uses the same `Cast(TargetGraph->GetOuter())` pattern, so this should be fine in practice. - -- **SerializeNode removal.** Removing the full node dump from the response makes the output much more concise, but callers that relied on getting back the full node state (pins, default values, etc.) in the spawn response will now need a second call. This seemed like the right trade-off per the "concise output" standard. - -- **FSpawnNodeEntry USTRUCT kept.** The struct is still needed for `PopulateFromJson` to work with the batch entries. It's defined in the same header, which is fine. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/TODO.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/TODO.md deleted file mode 100644 index f3ffbb7b..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/TODO.md +++ /dev/null @@ -1,7 +0,0 @@ -# BlueprintMCP TODO - -- When compiling blueprints, always check for errors properly. Many handlers call `FKismetEditorUtilities::CompileBlueprint` and assume success without checking. The validation handler (`MCPHandlers_Validation.h`) already has proper error collection logic — all compile sites should follow that pattern. - -- Add `FindBlueprintGraph` to `UMCPAssetFinder`: load blueprint, URL-decode graph name, search all graphs by name, return `UEdGraph*` or nullptr with error. Currently ~10 handlers duplicate this 25-line pattern (see `MCPHandlers_Mutation.h:186-210` for a typical example). - -- Look for all uses of `GetAllGraphs`. Every place this is used is terrible — replace with a proper graph lookup helper. diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/paths.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/paths.md deleted file mode 100644 index 5dc31637..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/paths.md +++ /dev/null @@ -1,11 +0,0 @@ - -Fetches a graph pin: - -/Game/Widgets/WB_Hotkeys,graph:ReadLuaConfiguration,node:Self_Reference_03,pin:Result - -Fetches an skeletal mesh: - -/Game/Tangibles/TAN_Character,component:CharacterMesh0,property:SkeletalMeshAsset - - -