diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_BackupAsset.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_BackupAsset.Notes.md new file mode 100644 index 00000000..d33af325 --- /dev/null +++ b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_BackupAsset.Notes.md @@ -0,0 +1,26 @@ +# 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_BackupAsset.h b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_BackupAsset.h index 781e09dd..6a9c1ec6 100644 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_BackupAsset.h +++ b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_BackupAsset.h @@ -2,17 +2,8 @@ #include "CoreMinimal.h" #include "MCPHandler.h" -#include "MCPAssetFinder.h" #include "MCPUtils.h" -#include "Engine/Blueprint.h" -#include "Kismet2/BlueprintEditorUtils.h" -#include "UObject/SavePackage.h" #include "Misc/PackageName.h" -#include "AssetRegistry/AssetRegistryModule.h" -#include "AssetRegistry/IAssetRegistry.h" -#include "AssetToolsModule.h" -#include "IAssetTools.h" -#include "FileHelpers.h" #include "HAL/FileManager.h" #include "UMCPHandler_BackupAsset.generated.h" @@ -35,23 +26,25 @@ public: return TEXT("Copy an asset's .uasset file to a .uasset.bak backup."); } - virtual void Handle(const FJsonObject* Json, FJsonObject* Result) override + virtual void Handle(const FJsonObject* Json, FStringBuilderBase& Result) override { FString Filename = FPaths::ConvertRelativePathToFull( FPackageName::LongPackageNameToFilename(AssetPath, FPackageName::GetAssetPackageExtension())); if (!IFileManager::Get().FileExists(*Filename)) { - return MCPUtils::MakeErrorJson(Result, FString::Printf(TEXT("Asset file not found: %s"), *Filename)); + Result.Appendf(TEXT("ERROR: Asset file not found: %s\n"), *Filename); + return; } FString BackupFilename = Filename + TEXT(".bak"); uint32 CopyResult = IFileManager::Get().Copy(*BackupFilename, *Filename, true); if (CopyResult != COPY_OK) { - return MCPUtils::MakeErrorJson(Result, FString::Printf(TEXT("Failed to back up %s"), *Filename)); + Result.Appendf(TEXT("ERROR: Failed to copy %s to %s\n"), *Filename, *BackupFilename); + return; } - Result->SetStringField(TEXT("backupFile"), BackupFilename); + Result.Appendf(TEXT("Backed up to %s\n"), *BackupFilename); } }; diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CompileMaterial.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CompileMaterial.Notes.md new file mode 100644 index 00000000..466907f7 --- /dev/null +++ b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CompileMaterial.Notes.md @@ -0,0 +1,25 @@ +# 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_CompileMaterial.h b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CompileMaterial.h index 2da04bf1..2a79f8b2 100644 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CompileMaterial.h +++ b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CompileMaterial.h @@ -6,33 +6,6 @@ #include "MCPUtils.h" #include "Materials/Material.h" #include "MaterialDomain.h" -#include "Materials/MaterialInstanceConstant.h" -#include "Materials/MaterialFunction.h" -#include "Materials/MaterialExpression.h" -#include "Materials/MaterialExpressionScalarParameter.h" -#include "Materials/MaterialExpressionVectorParameter.h" -#include "Materials/MaterialExpressionTextureObjectParameter.h" -#include "Materials/MaterialExpressionTextureSampleParameter2D.h" -#include "Materials/MaterialExpressionStaticSwitchParameter.h" -#include "Materials/MaterialExpressionConstant.h" -#include "Materials/MaterialExpressionConstant3Vector.h" -#include "Materials/MaterialExpressionConstant4Vector.h" -#include "Materials/MaterialExpressionTextureSample.h" -#include "Materials/MaterialExpressionTextureCoordinate.h" -#include "Materials/MaterialExpressionComponentMask.h" -#include "Materials/MaterialExpressionCustom.h" -#include "Materials/MaterialExpressionFunctionInput.h" -#include "Materials/MaterialExpressionFunctionOutput.h" -#include "Materials/MaterialExpressionMaterialFunctionCall.h" -#include "MaterialGraph/MaterialGraph.h" -#include "MaterialGraph/MaterialGraphNode.h" -#include "MaterialGraph/MaterialGraphNode_Root.h" -#include "MaterialGraph/MaterialGraphSchema.h" -#include "Kismet2/BlueprintEditorUtils.h" -#include "AssetRegistry/AssetRegistryModule.h" -#include "AssetRegistry/IAssetRegistry.h" -#include "EdGraph/EdGraph.h" -#include "EdGraph/EdGraphNode.h" #include "UMCPHandler_CompileMaterial.generated.h" @@ -54,63 +27,37 @@ public: return TEXT("Force recompile a material and check for compilation errors."); } - virtual void Handle(const FJsonObject* Json, FJsonObject* Result) override + virtual void Handle(const FJsonObject* Json, FStringBuilderBase& Result) override { // Load material MCPAssets Assets; if (!Assets.Exact(Material).Errors(Result).ENone().ETwo().Load()) return; UMaterial* MaterialObj = Assets.Object(); - UE_LOG(LogTemp, Display, TEXT("BlueprintMCP: Validating material '%s'"), *MaterialObj->GetName()); - // Force recompile by triggering PreEditChange/PostEditChange MaterialObj->PreEditChange(nullptr); MaterialObj->PostEditChange(); - // Collect compilation errors - TArray> ErrorArray; - bool bValid = true; - // Check for compilation errors via FMaterialResource on current platform + TArray Errors; FMaterialResource* Resource = MaterialObj->GetMaterialResource(GMaxRHIFeatureLevel); if (Resource) { - const TArray& CompileErrors = Resource->GetCompileErrors(); - for (const FString& Err : CompileErrors) - { - bValid = false; - ErrorArray.Add(MakeShared(Err)); - } + Errors = Resource->GetCompileErrors(); } - // Count expressions and connections - auto Expressions = MaterialObj->GetExpressions(); - int32 ExprCount = Expressions.Num(); - int32 ConnectionCount = 0; - if (MaterialObj->MaterialGraph) + if (Errors.IsEmpty()) { - for (UEdGraphNode* Node : MaterialObj->MaterialGraph->Nodes) + Result.Appendf(TEXT("%s compiled successfully.\n"), *MCPUtils::FormatName(MaterialObj)); + Result.Append(TEXT("WARNING: Error detection is not working. GetCompileErrors() returns empty even when shader compilation fails. Do not trust this result.\n")); + } + else + { + Result.Appendf(TEXT("%s compiled with %d error(s):\n"), *MCPUtils::FormatName(MaterialObj), Errors.Num()); + for (const FString& Err : Errors) { - if (!Node) continue; - for (UEdGraphPin* Pin : Node->Pins) - { - if (Pin && Pin->Direction == EGPD_Output) - { - ConnectionCount += Pin->LinkedTo.Num(); - } - } + Result.Appendf(TEXT(" %s\n"), *Err); } } - - Result->SetBoolField(TEXT("valid"), bValid); - Result->SetStringField(TEXT("material"), MaterialObj->GetName()); - Result->SetStringField(TEXT("materialPath"), MaterialObj->GetPathName()); - Result->SetNumberField(TEXT("expressionCount"), ExprCount); - Result->SetNumberField(TEXT("connectionCount"), ConnectionCount); - Result->SetArrayField(TEXT("errors"), ErrorArray); - Result->SetNumberField(TEXT("errorCount"), ErrorArray.Num()); - - UE_LOG(LogTemp, Display, TEXT("BlueprintMCP: Material '%s' validation %s (%d errors)"), - *MaterialObj->GetName(), bValid ? TEXT("passed") : TEXT("failed"), ErrorArray.Num()); } }; diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateBlueprintGraph.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateBlueprintGraph.Notes.md new file mode 100644 index 00000000..40580710 --- /dev/null +++ b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateBlueprintGraph.Notes.md @@ -0,0 +1,28 @@ +# 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_CreateBlueprintGraph.h b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateBlueprintGraph.h index ebb9e021..2b86893c 100644 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateBlueprintGraph.h +++ b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateBlueprintGraph.h @@ -2,7 +2,7 @@ #include "CoreMinimal.h" #include "MCPHandler.h" -#include "MCPAssetFinder.h" +#include "MCPFetcher.h" #include "MCPUtils.h" #include "Engine/Blueprint.h" #include "EdGraph/EdGraph.h" @@ -10,8 +10,6 @@ #include "EdGraphSchema_K2.h" #include "K2Node_CustomEvent.h" #include "Kismet2/BlueprintEditorUtils.h" -#include "Kismet2/KismetEditorUtilities.h" -#include "UObject/UObjectIterator.h" #include "UMCPHandler_CreateBlueprintGraph.generated.h" @@ -39,53 +37,49 @@ public: return TEXT("Create a new function, macro, or custom event graph in a Blueprint."); } - virtual void Handle(const FJsonObject* Json, FJsonObject* Result) override + virtual void Handle(const FJsonObject* Json, FStringBuilderBase& Result) override { if (GraphType != TEXT("function") && GraphType != TEXT("macro") && GraphType != TEXT("customEvent")) { - return MCPUtils::MakeErrorJson(Result, FString::Printf( - TEXT("Invalid graphType '%s'. Valid values: function, macro, customEvent"), *GraphType)); + Result.Appendf(TEXT("ERROR: Invalid GraphType '%s'. Valid values: function, macro, customEvent\n"), *GraphType); + return; } - // Load Blueprint - MCPAssets Assets; - if (!Assets.Exact(Blueprint).Errors(Result).ENone().ETwo().Load()) return; - UBlueprint* BP = Assets.Object(); + MCPFetcher F(Result); + UBlueprint* BP = F.Walk(Blueprint).Cast(); + if (!BP) return; // Check graph name uniqueness if (!MCPUtils::AllGraphsNamed(BP, Graph).IsEmpty()) { - return MCPUtils::MakeErrorJson(Result, FString::Printf( - TEXT("A graph named '%s' already exists in Blueprint '%s'"), *Graph, *Blueprint)); + Result.Appendf(TEXT("ERROR: A graph named '%s' already exists in %s\n"), *Graph, *MCPUtils::FormatName(BP)); + return; } - // Also check for existing custom events with the same name + // For custom events, also check for existing custom events with the same name if (GraphType == TEXT("customEvent")) { for (UK2Node_CustomEvent* CE : MCPUtils::AllNodes(BP)) { if (CE->CustomFunctionName == FName(*Graph)) { - return MCPUtils::MakeErrorJson(Result, FString::Printf( - TEXT("A custom event named '%s' already exists in Blueprint '%s'"), *Graph, *Blueprint)); + Result.Appendf(TEXT("ERROR: A custom event named '%s' already exists in %s\n"), *Graph, *MCPUtils::FormatName(BP)); + return; } } } - UE_LOG(LogTemp, Display, TEXT("BlueprintMCP: Creating %s graph '%s' in Blueprint '%s'"), - *GraphType, *Graph, *Blueprint); - - FString CreatedNodeId; - if (GraphType == TEXT("function")) { UEdGraph* NewGraph = FBlueprintEditorUtils::CreateNewGraph(BP, FName(*Graph), UEdGraph::StaticClass(), UEdGraphSchema_K2::StaticClass()); if (!NewGraph) { - return MCPUtils::MakeErrorJson(Result, TEXT("Failed to create function graph")); + Result.Append(TEXT("ERROR: Failed to create function graph\n")); + return; } FBlueprintEditorUtils::AddFunctionGraph(BP, NewGraph, /*bIsUserCreated=*/true, /*SignatureFromObject=*/static_cast(nullptr)); + Result.Appendf(TEXT("Created function graph: %s\n"), *MCPUtils::FormatName(NewGraph)); } else if (GraphType == TEXT("macro")) { @@ -93,24 +87,23 @@ public: UEdGraph::StaticClass(), UEdGraphSchema_K2::StaticClass()); if (!NewGraph) { - return MCPUtils::MakeErrorJson(Result, TEXT("Failed to create macro graph")); + Result.Append(TEXT("ERROR: Failed to create macro graph\n")); + return; } FBlueprintEditorUtils::AddMacroGraph(BP, NewGraph, /*bIsUserCreated=*/true, /*SignatureFromClass=*/nullptr); + Result.Appendf(TEXT("Created macro graph: %s\n"), *MCPUtils::FormatName(NewGraph)); } else // customEvent { - // Find the EventGraph (first UbergraphPage) UEdGraph* EventGraph = nullptr; if (BP->UbergraphPages.Num() > 0) - { EventGraph = BP->UbergraphPages[0]; - } if (!EventGraph) { - return MCPUtils::MakeErrorJson(Result, TEXT("Blueprint has no EventGraph to add a custom event to")); + Result.Append(TEXT("ERROR: Blueprint has no EventGraph to add a custom event to\n")); + return; } - // Create a custom event node in the EventGraph UK2Node_CustomEvent* NewEvent = NewObject(EventGraph); NewEvent->CustomFunctionName = FName(*Graph); NewEvent->bIsEditable = true; @@ -118,19 +111,10 @@ public: NewEvent->CreateNewGuid(); NewEvent->PostPlacedNewNode(); NewEvent->AllocateDefaultPins(); - CreatedNodeId = NewEvent->NodeGuid.ToString(); + Result.Appendf(TEXT("Created custom event: %s\n"), *MCPUtils::FormatName(NewEvent)); } FBlueprintEditorUtils::MarkBlueprintAsStructurallyModified(BP); - bool bSaved = MCPUtils::SaveBlueprintPackage(BP); - - UE_LOG(LogTemp, Display, TEXT("BlueprintMCP: Created %s graph '%s' in '%s' (saved: %s)"), - *GraphType, *Graph, *Blueprint, bSaved ? TEXT("true") : TEXT("false")); - - Result->SetBoolField(TEXT("saved"), bSaved); - if (!CreatedNodeId.IsEmpty()) - { - Result->SetStringField(TEXT("nodeId"), CreatedNodeId); - } + MCPUtils::SaveBlueprintPackage(BP); } }; diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateMaterialAsset.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateMaterialAsset.Notes.md new file mode 100644 index 00000000..6385c805 --- /dev/null +++ b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateMaterialAsset.Notes.md @@ -0,0 +1,60 @@ +# 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_CreateMaterialAsset.h b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateMaterialAsset.h index e21c3a70..bd21d7c0 100644 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateMaterialAsset.h +++ b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_CreateMaterialAsset.h @@ -6,55 +6,12 @@ #include "MCPUtils.h" #include "Materials/Material.h" #include "MaterialDomain.h" -#include "Materials/MaterialInstanceConstant.h" -#include "Materials/MaterialFunction.h" -#include "Materials/MaterialExpression.h" -#include "Materials/MaterialExpressionScalarParameter.h" -#include "Materials/MaterialExpressionVectorParameter.h" -#include "Materials/MaterialExpressionTextureObjectParameter.h" -#include "Materials/MaterialExpressionTextureSampleParameter2D.h" -#include "Materials/MaterialExpressionStaticSwitchParameter.h" -#include "Materials/MaterialExpressionConstant.h" -#include "Materials/MaterialExpressionConstant3Vector.h" -#include "Materials/MaterialExpressionConstant4Vector.h" -#include "Materials/MaterialExpressionTextureSample.h" -#include "Materials/MaterialExpressionTextureCoordinate.h" -#include "Materials/MaterialExpressionComponentMask.h" -#include "Materials/MaterialExpressionCustom.h" -#include "Materials/MaterialExpressionFunctionInput.h" -#include "Materials/MaterialExpressionFunctionOutput.h" -#include "Materials/MaterialExpressionMaterialFunctionCall.h" -#include "MaterialGraph/MaterialGraph.h" -#include "MaterialGraph/MaterialGraphNode.h" -#include "MaterialGraph/MaterialGraphSchema.h" #include "Factories/MaterialFactoryNew.h" -#include "Factories/MaterialFunctionFactoryNew.h" #include "AssetToolsModule.h" #include "IAssetTools.h" -#include "AssetRegistry/AssetRegistryModule.h" -#include "EdGraph/EdGraph.h" -#include "EdGraph/EdGraphNode.h" -#include "Serialization/JsonReader.h" -#include "Serialization/JsonWriter.h" -#include "Serialization/JsonSerializer.h" -#include "Misc/Guid.h" -#include "Misc/FileHelper.h" -#include "Misc/Paths.h" -#include "UObject/SavePackage.h" -#include "UObject/UObjectIterator.h" -#include "Kismet2/BlueprintEditorUtils.h" #include "UMCPHandler_CreateMaterialAsset.generated.h" -// SEH wrapper defined in BlueprintMCPServer.cpp — catches crashes from abstract/invalid expression classes. -// Wraps the entire creation + registration + PostEditChange flow so that if the expression crashes -// (e.g. UMaterialExpressionParameter), it cleans up and returns -1 instead of terminating the process. -#if PLATFORM_WINDOWS -extern int32 TryAddMaterialExpressionSEH( - UObject* Owner, UClass* ExprClass, UMaterial* Material, UMaterialFunction* MatFunc, - int32 PosX, int32 PosY, UMaterialExpression** OutExpr); -#endif - // --------------------------------------------------------------------------- // --------------------------------------------------------------------------- // --------------------------------------------------------------------------- @@ -85,91 +42,68 @@ public: return TEXT("Create a new UMaterial asset with optional domain, blend mode, and two-sided settings."); } - virtual void Handle(const FJsonObject* Json, FJsonObject* Result) override + virtual void Handle(const FJsonObject* Json, FStringBuilderBase& Result) override { if (!PackagePath.StartsWith(TEXT("/Game"))) { - return MCPUtils::MakeErrorJson(Result, TEXT("packagePath must start with '/Game'")); + Result.Append(TEXT("ERROR: PackagePath must start with '/Game'\n")); + return; } - // Check if asset already exists + // Parse optional enum properties before creating the asset. + EMaterialDomain ParsedDomain = MD_Surface; + if (!Domain.IsEmpty()) + { + if (!MCPUtils::StringToEnum(Domain, ParsedDomain, Result, TEXT("MD_"))) + return; + } + + EBlendMode ParsedBlendMode = BLEND_Opaque; + if (!BlendMode.IsEmpty()) + { + if (!MCPUtils::StringToEnum(BlendMode, ParsedBlendMode, Result, TEXT("BLEND_"))) + return; + } + + // Check if an asset with this name already exists. MCPAssets ExistCheck; if (!ExistCheck.Exact(Name).Errors(Result).EAny().Info()) return; - UE_LOG(LogTemp, Display, TEXT("BlueprintMCP: Creating Material '%s' in '%s'"), *Name, *PackagePath); - - // Create via IAssetTools + factory + // Create via IAssetTools + factory. IAssetTools& AssetTools = FModuleManager::LoadModuleChecked("AssetTools").Get(); UMaterialFactoryNew* Factory = NewObject(); UObject* NewAsset = AssetTools.CreateAsset(Name, PackagePath, UMaterial::StaticClass(), Factory); - if (!NewAsset) - { - return MCPUtils::MakeErrorJson(Result, FString::Printf(TEXT("Failed to create Material '%s' in '%s'"), *Name, *PackagePath)); - } - UMaterial* MaterialObj = Cast(NewAsset); if (!MaterialObj) { - return MCPUtils::MakeErrorJson(Result, TEXT("Created asset is not a UMaterial")); + Result.Appendf(TEXT("ERROR: Failed to create Material '%s' in '%s'\n"), *Name, *PackagePath); + return; } - // Apply optional properties + // Apply optional properties. bool bHasTwoSided = Json->HasField(TEXT("twoSided")); MaterialObj->PreEditChange(nullptr); - // Parse domain if (!Domain.IsEmpty()) - { - if (Domain == TEXT("Surface")) - MaterialObj->MaterialDomain = MD_Surface; - else if (Domain == TEXT("DeferredDecal")) - MaterialObj->MaterialDomain = MD_DeferredDecal; - else if (Domain == TEXT("LightFunction")) - MaterialObj->MaterialDomain = MD_LightFunction; - else if (Domain == TEXT("Volume")) - MaterialObj->MaterialDomain = MD_Volume; - else if (Domain == TEXT("PostProcess")) - MaterialObj->MaterialDomain = MD_PostProcess; - else if (Domain == TEXT("UI")) - MaterialObj->MaterialDomain = MD_UI; - } + MaterialObj->MaterialDomain = ParsedDomain; - // Parse blend mode if (!BlendMode.IsEmpty()) - { - if (BlendMode == TEXT("Opaque")) - MaterialObj->BlendMode = BLEND_Opaque; - else if (BlendMode == TEXT("Masked")) - MaterialObj->BlendMode = BLEND_Masked; - else if (BlendMode == TEXT("Translucent")) - MaterialObj->BlendMode = BLEND_Translucent; - else if (BlendMode == TEXT("Additive")) - MaterialObj->BlendMode = BLEND_Additive; - else if (BlendMode == TEXT("Modulate")) - MaterialObj->BlendMode = BLEND_Modulate; - } + MaterialObj->BlendMode = ParsedBlendMode; if (bHasTwoSided) - { MaterialObj->TwoSided = TwoSided; - } MaterialObj->PostEditChange(); - // Save bool bSaved = MCPUtils::SaveMaterialPackage(MaterialObj); - - - UE_LOG(LogTemp, Display, TEXT("BlueprintMCP: Created Material '%s' (saved: %s)"), - *Name, bSaved ? TEXT("true") : TEXT("false")); - - Result->SetStringField(TEXT("path"), MaterialObj->GetPathName()); - Result->SetStringField(TEXT("domain"), MCPUtils::EnumToString(MaterialObj->MaterialDomain, TEXT("MD_"))); - Result->SetStringField(TEXT("blendMode"), MCPUtils::EnumToString(MaterialObj->BlendMode, TEXT("BLEND_"))); - Result->SetBoolField(TEXT("twoSided"), MaterialObj->TwoSided != 0); - Result->SetBoolField(TEXT("saved"), bSaved); + Result.Appendf(TEXT("Created %s\n"), *MaterialObj->GetPathName()); + Result.Appendf(TEXT("Domain: %s\n"), *MCPUtils::EnumToString(MaterialObj->MaterialDomain, TEXT("MD_"))); + Result.Appendf(TEXT("BlendMode: %s\n"), *MCPUtils::EnumToString(MaterialObj->BlendMode, TEXT("BLEND_"))); + Result.Appendf(TEXT("TwoSided: %s\n"), MaterialObj->TwoSided ? TEXT("true") : TEXT("false")); + if (!bSaved) + Result.Append(TEXT("WARNING: Package save failed\n")); } }; diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DeleteBlueprintGraph.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DeleteBlueprintGraph.Notes.md new file mode 100644 index 00000000..7f2a6088 --- /dev/null +++ b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DeleteBlueprintGraph.Notes.md @@ -0,0 +1,30 @@ +# 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_DeleteBlueprintGraph.h b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DeleteBlueprintGraph.h index baafd65f..6ac2b242 100644 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DeleteBlueprintGraph.h +++ b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DeleteBlueprintGraph.h @@ -2,16 +2,11 @@ #include "CoreMinimal.h" #include "MCPHandler.h" -#include "MCPAssetFinder.h" +#include "MCPFetcher.h" #include "MCPUtils.h" #include "Engine/Blueprint.h" #include "EdGraph/EdGraph.h" -#include "EdGraph/EdGraphNode.h" -#include "EdGraphSchema_K2.h" -#include "K2Node_CustomEvent.h" #include "Kismet2/BlueprintEditorUtils.h" -#include "Kismet2/KismetEditorUtilities.h" -#include "UObject/UObjectIterator.h" #include "UMCPHandler_DeleteBlueprintGraph.generated.h" @@ -25,8 +20,8 @@ class UMCPHandler_DeleteBlueprintGraph : public UObject, public IMCPHandler GENERATED_BODY() public: - UPROPERTY(meta=(Description="Blueprint name or package path")) - FString Blueprint; + UPROPERTY(meta=(Description="Path to a blueprint, e.g. /Game/Foo/Bar")) + FString Path; UPROPERTY(meta=(Description="Name of the graph to delete")) FString Graph; @@ -36,32 +31,35 @@ public: return TEXT("Delete a function or macro graph from a Blueprint. Cannot delete EventGraph pages."); } - virtual void Handle(const FJsonObject* Json, FJsonObject* Result) override + virtual void Handle(const FJsonObject* Json, FStringBuilderBase& Result) override { - MCPAssets Assets; - if (!Assets.Exact(Blueprint).Errors(Result).ENone().ETwo().Load()) return; - UBlueprint* BP = Assets.Object(); + MCPFetcher F(Result); + F.Walk(Path); + if (!F.Ok()) return; - // Find the graph + UBlueprint* BP = F.Cast(); + if (!BP) return; + + // Search function graphs, then macro graphs UEdGraph* TargetGraph = nullptr; FString GraphType; - for (UEdGraph* CandidateGraph : BP->FunctionGraphs) + for (UEdGraph* G : BP->FunctionGraphs) { - if (CandidateGraph && MCPUtils::Identifies(Graph, CandidateGraph)) + if (G && MCPUtils::Identifies(Graph, G)) { - TargetGraph = CandidateGraph; + TargetGraph = G; GraphType = TEXT("function"); break; } } if (!TargetGraph) { - for (UEdGraph* CandidateGraph : BP->MacroGraphs) + for (UEdGraph* G : BP->MacroGraphs) { - if (CandidateGraph && MCPUtils::Identifies(Graph, CandidateGraph)) + if (G && MCPUtils::Identifies(Graph, G)) { - TargetGraph = CandidateGraph; + TargetGraph = G; GraphType = TEXT("macro"); break; } @@ -71,35 +69,28 @@ public: // Check if it's an UbergraphPage (EventGraph) — disallow deletion if (!TargetGraph) { - for (UEdGraph* CandidateGraph : BP->UbergraphPages) + for (UEdGraph* G : BP->UbergraphPages) { - if (CandidateGraph && MCPUtils::Identifies(Graph, CandidateGraph)) + if (G && MCPUtils::Identifies(Graph, G)) { - return MCPUtils::MakeErrorJson(Result, FString::Printf( - TEXT("Cannot delete UbergraphPage '%s'. EventGraph and other Ubergraph pages cannot be deleted."), - *Graph)); + Result.Appendf(TEXT("ERROR: Cannot delete UbergraphPage '%s'. EventGraph pages cannot be deleted.\n"), + *MCPUtils::FormatName(G)); + return; } } - return MCPUtils::MakeErrorJson(Result, FString::Printf(TEXT("Graph '%s' not found in Blueprint '%s'"), *Graph, *Blueprint)); + Result.Appendf(TEXT("ERROR: Graph '%s' not found in blueprint %s\n"), + *Graph, *MCPUtils::FormatName(BP)); + return; } - UE_LOG(LogTemp, Display, TEXT("BlueprintMCP: Deleting %s graph '%s' from Blueprint '%s'"), - *GraphType, *Graph, *Blueprint); - - // Count nodes for reporting - int32 NodeCount = TargetGraph->Nodes.Num(); - // Remove the graph + FString GraphName = MCPUtils::FormatName(TargetGraph); FBlueprintEditorUtils::RemoveGraph(BP, TargetGraph, EGraphRemoveFlags::Default); - FBlueprintEditorUtils::MarkBlueprintAsStructurallyModified(BP); bool bSaved = MCPUtils::SaveBlueprintPackage(BP); - UE_LOG(LogTemp, Display, TEXT("BlueprintMCP: Deleted graph '%s' (%d nodes), save %s"), - *Graph, NodeCount, bSaved ? TEXT("true") : TEXT("false")); - - Result->SetStringField(TEXT("graphType"), GraphType); - Result->SetNumberField(TEXT("nodeCount"), NodeCount); - Result->SetBoolField(TEXT("saved"), bSaved); + Result.Appendf(TEXT("Deleted %s graph %s\n"), *GraphType, *GraphName); + if (!bSaved) + Result.Append(TEXT("WARNING: Package save failed.\n")); } }; diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DumpMaterial.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DumpMaterial.Notes.md new file mode 100644 index 00000000..a3e89e5c --- /dev/null +++ b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DumpMaterial.Notes.md @@ -0,0 +1,40 @@ +# 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_DumpMaterial.h b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DumpMaterial.h index 6f94c28d..fe3a410b 100644 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DumpMaterial.h +++ b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DumpMaterial.h @@ -7,33 +7,13 @@ #include "Materials/Material.h" #include "MaterialDomain.h" #include "Materials/MaterialInstanceConstant.h" -#include "Materials/MaterialFunction.h" #include "Materials/MaterialExpression.h" #include "Materials/MaterialExpressionScalarParameter.h" #include "Materials/MaterialExpressionVectorParameter.h" -#include "Materials/MaterialExpressionTextureObjectParameter.h" -#include "Engine/Texture.h" #include "Materials/MaterialExpressionTextureSampleParameter2D.h" #include "Materials/MaterialExpressionStaticSwitchParameter.h" -#include "Materials/MaterialExpressionConstant.h" -#include "Materials/MaterialExpressionConstant3Vector.h" -#include "Materials/MaterialExpressionConstant4Vector.h" #include "Materials/MaterialExpressionTextureSample.h" -#include "Materials/MaterialExpressionTextureCoordinate.h" -#include "Materials/MaterialExpressionComponentMask.h" -#include "Materials/MaterialExpressionCustom.h" -#include "Materials/MaterialExpressionFunctionInput.h" -#include "Materials/MaterialExpressionFunctionOutput.h" -#include "Materials/MaterialExpressionMaterialFunctionCall.h" -#include "MaterialGraph/MaterialGraph.h" -#include "MaterialGraph/MaterialGraphNode.h" -#include "MaterialGraph/MaterialGraphNode_Root.h" -#include "MaterialGraph/MaterialGraphSchema.h" -#include "Kismet2/BlueprintEditorUtils.h" -#include "AssetRegistry/AssetRegistryModule.h" -#include "AssetRegistry/IAssetRegistry.h" -#include "EdGraph/EdGraph.h" -#include "EdGraph/EdGraphNode.h" +#include "Engine/Texture.h" #include "UMCPHandler_DumpMaterial.generated.h" @@ -55,240 +35,194 @@ public: return TEXT("Get detailed info about a material or material instance, including parameters, usage flags, and referenced textures."); } - virtual void Handle(const FJsonObject* Json, FJsonObject* Result) override + virtual void Handle(const FJsonObject* Json, FStringBuilderBase& Result) override { - FString DecodedName = MCPUtils::UrlDecode(Material); - - // Try loading as UMaterial or UMaterialInstanceConstant MCPAssets Assets; - Assets.Scan(UMaterial::StaticClass()); - Assets.Scan(UMaterialInstanceConstant::StaticClass()); - if (!Assets.Exact(DecodedName).Errors(Result).ENone().ETwo().Load()) return; + Assets.NoScans(); + Assets.Scan(); + Assets.Scan(); + if (!Assets.Exact(Material).Errors(Result).ENone().ETwo().Load()) return; UMaterialInterface* LoadedObj = Assets.Object(); - if (UMaterial* MaterialObj = Cast(LoadedObj)) + if (UMaterial* Mat = Cast(LoadedObj)) { - UE_LOG(LogTemp, Display, TEXT("BlueprintMCP: GetMaterial — loaded material '%s'"), *MaterialObj->GetName()); - - Result->SetStringField(TEXT("name"), MaterialObj->GetName()); - Result->SetStringField(TEXT("path"), MaterialObj->GetPathName()); - Result->SetStringField(TEXT("type"), TEXT("Material")); - - // Material domain - FString DomainStr = TEXT("Unknown"); - if (const UEnum* DomainEnum = StaticEnum()) - { - DomainStr = DomainEnum->GetNameStringByValue((int64)MaterialObj->MaterialDomain); - } - Result->SetStringField(TEXT("domain"), DomainStr); - - // Blend mode - FString BlendModeStr = TEXT("Unknown"); - if (const UEnum* BlendEnum = StaticEnum()) - { - BlendModeStr = BlendEnum->GetNameStringByValue((int64)MaterialObj->BlendMode); - } - Result->SetStringField(TEXT("blendMode"), BlendModeStr); - - // Shading models - TArray> ShadingModels; - FMaterialShadingModelField SMField = MaterialObj->GetShadingModels(); - if (const UEnum* SMEnum = StaticEnum()) - { - for (int32 i = 0; i < SMEnum->NumEnums() - 1; ++i) - { - EMaterialShadingModel SM = (EMaterialShadingModel)SMEnum->GetValueByIndex(i); - if (SMField.HasShadingModel(SM)) - { - ShadingModels.Add(MakeShared(SMEnum->GetNameStringByIndex(i))); - } - } - } - Result->SetArrayField(TEXT("shadingModels"), ShadingModels); - - // Two-sided - Result->SetBoolField(TEXT("twoSided"), MaterialObj->IsTwoSided()); - - // Expression count - auto Expressions = MaterialObj->GetExpressions(); - Result->SetNumberField(TEXT("expressionCount"), Expressions.Num()); - - // Parameters — iterate expressions for parameter types - TArray> Parameters; - for (UMaterialExpression* Expr : Expressions) - { - if (!Expr) continue; - - TSharedRef ParamObj = MakeShared(); - bool bIsParam = false; - - if (auto* SP = Cast(Expr)) - { - bIsParam = true; - ParamObj->SetStringField(TEXT("name"), SP->ParameterName.ToString()); - ParamObj->SetStringField(TEXT("type"), TEXT("Scalar")); - ParamObj->SetStringField(TEXT("group"), SP->Group.ToString()); - ParamObj->SetNumberField(TEXT("defaultValue"), SP->DefaultValue); - } - else if (auto* VP = Cast(Expr)) - { - bIsParam = true; - ParamObj->SetStringField(TEXT("name"), VP->ParameterName.ToString()); - ParamObj->SetStringField(TEXT("type"), TEXT("Vector")); - ParamObj->SetStringField(TEXT("group"), VP->Group.ToString()); - TSharedRef DefVal = MakeShared(); - DefVal->SetNumberField(TEXT("r"), VP->DefaultValue.R); - DefVal->SetNumberField(TEXT("g"), VP->DefaultValue.G); - DefVal->SetNumberField(TEXT("b"), VP->DefaultValue.B); - DefVal->SetNumberField(TEXT("a"), VP->DefaultValue.A); - ParamObj->SetObjectField(TEXT("defaultValue"), DefVal); - } - else if (auto* TP = Cast(Expr)) - { - bIsParam = true; - ParamObj->SetStringField(TEXT("name"), TP->ParameterName.ToString()); - ParamObj->SetStringField(TEXT("type"), TEXT("Texture")); - ParamObj->SetStringField(TEXT("group"), TP->Group.ToString()); - if (TP->Texture) - ParamObj->SetStringField(TEXT("defaultValue"), TP->Texture->GetPathName()); - } - else if (auto* SSP = Cast(Expr)) - { - bIsParam = true; - ParamObj->SetStringField(TEXT("name"), SSP->ParameterName.ToString()); - ParamObj->SetStringField(TEXT("type"), TEXT("StaticSwitch")); - ParamObj->SetStringField(TEXT("group"), SSP->Group.ToString()); - ParamObj->SetBoolField(TEXT("defaultValue"), SSP->DefaultValue); - } - - if (bIsParam) - { - Parameters.Add(MakeShared(ParamObj)); - } - } - Result->SetArrayField(TEXT("parameters"), Parameters); - - // Referenced textures - TArray> ReferencedTextures; - auto RefTexObjs = MaterialObj->GetReferencedTextures(); - for (const TObjectPtr& TexObj : RefTexObjs) - { - if (TexObj) - { - ReferencedTextures.Add(MakeShared(TexObj->GetPathName())); - } - } - Result->SetArrayField(TEXT("referencedTextures"), ReferencedTextures); - - // Graph node count - int32 GraphNodeCount = 0; - if (MaterialObj->MaterialGraph) - { - GraphNodeCount = MaterialObj->MaterialGraph->Nodes.Num(); - } - Result->SetNumberField(TEXT("graphNodeCount"), GraphNodeCount); - - // Usage flags - TSharedRef UsageFlags = MakeShared(); - UsageFlags->SetBoolField(TEXT("bUsedWithSkeletalMesh"), MaterialObj->bUsedWithSkeletalMesh != 0); - UsageFlags->SetBoolField(TEXT("bUsedWithMorphTargets"), MaterialObj->bUsedWithMorphTargets != 0); - UsageFlags->SetBoolField(TEXT("bUsedWithNiagaraSprites"), MaterialObj->bUsedWithNiagaraSprites != 0); - UsageFlags->SetBoolField(TEXT("bUsedWithParticleSprites"), MaterialObj->bUsedWithParticleSprites != 0); - UsageFlags->SetBoolField(TEXT("bUsedWithStaticLighting"), MaterialObj->bUsedWithStaticLighting != 0); - Result->SetObjectField(TEXT("usageFlags"), UsageFlags); - - // Opacity mask clip value - Result->SetNumberField(TEXT("opacityMaskClipValue"), MaterialObj->OpacityMaskClipValue); - - // Additional settings - Result->SetBoolField(TEXT("ditheredLODTransition"), MaterialObj->DitheredLODTransition != 0); - Result->SetBoolField(TEXT("bAllowNegativeEmissiveColor"), MaterialObj->bAllowNegativeEmissiveColor != 0); - - // Texture sample count (simple expression scan) - int32 TextureSampleCount = 0; - for (UMaterialExpression* Expr : Expressions) - { - if (Expr && Expr->IsA()) - { - TextureSampleCount++; - } - } - Result->SetNumberField(TEXT("textureSampleCount"), TextureSampleCount); - + EmitMaterial(Mat, Result); return; } if (UMaterialInstanceConstant* MI = Cast(LoadedObj)) { - UE_LOG(LogTemp, Display, TEXT("BlueprintMCP: GetMaterial — loaded material instance '%s'"), *MI->GetName()); - - Result->SetStringField(TEXT("name"), MI->GetName()); - Result->SetStringField(TEXT("path"), MI->GetPathName()); - Result->SetStringField(TEXT("type"), TEXT("MaterialInstance")); - - if (MI->Parent) - { - Result->SetStringField(TEXT("parent"), MI->Parent->GetName()); - Result->SetStringField(TEXT("parentPath"), MI->Parent->GetPathName()); - } - - // Overridden parameters - TArray> OverriddenParams; - - // Scalar parameters - for (const FScalarParameterValue& Param : MI->ScalarParameterValues) - { - TSharedRef PObj = MakeShared(); - PObj->SetStringField(TEXT("name"), Param.ParameterInfo.Name.ToString()); - PObj->SetStringField(TEXT("type"), TEXT("Scalar")); - PObj->SetNumberField(TEXT("value"), Param.ParameterValue); - OverriddenParams.Add(MakeShared(PObj)); - } - - // Vector parameters - for (const FVectorParameterValue& Param : MI->VectorParameterValues) - { - TSharedRef PObj = MakeShared(); - PObj->SetStringField(TEXT("name"), Param.ParameterInfo.Name.ToString()); - PObj->SetStringField(TEXT("type"), TEXT("Vector")); - TSharedRef Val = MakeShared(); - Val->SetNumberField(TEXT("r"), Param.ParameterValue.R); - Val->SetNumberField(TEXT("g"), Param.ParameterValue.G); - Val->SetNumberField(TEXT("b"), Param.ParameterValue.B); - Val->SetNumberField(TEXT("a"), Param.ParameterValue.A); - PObj->SetObjectField(TEXT("value"), Val); - OverriddenParams.Add(MakeShared(PObj)); - } - - // Texture parameters - for (const FTextureParameterValue& Param : MI->TextureParameterValues) - { - TSharedRef PObj = MakeShared(); - PObj->SetStringField(TEXT("name"), Param.ParameterInfo.Name.ToString()); - PObj->SetStringField(TEXT("type"), TEXT("Texture")); - if (Param.ParameterValue) - PObj->SetStringField(TEXT("value"), Param.ParameterValue->GetPathName()); - else - PObj->SetStringField(TEXT("value"), TEXT("None")); - OverriddenParams.Add(MakeShared(PObj)); - } - - // Static switch parameters - for (const FStaticSwitchParameter& Param : MI->GetStaticParameters().StaticSwitchParameters) - { - TSharedRef PObj = MakeShared(); - PObj->SetStringField(TEXT("name"), Param.ParameterInfo.Name.ToString()); - PObj->SetStringField(TEXT("type"), TEXT("StaticSwitch")); - PObj->SetBoolField(TEXT("value"), Param.Value); - PObj->SetBoolField(TEXT("overridden"), Param.bOverride); - OverriddenParams.Add(MakeShared(PObj)); - } - - Result->SetArrayField(TEXT("overriddenParameters"), OverriddenParams); - + EmitMaterialInstance(MI, Result); return; } - MCPUtils::MakeErrorJson(Result, FString::Printf(TEXT("Material or MaterialInstance '%s' not found. Use list_materials to see available assets."), *DecodedName)); + Result.Appendf(TEXT("ERROR: Loaded object is %s, expected Material or MaterialInstance.\n"), + *LoadedObj->GetClass()->GetName()); + } + +private: + void EmitMaterial(UMaterial* Mat, FStringBuilderBase& Result) + { + Result.Appendf(TEXT("Material: %s\n"), *MCPUtils::FormatName(Mat)); + Result.Appendf(TEXT("Domain: %s\n"), *MCPUtils::EnumToString(Mat->MaterialDomain, TEXT("MD_"))); + Result.Appendf(TEXT("BlendMode: %s\n"), *MCPUtils::EnumToString(Mat->BlendMode, TEXT("BLEND_"))); + Result.Appendf(TEXT("TwoSided: %s\n"), Mat->IsTwoSided() ? TEXT("true") : TEXT("false")); + + // Shading models + FMaterialShadingModelField SMField = Mat->GetShadingModels(); + const UEnum* SMEnum = StaticEnum(); + TArray SMNames; + for (int32 i = 0; i < SMEnum->NumEnums() - 1; ++i) + { + EMaterialShadingModel SM = (EMaterialShadingModel)SMEnum->GetValueByIndex(i); + if (SMField.HasShadingModel(SM)) + SMNames.Add(SMEnum->GetNameStringByIndex(i)); + } + Result.Appendf(TEXT("ShadingModels: %s\n"), *FString::Join(SMNames, TEXT(", "))); + + // Parameters + auto Expressions = Mat->GetExpressions(); + bool bHasParams = false; + for (UMaterialExpression* Expr : Expressions) + { + if (!Expr) continue; + + if (auto* SP = Cast(Expr)) + { + if (!bHasParams) { Result.Append(TEXT("\nParameters:\n")); bHasParams = true; } + Result.Appendf(TEXT(" Scalar \"%s\" = %g"), *SP->ParameterName.ToString(), SP->DefaultValue); + if (!SP->Group.IsNone()) Result.Appendf(TEXT(" [%s]"), *SP->Group.ToString()); + Result.Append(TEXT("\n")); + } + else if (auto* VP = Cast(Expr)) + { + if (!bHasParams) { Result.Append(TEXT("\nParameters:\n")); bHasParams = true; } + Result.Appendf(TEXT(" Vector \"%s\" = (%.3f, %.3f, %.3f, %.3f)"), + *VP->ParameterName.ToString(), + VP->DefaultValue.R, VP->DefaultValue.G, VP->DefaultValue.B, VP->DefaultValue.A); + if (!VP->Group.IsNone()) Result.Appendf(TEXT(" [%s]"), *VP->Group.ToString()); + Result.Append(TEXT("\n")); + } + else if (auto* TP = Cast(Expr)) + { + if (!bHasParams) { Result.Append(TEXT("\nParameters:\n")); bHasParams = true; } + Result.Appendf(TEXT(" Texture \"%s\" = %s"), + *TP->ParameterName.ToString(), + TP->Texture ? *MCPUtils::FormatName(TP->Texture) : TEXT("None")); + if (!TP->Group.IsNone()) Result.Appendf(TEXT(" [%s]"), *TP->Group.ToString()); + Result.Append(TEXT("\n")); + } + else if (auto* SSP = Cast(Expr)) + { + if (!bHasParams) { Result.Append(TEXT("\nParameters:\n")); bHasParams = true; } + Result.Appendf(TEXT(" StaticSwitch \"%s\" = %s"), + *SSP->ParameterName.ToString(), + SSP->DefaultValue ? TEXT("true") : TEXT("false")); + if (!SSP->Group.IsNone()) Result.Appendf(TEXT(" [%s]"), *SSP->Group.ToString()); + Result.Append(TEXT("\n")); + } + } + + // Referenced textures + auto RefTexObjs = Mat->GetReferencedTextures(); + bool bHasTextures = false; + for (const TObjectPtr& TexObj : RefTexObjs) + { + if (!TexObj) continue; + if (!bHasTextures) { Result.Append(TEXT("\nReferenced Textures:\n")); bHasTextures = true; } + if (UTexture* Tex = Cast(TexObj.Get())) + Result.Appendf(TEXT(" %s\n"), *MCPUtils::FormatName(Tex)); + else + Result.Appendf(TEXT(" %s\n"), *TexObj->GetPathName()); + } + + // Usage flags — only print enabled ones + Result.Append(TEXT("\nUsage Flags:")); + bool bAnyUsage = false; + auto EmitFlag = [&](bool bSet, const TCHAR* Name) { + if (bSet) { Result.Appendf(TEXT(" %s"), Name); bAnyUsage = true; } + }; + EmitFlag(Mat->bUsedWithSkeletalMesh, TEXT("SkeletalMesh")); + EmitFlag(Mat->bUsedWithMorphTargets, TEXT("MorphTargets")); + EmitFlag(Mat->bUsedWithNiagaraSprites, TEXT("NiagaraSprites")); + EmitFlag(Mat->bUsedWithParticleSprites, TEXT("ParticleSprites")); + EmitFlag(Mat->bUsedWithStaticLighting, TEXT("StaticLighting")); + if (!bAnyUsage) Result.Append(TEXT(" (none)")); + Result.Append(TEXT("\n")); + + // Stats + Result.Appendf(TEXT("Expressions: %d\n"), Expressions.Num()); + int32 TextureSampleCount = 0; + for (UMaterialExpression* Expr : Expressions) + if (Expr && Expr->IsA()) + TextureSampleCount++; + Result.Appendf(TEXT("TextureSamples: %d\n"), TextureSampleCount); + if (Mat->MaterialGraph) + Result.Appendf(TEXT("GraphNodes: %d\n"), Mat->MaterialGraph->Nodes.Num()); + + // Additional settings — only print non-default values + if (Mat->OpacityMaskClipValue != 0.3333f) + Result.Appendf(TEXT("OpacityMaskClipValue: %g\n"), Mat->OpacityMaskClipValue); + if (Mat->DitheredLODTransition) + Result.Append(TEXT("DitheredLODTransition: true\n")); + if (Mat->bAllowNegativeEmissiveColor) + Result.Append(TEXT("AllowNegativeEmissiveColor: true\n")); + } + + void EmitMaterialInstance(UMaterialInstanceConstant* MI, FStringBuilderBase& Result) + { + Result.Appendf(TEXT("MaterialInstance: %s\n"), *MCPUtils::FormatName(MI)); + if (MI->Parent) + { + if (UMaterial* ParentMat = Cast(MI->Parent)) + Result.Appendf(TEXT("Parent: %s\n"), *MCPUtils::FormatName(ParentMat)); + else if (UMaterialInstance* ParentMI = Cast(MI->Parent)) + Result.Appendf(TEXT("Parent: %s\n"), *MCPUtils::FormatName(ParentMI)); + else + Result.Appendf(TEXT("Parent: %s\n"), *MI->Parent->GetPathName()); + } + + // Overridden parameters + bool bHasParams = false; + auto EnsureHeader = [&]() { + if (!bHasParams) { Result.Append(TEXT("\nOverridden Parameters:\n")); bHasParams = true; } + }; + + for (const FScalarParameterValue& P : MI->ScalarParameterValues) + { + EnsureHeader(); + Result.Appendf(TEXT(" Scalar \"%s\" = %g\n"), *P.ParameterInfo.Name.ToString(), P.ParameterValue); + } + + for (const FVectorParameterValue& P : MI->VectorParameterValues) + { + EnsureHeader(); + Result.Appendf(TEXT(" Vector \"%s\" = (%.3f, %.3f, %.3f, %.3f)\n"), + *P.ParameterInfo.Name.ToString(), + P.ParameterValue.R, P.ParameterValue.G, P.ParameterValue.B, P.ParameterValue.A); + } + + for (const FTextureParameterValue& P : MI->TextureParameterValues) + { + EnsureHeader(); + if (P.ParameterValue) + { + Result.Appendf(TEXT(" Texture \"%s\" = %s\n"), + *P.ParameterInfo.Name.ToString(), *MCPUtils::FormatName(P.ParameterValue)); + } + else + { + Result.Appendf(TEXT(" Texture \"%s\" = None\n"), *P.ParameterInfo.Name.ToString()); + } + } + + for (const FStaticSwitchParameter& P : MI->GetStaticParameters().StaticSwitchParameters) + { + EnsureHeader(); + Result.Appendf(TEXT(" StaticSwitch \"%s\" = %s%s\n"), + *P.ParameterInfo.Name.ToString(), + P.Value ? TEXT("true") : TEXT("false"), + P.bOverride ? TEXT("") : TEXT(" (not overridden)")); + } } }; diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_FindMaterialReferences.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_FindMaterialReferences.Notes.md new file mode 100644 index 00000000..2c8d46e3 --- /dev/null +++ b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_FindMaterialReferences.Notes.md @@ -0,0 +1,28 @@ +# 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_FindMaterialReferences.h b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_FindMaterialReferences.h index 52c484bd..f8e23f5f 100644 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_FindMaterialReferences.h +++ b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_FindMaterialReferences.h @@ -5,34 +5,8 @@ #include "MCPAssetFinder.h" #include "MCPUtils.h" #include "Materials/Material.h" -#include "MaterialDomain.h" #include "Materials/MaterialInstanceConstant.h" -#include "Materials/MaterialFunction.h" -#include "Materials/MaterialExpression.h" -#include "Materials/MaterialExpressionScalarParameter.h" -#include "Materials/MaterialExpressionVectorParameter.h" -#include "Materials/MaterialExpressionTextureObjectParameter.h" -#include "Materials/MaterialExpressionTextureSampleParameter2D.h" -#include "Materials/MaterialExpressionStaticSwitchParameter.h" -#include "Materials/MaterialExpressionConstant.h" -#include "Materials/MaterialExpressionConstant3Vector.h" -#include "Materials/MaterialExpressionConstant4Vector.h" -#include "Materials/MaterialExpressionTextureSample.h" -#include "Materials/MaterialExpressionTextureCoordinate.h" -#include "Materials/MaterialExpressionComponentMask.h" -#include "Materials/MaterialExpressionCustom.h" -#include "Materials/MaterialExpressionFunctionInput.h" -#include "Materials/MaterialExpressionFunctionOutput.h" -#include "Materials/MaterialExpressionMaterialFunctionCall.h" -#include "MaterialGraph/MaterialGraph.h" -#include "MaterialGraph/MaterialGraphNode.h" -#include "MaterialGraph/MaterialGraphNode_Root.h" -#include "MaterialGraph/MaterialGraphSchema.h" -#include "Kismet2/BlueprintEditorUtils.h" -#include "AssetRegistry/AssetRegistryModule.h" #include "AssetRegistry/IAssetRegistry.h" -#include "EdGraph/EdGraph.h" -#include "EdGraph/EdGraphNode.h" #include "UMCPHandler_FindMaterialReferences.generated.h" @@ -54,32 +28,44 @@ public: return TEXT("Find all assets that reference a given material or material instance."); } - virtual void Handle(const FJsonObject* Json, FJsonObject* Result) override + virtual void Handle(const FJsonObject* Json, FStringBuilderBase& Result) override { - // Try to find the material's package path (search both Material and MaterialInstance) + // Find the material asset (searching both Material and MaterialInstance) MCPAssets Assets; Assets.Scan().Scan(); if (!Assets.Exact(Material).Errors(Result).ENone().ETwo().Info()) return; FString PackagePath = Assets.OneData().PackageName.ToString(); - UE_LOG(LogTemp, Display, TEXT("BlueprintMCP: FindMaterialReferences — '%s' (package: %s)"), *Material, *PackagePath); - IAssetRegistry& Registry = *IAssetRegistry::Get(); TArray Referencers; Registry.GetReferencers(FName(*PackagePath), Referencers); - TArray> RefArray; + // Filter out self-references and classify each referencer + int32 Count = 0; for (const FName& Ref : Referencers) { FString RefStr = Ref.ToString(); - // Skip self-reference if (RefStr == PackagePath) continue; - RefArray.Add(MakeShared(RefStr)); + + TArray RefAssets; + Registry.GetAssetsByPackageName(Ref, RefAssets); + if (RefAssets.Num() > 0) + { + Result.Appendf(TEXT("%s %s\n"), + *MCPUtils::FormatName(RefAssets[0].GetClass()), + *RefStr); + } + else + { + Result.Appendf(TEXT("Unknown %s\n"), *RefStr); + } + Count++; } - Result->SetStringField(TEXT("packagePath"), PackagePath); - Result->SetNumberField(TEXT("totalReferencers"), RefArray.Num()); - Result->SetArrayField(TEXT("referencers"), RefArray); + if (Count == 0) + { + Result.Append(TEXT("No referencers found.\n")); + } } }; diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ListBlueprintComponents.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ListBlueprintComponents.Notes.md new file mode 100644 index 00000000..ce3a013a --- /dev/null +++ b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ListBlueprintComponents.Notes.md @@ -0,0 +1,27 @@ +# 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_ListBlueprintComponents.h b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ListBlueprintComponents.h index 94f0b4e1..88651584 100644 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ListBlueprintComponents.h +++ b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ListBlueprintComponents.h @@ -2,14 +2,11 @@ #include "CoreMinimal.h" #include "MCPHandler.h" -#include "MCPAssetFinder.h" +#include "MCPFetcher.h" #include "MCPUtils.h" #include "Engine/Blueprint.h" #include "Engine/SimpleConstructionScript.h" #include "Engine/SCS_Node.h" -#include "Components/ActorComponent.h" -#include "Kismet2/BlueprintEditorUtils.h" -#include "UObject/UObjectIterator.h" #include "UMCPHandler_ListBlueprintComponents.generated.h" @@ -23,83 +20,76 @@ class UMCPHandler_ListBlueprintComponents : public UObject, public IMCPHandler GENERATED_BODY() public: - UPROPERTY(meta=(Description="Blueprint name or package path")) - FString Blueprint; + UPROPERTY(meta=(Description="Path to a blueprint, e.g. /Game/Tangibles/TAN_Tree")) + FString Path; virtual FString GetDescription() const override { return TEXT("List all components in a Blueprint's SimpleConstructionScript, " - "including hierarchy and scene root information."); + "showing hierarchy and component classes."); } - virtual void Handle(const FJsonObject* Json, FJsonObject* Result) override + virtual void Handle(const FJsonObject* Json, FStringBuilderBase& Result) override { - MCPAssets Assets; - if (!Assets.Exact(Blueprint).Errors(Result).ENone().ETwo().Load()) return; - UBlueprint* BP = Assets.Object(); + MCPFetcher F(Result); + F.Walk(Path); + if (!F.Ok()) return; + + UBlueprint* BP = F.Cast(); + if (!BP) return; USimpleConstructionScript* SCS = BP->SimpleConstructionScript; if (!SCS) { - return MCPUtils::MakeErrorJson(Result, FString::Printf( - TEXT("Blueprint '%s' does not have a SimpleConstructionScript (not an Actor Blueprint)"), - *Blueprint)); + Result.Append(TEXT("ERROR: Not an Actor Blueprint (no SimpleConstructionScript)\n")); + return; } + const TArray& RootNodes = SCS->GetRootNodes(); const TArray& AllNodes = SCS->GetAllNodes(); - TArray> ComponentsArr; - for (USCS_Node* Node : AllNodes) + if (AllNodes.Num() == 0) { - if (!Node) - { - continue; - } - - TSharedRef CompObj = MakeShared(); - CompObj->SetStringField(TEXT("name"), Node->GetVariableName().ToString()); - - if (Node->ComponentClass) - { - CompObj->SetStringField(TEXT("componentClass"), MCPUtils::FormatName(Node->ComponentClass)); - } - else - { - CompObj->SetStringField(TEXT("componentClass"), TEXT("None")); - } - - // Parent component info - USCS_Node* ParentNode = nullptr; - for (USCS_Node* Candidate : AllNodes) - { - if (Candidate && Candidate->GetChildNodes().Contains(Node)) - { - ParentNode = Candidate; - break; - } - } - - if (ParentNode) - { - CompObj->SetStringField(TEXT("parentComponent"), ParentNode->GetVariableName().ToString()); - } - - // Check if this is a default scene root (first root node with SceneComponent class) - bool bIsSceneRoot = false; - const TArray& RootNodes = SCS->GetRootNodes(); - if (RootNodes.Num() > 0 && RootNodes[0] == Node) - { - bIsSceneRoot = true; - } - CompObj->SetBoolField(TEXT("isSceneRoot"), bIsSceneRoot); - - // List child count for informational purposes - CompObj->SetNumberField(TEXT("childCount"), Node->GetChildNodes().Num()); - - ComponentsArr.Add(MakeShared(CompObj)); + Result.Append(TEXT("No components.\n")); + return; } - Result->SetNumberField(TEXT("count"), ComponentsArr.Num()); - Result->SetArrayField(TEXT("components"), ComponentsArr); + Result.Append(TEXT("WARNING: This only lists components added in this blueprint's SCS. " + "It does not include inherited components from C++ parent classes " + "(available via the CDO's OwnedComponents) or from parent blueprint SCS nodes.\n")); + + // Emit components as a tree, starting from root nodes + for (USCS_Node* Root : RootNodes) + { + if (!Root) continue; + EmitNode(Root, 0, Root == RootNodes[0], Result); + } + } + +private: + void EmitNode(USCS_Node* Node, int32 Depth, bool bIsSceneRoot, FStringBuilderBase& Result) + { + // Indent to show hierarchy + for (int32 i = 0; i < Depth; i++) + Result.Append(TEXT(" ")); + + FString ClassName = Node->ComponentClass + ? MCPUtils::FormatName(Node->ComponentClass) + : TEXT("None"); + + Result.Appendf(TEXT("%s %s"), + *ClassName, + *MCPUtils::FormatName(Node->ComponentTemplate)); + + if (bIsSceneRoot && Depth == 0) + Result.Append(TEXT(" [SceneRoot]")); + + Result.Append(TEXT("\n")); + + for (USCS_Node* Child : Node->GetChildNodes()) + { + if (!Child) continue; + EmitNode(Child, Depth + 1, false, Result); + } } }; diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ListBlueprintInterfaces.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ListBlueprintInterfaces.Notes.md new file mode 100644 index 00000000..1770786c --- /dev/null +++ b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ListBlueprintInterfaces.Notes.md @@ -0,0 +1,25 @@ +# 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_ListBlueprintInterfaces.h b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ListBlueprintInterfaces.h index 507c5932..e95a2b5d 100644 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ListBlueprintInterfaces.h +++ b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ListBlueprintInterfaces.h @@ -2,13 +2,9 @@ #include "CoreMinimal.h" #include "MCPHandler.h" -#include "MCPAssetFinder.h" -#include "MCPServer.h" +#include "MCPFetcher.h" #include "MCPUtils.h" #include "Engine/Blueprint.h" -#include "EdGraph/EdGraph.h" -#include "Kismet2/BlueprintEditorUtils.h" -#include "UObject/UObjectIterator.h" #include "UMCPHandler_ListBlueprintInterfaces.generated.h" @@ -22,8 +18,8 @@ class UMCPHandler_ListBlueprintInterfaces : public UObject, public IMCPHandler GENERATED_BODY() public: - UPROPERTY(meta=(Description="Blueprint name or package path")) - FString Blueprint; + UPROPERTY(meta=(Description="Path to a blueprint, e.g. /Game/Foo/MyBlueprint")) + FString Path; virtual FString GetDescription() const override { @@ -31,39 +27,31 @@ public: "including their function graphs."); } - virtual void Handle(const FJsonObject* Json, FJsonObject* Result) override + virtual void Handle(const FJsonObject* Json, FStringBuilderBase& Result) override { + MCPFetcher F(Result); + F.Walk(Path); + if (!F.Ok()) return; + UBlueprint* BP = F.Cast(); + if (!BP) return; - MCPAssets Assets; - if (!Assets.Exact(Blueprint).Errors(Result).ENone().ETwo().Load()) return; - UBlueprint* BP = Assets.Object(); - - TArray> InterfacesArr; + bool bAny = false; for (const FBPInterfaceDescription& IfaceDesc : BP->ImplementedInterfaces) { - if (!IfaceDesc.Interface) - { - continue; - } + if (!IfaceDesc.Interface) continue; + bAny = true; - TSharedRef IfaceObj = MakeShared(); - IfaceObj->SetStringField(TEXT("name"), MCPUtils::FormatName(IfaceDesc.Interface)); - IfaceObj->SetStringField(TEXT("classPath"), IfaceDesc.Interface->GetPathName()); - - TArray> FuncArr; + Result.Appendf(TEXT("Interface: %s\n"), *MCPUtils::FormatName(IfaceDesc.Interface)); for (const UEdGraph* Graph : IfaceDesc.Graphs) { - if (Graph) - { - FuncArr.Add(MakeShared(MCPUtils::FormatName(Graph))); - } + if (!Graph) continue; + Result.Appendf(TEXT(" %s\n"), *MCPUtils::FormatName(Graph)); } - IfaceObj->SetArrayField(TEXT("functions"), FuncArr); - - InterfacesArr.Add(MakeShared(IfaceObj)); } - Result->SetNumberField(TEXT("count"), InterfacesArr.Num()); - Result->SetArrayField(TEXT("interfaces"), InterfacesArr); + if (!bAny) + { + Result.Append(TEXT("No interfaces implemented.\n")); + } } }; diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ListClassFunctions.h b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ListClassFunctions.h deleted file mode 100644 index a2987c09..00000000 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_ListClassFunctions.h +++ /dev/null @@ -1,131 +0,0 @@ -#pragma once - -#include "CoreMinimal.h" -#include "MCPHandler.h" -#include "MCPAssetFinder.h" -#include "MCPUtils.h" -#include "Engine/Blueprint.h" -#include "EdGraph/EdGraph.h" -#include "EdGraph/EdGraphNode.h" -#include "EdGraph/EdGraphPin.h" -#include "EdGraphSchema_K2.h" -#include "UObject/UObjectIterator.h" -#include "UMCPHandler_ListClassFunctions.generated.h" - - -// --------------------------------------------------------------------------- -// --------------------------------------------------------------------------- -// --------------------------------------------------------------------------- - -// ============================================================ -// HandleListFunctions — list Blueprint-callable functions on a class -// ============================================================ - -UCLASS() -class UMCPHandler_ListClassFunctions : public UObject, public IMCPHandler -{ - GENERATED_BODY() - -public: - UPROPERTY(meta=(Description="Class name to list functions for")) - FString ClassName; - - UPROPERTY(meta=(Optional, Description="Substring filter for function names")) - FString Filter; - - virtual FString GetDescription() const override - { - return TEXT("List Blueprint-callable functions on a UClass, including parameter info and flags."); - } - - virtual void Handle(const FJsonObject* Json, FJsonObject* Result) override - { - // Find the class - UClass* FoundClass = nullptr; - for (TObjectIterator It; It; ++It) - { - if (It->GetName() == ClassName || It->GetName() == ClassName + TEXT("_C")) - { - FoundClass = *It; - break; - } - } - if (!FoundClass) - { - return MCPUtils::MakeErrorJson(Result, FString::Printf(TEXT("Class '%s' not found"), *ClassName)); - } - - TArray> FuncList; - - for (TFieldIterator FuncIt(FoundClass); FuncIt; ++FuncIt) - { - UFunction* Func = *FuncIt; - if (!Func) continue; - - // Only include Blueprint-callable functions - if (!Func->HasAnyFunctionFlags(FUNC_BlueprintCallable | FUNC_BlueprintPure | FUNC_BlueprintEvent)) continue; - - FString FuncName = Func->GetName(); - - // Apply filter - if (!Filter.IsEmpty() && !FuncName.Contains(Filter, ESearchCase::IgnoreCase)) - { - continue; - } - - TSharedRef FuncObj = MakeShared(); - FuncObj->SetStringField(TEXT("name"), FuncName); - - // Determine the owning class - UClass* OwnerClass = Func->GetOwnerClass(); - if (OwnerClass) - { - FuncObj->SetStringField(TEXT("definedIn"), MCPUtils::FormatName(OwnerClass)); - } - - // Function flags - FuncObj->SetBoolField(TEXT("isPure"), Func->HasAnyFunctionFlags(FUNC_BlueprintPure)); - FuncObj->SetBoolField(TEXT("isStatic"), Func->HasAnyFunctionFlags(FUNC_Static)); - FuncObj->SetBoolField(TEXT("isEvent"), Func->HasAnyFunctionFlags(FUNC_BlueprintEvent)); - FuncObj->SetBoolField(TEXT("isConst"), Func->HasAnyFunctionFlags(FUNC_Const)); - - // Parameters - TArray> Params; - FString ReturnType; - for (TFieldIterator PropIt(Func); PropIt; ++PropIt) - { - FProperty* Prop = *PropIt; - if (!Prop) continue; - - FString PropType = Prop->GetCPPType(); - - if (Prop->HasAnyPropertyFlags(CPF_ReturnParm)) - { - ReturnType = PropType; - continue; - } - - if (Prop->HasAnyPropertyFlags(CPF_Parm)) - { - TSharedRef ParamObj = MakeShared(); - ParamObj->SetStringField(TEXT("name"), Prop->GetName()); - ParamObj->SetStringField(TEXT("type"), PropType); - ParamObj->SetBoolField(TEXT("isOutput"), Prop->HasAnyPropertyFlags(CPF_OutParm) && !Prop->HasAnyPropertyFlags(CPF_ReferenceParm)); - ParamObj->SetBoolField(TEXT("isReference"), Prop->HasAnyPropertyFlags(CPF_ReferenceParm)); - Params.Add(MakeShared(ParamObj)); - } - } - FuncObj->SetArrayField(TEXT("parameters"), Params); - if (!ReturnType.IsEmpty()) - { - FuncObj->SetStringField(TEXT("returnType"), ReturnType); - } - - FuncList.Add(MakeShared(FuncObj)); - } - - Result->SetStringField(TEXT("className"), MCPUtils::FormatName(FoundClass)); - Result->SetNumberField(TEXT("count"), FuncList.Num()); - Result->SetArrayField(TEXT("functions"), FuncList); - } -}; diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RemoveBlueprintVariable.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RemoveBlueprintVariable.Notes.md new file mode 100644 index 00000000..2a3f1b3f --- /dev/null +++ b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RemoveBlueprintVariable.Notes.md @@ -0,0 +1,29 @@ +# 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_RemoveBlueprintVariable.h b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RemoveBlueprintVariable.h index d08597ec..b8f52913 100644 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RemoveBlueprintVariable.h +++ b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RemoveBlueprintVariable.h @@ -2,13 +2,9 @@ #include "CoreMinimal.h" #include "MCPHandler.h" -#include "MCPAssetFinder.h" +#include "MCPFetcher.h" #include "MCPUtils.h" #include "Engine/Blueprint.h" -#include "EdGraph/EdGraph.h" -#include "EdGraph/EdGraphPin.h" -#include "K2Node_VariableGet.h" -#include "K2Node_VariableSet.h" #include "Kismet2/BlueprintEditorUtils.h" #include "UMCPHandler_RemoveBlueprintVariable.generated.h" @@ -34,52 +30,44 @@ public: return TEXT("Remove a member variable from a Blueprint."); } - virtual void Handle(const FJsonObject* Json, FJsonObject* Result) override + virtual void Handle(const FJsonObject* Json, FStringBuilderBase& Result) override { - MCPAssets Assets; - if (!Assets.Exact(Blueprint).Errors(Result).ENone().ETwo().Load()) return; - UBlueprint* BP = Assets.Object(); + MCPFetcher F(Result); + UBlueprint* BP = F.Walk(Blueprint).Cast(); + if (!BP) return; - // Find variable by name (case-insensitive) - FName VarFName(*VariableName); - bool bVarFound = false; + // Find the variable using Identifies for consistent name matching + const FBPVariableDescription* Found = nullptr; for (const FBPVariableDescription& Var : BP->NewVariables) { - if (Var.VarName.ToString().Equals(VariableName, ESearchCase::IgnoreCase)) + if (MCPUtils::FormatName(Var) == VariableName || + Var.VarName.ToString().Equals(VariableName, ESearchCase::IgnoreCase)) { - VarFName = Var.VarName; // Use the exact name found - bVarFound = true; + Found = &Var; break; } } - if (!bVarFound) + if (!Found) { - // Build available variables list for helpful error message - TArray> AvailVars; + Result.Appendf(TEXT("ERROR: Variable '%s' not found in %s.\nAvailable variables:\n"), + *VariableName, *MCPUtils::FormatName(BP)); for (const FBPVariableDescription& Var : BP->NewVariables) { - AvailVars.Add(MakeShared(Var.VarName.ToString())); + Result.Appendf(TEXT(" %s\n"), *MCPUtils::FormatName(Var)); } - - MCPUtils::MakeErrorJson(Result, FString::Printf( - TEXT("Variable '%s' not found in Blueprint '%s'"), *VariableName, *Blueprint)); - Result->SetArrayField(TEXT("availableVariables"), AvailVars); return; } - UE_LOG(LogTemp, Display, TEXT("BlueprintMCP: Removing variable '%s' from Blueprint '%s'"), - *VariableName, *Blueprint); + FName VarFName = Found->VarName; - // Use the editor utility to remove the variable (also cleans up Get/Set nodes) + // RemoveMemberVariable also cleans up Get/Set nodes FBlueprintEditorUtils::RemoveMemberVariable(BP, VarFName); - FBlueprintEditorUtils::MarkBlueprintAsStructurallyModified(BP); + bool bSaved = MCPUtils::SaveBlueprintPackage(BP); - - UE_LOG(LogTemp, Display, TEXT("BlueprintMCP: Removed variable '%s' from '%s' (saved: %s)"), - *VariableName, *Blueprint, bSaved ? TEXT("true") : TEXT("false")); - - Result->SetBoolField(TEXT("saved"), bSaved); + Result.Appendf(TEXT("Removed variable %s from %s.%s\n"), + *VarFName.ToString(), *MCPUtils::FormatName(BP), + bSaved ? TEXT("") : TEXT(" WARNING: save failed.")); } }; diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RestoreAsset.Notes.md b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RestoreAsset.Notes.md new file mode 100644 index 00000000..4a135020 --- /dev/null +++ b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RestoreAsset.Notes.md @@ -0,0 +1,27 @@ +# 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_RestoreAsset.h b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RestoreAsset.h index 5a01bee0..cba3ebdc 100644 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RestoreAsset.h +++ b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_RestoreAsset.h @@ -2,16 +2,8 @@ #include "CoreMinimal.h" #include "MCPHandler.h" -#include "MCPAssetFinder.h" #include "MCPUtils.h" -#include "Engine/Blueprint.h" -#include "Kismet2/BlueprintEditorUtils.h" -#include "UObject/SavePackage.h" #include "Misc/PackageName.h" -#include "AssetRegistry/AssetRegistryModule.h" -#include "AssetRegistry/IAssetRegistry.h" -#include "AssetToolsModule.h" -#include "IAssetTools.h" #include "FileHelpers.h" #include "HAL/FileManager.h" #include "UMCPHandler_RestoreAsset.generated.h" @@ -35,7 +27,7 @@ public: return TEXT("Restore a .uasset file from its .uasset.bak backup, reloading it in the editor."); } - virtual void Handle(const FJsonObject* Json, FJsonObject* Result) override + virtual void Handle(const FJsonObject* Json, FStringBuilderBase& Result) override { FString Filename = FPaths::ConvertRelativePathToFull( FPackageName::LongPackageNameToFilename(AssetPath, FPackageName::GetAssetPackageExtension())); @@ -43,7 +35,8 @@ public: if (!IFileManager::Get().FileExists(*BackupFilename)) { - return MCPUtils::MakeErrorJson(Result, FString::Printf(TEXT("Backup file not found: %s"), *BackupFilename)); + Result.Appendf(TEXT("ERROR: Backup file not found: %s\n"), *BackupFilename); + return; } // Release file handles if the package is loaded @@ -57,7 +50,8 @@ public: uint32 CopyResult = IFileManager::Get().Copy(*Filename, *BackupFilename, true); if (CopyResult != COPY_OK) { - return MCPUtils::MakeErrorJson(Result, FString::Printf(TEXT("Failed to restore %s"), *Filename)); + Result.Appendf(TEXT("ERROR: Failed to copy backup over %s\n"), *AssetPath); + return; } // Reload the package if it was loaded @@ -66,8 +60,14 @@ public: bool bReloaded = false; FText ErrorMessage; UEditorLoadingAndSavingUtils::ReloadPackages({Package}, bReloaded, ErrorMessage, EReloadPackagesInteractionMode::AssumePositive); + if (!bReloaded) + { + Result.Appendf(TEXT("WARNING: Restored %s but reload failed: %s\n"), + *AssetPath, *ErrorMessage.ToString()); + return; + } } - Result->SetStringField(TEXT("restoredFrom"), BackupFilename); + Result.Appendf(TEXT("Restored %s from backup\n"), *AssetPath); } }; diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SearchAssets.h b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SearchAssets.h index ac51f5f7..f1044f73 100644 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SearchAssets.h +++ b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_SearchAssets.h @@ -68,7 +68,11 @@ public: *Data.PackageName.ToString()); } - if (AllData.Num() >= Limit) + if (AllData.Num() == 0) + { + Result.Append(TEXT("No assets found.\n")); + } + else if (AllData.Num() >= Limit) { Result.Appendf(TEXT("WARNING: You reached the limit of %d, to raise it, specify the Limit parameter.\n"), Limit); } diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/MCPHandlers.cpp b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/MCPHandlers.cpp index 58606fee..85a8dd66 100644 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/MCPHandlers.cpp +++ b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/MCPHandlers.cpp @@ -49,7 +49,6 @@ #include "Handlers/UMCPHandler_ListBlueprintAssets.h" #include "Handlers/UMCPHandler_ListBlueprintComponents.h" #include "Handlers/UMCPHandler_ListBlueprintInterfaces.h" -#include "Handlers/UMCPHandler_ListClassFunctions.h" #include "Handlers/UMCPHandler_ListClassProperties.h" #include "Handlers/UMCPHandler_ListEventDispatchers.h" #include "Handlers/UMCPHandler_ListMaterialAssets.h" diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/MCPServer.cpp b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/MCPServer.cpp index 6158427d..286c528a 100644 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/MCPServer.cpp +++ b/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/MCPServer.cpp @@ -292,25 +292,24 @@ FString UMCPServer::HandleRequest(const FString& Line) return PopulateError.ToString(); } - // Call the text handler. This may be a no-op, in which case - // the string builder will remain empty. That indicates that - // the json handler should be used instead. - TStringBuilder<32768> TextResult; - Handler->Handle(&*Request, TextResult); - if (TextResult.Len() > 0) - { - FString Result = TextResult.ToString(); - for (int32 i = 0; i < Result.Len(); ++i) - { - if (Result[i] == TEXT('\0')) Result[i] = TEXT(' '); - } - return Result; - } - - // Invoke the Json handler. + // Call the JSON handler first. If it leaves the object empty, + // fall back to the plain-text handler. TSharedRef JsonResult = MakeShared(); Handler->Handle(&*Request, &*JsonResult); - return MCPUtils::JsonToString(JsonResult); + if (JsonResult->Values.Num() > 0) + { + return MCPUtils::JsonToString(JsonResult); + } + + // Invoke the plain-text handler. + TStringBuilder<32768> TextResult; + Handler->Handle(&*Request, TextResult); + FString Result = TextResult.ToString(); + for (int32 i = 0; i < Result.Len(); ++i) + { + if (Result[i] == TEXT('\0')) Result[i] = TEXT(' '); + } + return Result; } // ============================================================ diff --git a/Plugins/BlueprintMCP/Source/BlueprintMCP/Public/MCPHandler.h b/Plugins/BlueprintMCP/Source/BlueprintMCP/Public/MCPHandler.h index 7b4b92b0..afff5f1e 100644 --- a/Plugins/BlueprintMCP/Source/BlueprintMCP/Public/MCPHandler.h +++ b/Plugins/BlueprintMCP/Source/BlueprintMCP/Public/MCPHandler.h @@ -52,8 +52,8 @@ public: // Called after parameter fields have been populated from JSON. // Override the FStringBuilderBase version for plain-text responses, or the - // FJsonObject version for JSON responses. The dispatcher calls the text - // version first; if the builder remains empty, it falls back to JSON. + // FJsonObject version for JSON responses. The dispatcher calls the JSON + // version first; if the result object is empty, it falls back to plain-text. virtual void Handle(const FJsonObject* Json, FStringBuilderBase& Result) {} virtual void Handle(const FJsonObject* Json, FJsonObject* Result) {} };