An honest, brutal architectural review of the Ned editor submodule (Hazel/vendor/Ned/), as embedded within LuckyEngine.
- What Ned Is
- System Map
- Architectural Critique
- 1. The God Object: NedContext
- 2. Global State Catastrophe
- 3. The Dummy Instance Anti-Pattern
- 4. The Callback Spaghetti
- 5. NedEmbed: The Options Struct From Hell
- 6. Editor Architecture: The Singleton Graveyard
- 7. AI Agent: Exposed Guts
- 8. MCP System: No Real Architecture
- 9. The Settings Crash: A Confession In Code
- 10. File System: Mixed Concerns
- 11. Build System: Double Maintenance
- 12. Type Safety: Everything Is A String
- 13. Thread Safety: Undocumented Gambling
- 14. Memory Management: Three Different Strategies, None Correct
- 15. The 10,000-Character Input Buffer
- 16. Comment & Documentation Quality
- Proposed Architecture
Ned is a text editor / AI coding assistant embedded inside LuckyEngine's editor. It provides:
- Text editing — syntax highlighting (TreeSitter + custom lexers), bracket matching, multi-cursor, undo/redo, git integration
- AI agent — LLM chat via OpenRouter API, tool calls, file editing with diff/approval UI
- MCP servers — file system, terminal, AngelScript, Tripo 3D model generation, image editing
- LSP client — language server protocol for go-to-definition, references, diagnostics, hover
- File management — project tree, fuzzy finder, file monitoring, content search
- Hazel integration — entity/component manipulation, asset queries, viewport capture via callback bridge
~100+ source files, ~100K lines of code, 7 third-party libraries bundled.
graph TD
NedEmbed["NedEmbed<br/>(entry point)"]
NedContext["NedContext<br/>(god object)"]
Globals["globals.h<br/>(37 #define macros)"]
Editor["Editor<br/>(coordinator)"]
EditorState["EditorState<br/>(global extern struct)"]
EditorRender["EditorRender<br/>(extern singleton)"]
EditorKeyboard["EditorKeyboard<br/>(extern singleton)"]
EditorMouse["EditorMouse<br/>(extern singleton)"]
EditorScroll["EditorScroll<br/>(extern singleton)"]
EditorSelection["EditorSelection<br/>(extern singleton)"]
EditorIndent["EditorIndentation<br/>(extern singleton)"]
EditorLineNum["EditorLineNumbers<br/>(extern singleton)"]
EditorBracket["EditorBracketMatching<br/>(extern singleton)"]
EditorHighlight["EditorHighlight"]
EditorGit["EditorGit"]
EditorDiff["EditorDiff"]
EditorCursor["EditorCursor"]
AIAgent["AIAgent"]
AIRender["AIAgentRender"]
AIHistory["AIAgentHistory"]
AITextInput["AIAgentTextInput"]
OpenRouter["OpenRouter<br/>(all static)"]
AgentRequest["AgentRequest"]
MCPManager["MCP::Manager"]
MCPFileSystem["MCP::FileSystemServer"]
MCPTerminal["MCP::TerminalServer"]
MCPEditFile["MCP::EditFileHandler"]
MCPTripo["MCP::TripoService"]
MCPAngel["MCP::AngelScriptServer"]
FileExplorer["FileExplorer"]
FileTree["FileTree<br/>(dummy)"]
FileFinder["FileFinder<br/>(dummy)"]
FileMonitor["FileMonitor"]
FileSearch["FileContentSearch"]
LSPClient["LSPClient<br/>(dummy)"]
Settings["Settings<br/>(static leak)"]
Splitter["Splitter<br/>(static state)"]
Font["Font"]
Welcome["Welcome"]
TextureMgr["ITextureManager"]
HazelInterface["HazelAgentInterface"]
MainThreadDisp["MainThreadDispatcher"]
NedEmbed --> NedContext
NedContext --> Globals
NedContext -->|unique_ptr| Editor
NedContext -->|unique_ptr| AIAgent
NedContext -->|unique_ptr| FileExplorer
NedContext -->|unique_ptr| EditorHighlight
NedContext -->|unique_ptr| EditorGit
NedContext -->|unique_ptr| EditorCursor
NedContext -->|unique_ptr| Font
NedContext -->|unique_ptr| Settings
NedContext -->|static dummy| FileTree
NedContext -->|static dummy| FileFinder
NedContext -->|static dummy| LSPClient
NedContext -->|raw static new| Settings
NedContext -.->|30 std::function<br/>callbacks| HazelInterface
Editor --> EditorState
EditorRender --> EditorState
EditorKeyboard --> EditorState
EditorMouse --> EditorState
EditorScroll --> EditorState
AIAgent --> AIRender
AIAgent --> AIHistory
AIAgent --> AITextInput
AIAgent --> AgentRequest
AIAgent --> EditorDiff
AgentRequest --> OpenRouter
AIAgent --> MCPManager
MCPManager --> MCPFileSystem
MCPManager --> MCPTerminal
MCPManager --> MCPAngel
MCPFileSystem --> MCPEditFile
HazelInterface --> MainThreadDisp
style NedContext fill:#ff6b6b,color:#fff
style EditorState fill:#ff6b6b,color:#fff
style Globals fill:#ff6b6b,color:#fff
style Settings fill:#ff9800,color:#fff
style Splitter fill:#ff9800,color:#fff
Legend: Red = critical architectural problem. Orange = significant concern.
File: ned_context.h — 326 lines
NedContext owns 23 components AND 30 public std::function callback members. It's a service locator masquerading as a context object.
What's wrong:
-
30 public
std::functionfields dumped directly on the class with no grouping, no interface, no contract:std::function<std::string(const std::string&)> findObjectCallback; std::function<std::string()> getEntityListCallback; std::function<std::string(const std::string&)> getEntityPropertiesCallback; std::function<std::string(const std::string&, const std::string&, const std::string&, const std::string&)> setEntityPropertyCallback; // ... 26 more
These are not grouped into an interface. They're just raw fields. If you forget to set one, you get a null
std::functioncall →std::bad_function_call→ crash. No compile-time enforcement, no runtime validation, no documentation of which are required vs optional. -
The createGameObjectCallback has 15 parameters:
std::function<std::string(const std::string&, const std::string&, float, float, float, float, float, float, float, float, float, float, float, bool, const std::string&)> createGameObjectCallback;
This is unreadable and unmaintainable. Nobody can remember what the 9th float means.
-
23 getters all with identical lazy-init boilerplate:
Editor& NedContext::getEditor() { if (!editor) { editor = std::make_unique<Editor>(); } return *editor; }
This pattern is repeated 23 times verbatim. No template, no macro, just copy-paste.
Impact: Every time a new engine feature needs AI agent integration, someone has to: (a) add a std::function to NedContext, (b) add a std::function with the same signature to NedEmbedOptions, (c) add code to copy it from options to context in NedEmbed constructor, (d) add a matching tool definition in MCP, (e) add a method in HazelAgentInterface. That's 5 files touched for every callback. Zero type safety throughout.
File: globals.h — 48 lines of pure damage
#define gBookmarks (GetNedContext()->getBookmarks())
#define gAIAgent (GetNedContext()->getAIAgent())
#define gFont (GetNedContext()->getFont())
#define gSettings (GetNedContext()->getSettings())
// ... 37 total macrosThe file comments say // TODO: Replace these with direct GetNedContext()->getX() calls throughout codebase. This TODO is the entire architectural critique in one line — the developers know this is wrong and never fixed it.
What's wrong:
- These are
#definemacros, not even inline functions. They pollute every translation unit that includesglobals.h. The namegEditorwill collide with anything else in any project that embeds Ned. - They hide a thread-local pointer dereference + null check + potential heap allocation behind something that looks like a simple variable access.
gSettings.getAgentModel()looks cheap — it's actuallyGetNedContext()->getSettings()which (see critique #9) does astatic Settings*allocation. - The thread-local
g_nedContextis set by NedEmbed during init, but there's no validation that it's set before use. Any code that runs on a different thread before init will dereference nullptr.
File: ned_context.cpp — lines 128-246
When a component is excluded from the build, NedContext returns a static dummy:
Terminal& NedContext::getTerminal() {
static Terminal dummyTerminal;
return dummyTerminal; // Return dummy - Terminal excluded from build
}
FileTree& NedContext::getFileTree() {
static FileTree dummyFileTree;
return dummyFileTree;
}6 components use this pattern: Terminal, FileTree, FileFinder, LSPClient, LSPDashboard, LSPGotoDef, LSPGotoRef, LSPSymbolInfo, LSPUriOptions.
What's wrong:
- The caller doesn't know it's getting a dummy. Code calling
gTerminal.executeCommand("ls")will silently do nothing (or worse, crash on an uninitialized member). There's no error, no exception, no log — just silent failure. - Static local variables in a function returning a reference means the lifetime extends to program end. If any of these dummies touch global state in their constructor or destructor, you get static initialization/destruction order fiascos.
- The real fix is trivially obvious: return
std::optional, a pointer, or use compile-time#ifdefto remove the API entirely. Instead, they chose the worst option — pretend the component exists. - Thread safety:
staticlocal variables in C++11+ are guaranteed thread-safe construction, but the dummy objects themselves may not be thread-safe. Multiple threads callinggetTerminal()get the same dummy instance with no synchronization.
Files: ned_context.h (30 callbacks), ned_embed.h (30 identical callbacks in NedEmbedOptions), HazelAgentInterface.h (30 methods that wrap the callbacks)
The integration between Ned and LuckyEngine is done through 30 std::function callbacks, defined in three places:
- NedEmbedOptions — the user passes callbacks when constructing NedEmbed
- NedContext — NedEmbed copies each callback into the context
- HazelAgentInterface — provides the actual implementations that get wrapped into lambdas
The same callback signature is written three times. The same documentation comment is copied three times. There is no interface, no contract, no type checking beyond "does the std::function match."
What it should be: A single abstract interface (like ITextureManager, which Ned already does correctly for textures) that LuckyEngine implements:
class INedHostInterface {
public:
virtual std::string GetEntityList() = 0;
virtual std::string CreateEntity(const CreateEntityParams&) = 0;
// ...
};One definition. One implementation. Compile-time type checking. No possibility of forgetting a callback.
File: ned_embed.h — 257 lines, just for NedEmbedOptions
NedEmbedOptions has 70+ fields: 30 std::function callbacks, 12 multi-window layout floats, 4 viewport floats, booleans, strings, a void* pointer.
struct NedEmbedOptions {
ITextureManager *textureManager = nullptr;
bool showWelcome = true;
bool enableLogging = false;
bool multiWindow = false;
float multiWindowBorderRadius = 0.0f;
bool multiWindowShowTitleBar = true;
bool multiWindowResizable = true;
bool multiWindowMovable = false;
// ... 12 more float fields for window positions ...
// ... 30 callback functions ...
float cameraViewportX = 0.0f;
float cameraViewportY = 0.0f;
float cameraViewportWidth = 0.0f;
float cameraViewportHeight = 0.0f;
};What's wrong:
- This struct mixes construction-time configuration (textureManager, multiWindow), runtime state (cameraViewportX/Y/Width/Height), and behavioral contracts (30 callbacks). These are three separate concerns in one bag.
- The layout floats (
fileExplorerX,editorX,agentX, etc.) are duplicated in NedEmbed's private members (multiWindowFileExplorerX,multiWindowEditorX, etc.) — the struct sets the initial values, then NedEmbed copies them and tracks them separately. Double state, double bugs. - The comment says
cameraViewportX/Y/Width/Heightare "read-only, updated by NED internally." But they're in the Options struct — a config struct that mutates after construction.
Files: editor/editor_*.h — 10+ extern singletons
The editor is decomposed into ~12 sub-components. That part is fine. The problem is how they communicate — through a shared global EditorState and direct access to each other via extern singletons:
// editor_types.h
extern EditorState editor_state; // THE global mutable state
// editor_render.h
extern EditorRender gEditorRender;
// editor_keyboard.h
extern EditorKeyboard gEditorKeyboard;
// editor_scroll.h
extern EditorScroll gEditorScroll;
// editor_selection.h
extern EditorSelection gEditorSelection;
// editor_indentation.h
extern EditorIndentation gEditorIndentation;
// editor_line_numbers.h
extern EditorLineNumbers gEditorLineNumbers;
// editor_mouse.h
extern EditorMouse gEditorMouse;What's wrong:
-
EditorStateis the real god object. It's a 135-line struct with 40+ mutable fields — file content, syntax colors, cursor position, scroll state, selection, line widths, blink timers, margin calculations. Every editor sub-component reads and writes this struct directly. There are no access controls, no invalidation, no transactions. -
The singletons are not in NedContext. NedContext has
std::unique_ptr<Editor>, but EditorRender, EditorKeyboard, EditorMouse, EditorScroll, EditorSelection, EditorIndentation, EditorLineNumbers, and EditorBracketMatching are raw extern globals — not owned by anything. They're constructed at static init time and destroyed at static destruction time. This contradicts NedContext's entire purpose ("no code runs at module load time"). -
Multi-instance is broken. NedContext + thread-local was designed to support multiple Ned instances. But the
extern EditorState editor_stateis a process-wide global. Two NedEmbed instances share the same cursor position, selection, and file content. The architecture says "multi-instance ready" but the implementation says "absolutely not." -
EditorStatehas a mutex inside it (colorsMutex) but nothing else is synchronized. The cursor, selection, file content, and scroll state are all raceable.
File: ai_agent.h — 92 lines
class AIAgent {
public:
// Make messages public so it can be accessed from agent_request.cpp and rendering
std::vector<Message> messages;
std::mutex messagesMutex;
bool needsFollowUpMessage = false;
std::chrono::steady_clock::time_point followUpRequestTime;
std::atomic<bool> messageDisplayLinesDirty{true};
EditorDiff editorDiff;
AIAgentTextInput textInput;
char inputBuffer[10000] = {0};
AgentRequest agentRequest;
bool hasApiKeyError = false;
AIAgentRender* renderer;
// ...
};The comment // Make messages public so it can be accessed from agent_request.cpp and rendering is a confession. Instead of designing an API, they made everything public.
What's wrong:
char inputBuffer[10000] = {0}— a fixed-size C-style buffer for text input. In 2024+ C++20. This is a hard limit on input size with no runtime check. And it's public, so anyone can write anywhere in it.messagesis a publicstd::vector<Message>with amessagesMutexnext to it. The mutex exists, but there's no RAII wrapper — every caller must manuallylock_guardbefore touchingmessages. Miss one? Data race.rendereris a raw pointer (AIAgentRender*). Who owns it? Who deletes it? The class hasfriend class AIAgentRender— exposing yet more private state.EditorDiff editorDiffis directly embedded, not behind a pointer or interface. AIAgent is tightly coupled to diff rendering.
File: mcp/mcp_manager.h
class Manager {
public:
void registerTool(const ToolDefinition &toolDef);
std::string executeToolCall(const std::string &toolName,
const std::unordered_map<std::string, std::string> ¶meters);
private:
std::vector<ToolDefinition> toolDefinitions;
std::unique_ptr<AngelScriptServer> angelScriptServer;
};What's wrong:
- All parameters are strings.
std::unordered_map<std::string, std::string>means every tool call is an untyped string→string map. The type system is bypassed entirely. A tool expecting anintreceives astringand must parse it. The tool receiving afloatreceives astringand must parse it. Parsing errors become runtime crashes. - Return type is always
std::string. Success and failure look the same — you parse a string to find out. - Tool dispatch is by string name.
executeToolCall("edit_file", ...)is runtime string matching. Add a typo, get silent failure. - No error type. The execute function returns a
std::string. If the tool doesn't exist, what happens? If the parameters are wrong? You get a string back. Maybe it says "error." Maybe it doesn't. AngelScriptServeris special-cased with a dedicatedunique_ptron the Manager, while all other servers are... constructed somewhere else? The ownership model is inconsistent.
File: ned_context.cpp — lines 93-103
Settings& NedContext::getSettings()
{
// TEMPORARY: Use static dummy to debug crash
// The crash happens even at `if (!settings)` which suggests
// either `this` is invalid or there's an ABI/compilation issue
static Settings* staticSettings = nullptr;
if (!staticSettings) {
staticSettings = new Settings();
}
return *staticSettings;
}This is a raw new with no delete, ever. It leaks a Settings object for the lifetime of the process. The comment says the crash happens "even at if (!settings)" which means this is invalid — the NedContext pointer itself is corrupt.
This is the kind of bug you get when:
- Static initialization order is wrong (a global accesses NedContext before it's constructed)
- The thread-local
g_nedContextpointer is stale or uninitialized - ABI mismatch between compilation units (different compilers, different flags, different C++ standard)
Instead of finding and fixing the root cause, they worked around it with a permanent memory leak. The "TEMPORARY" comment suggests this is known and accepted.
File: files/files.h — 192 lines
FileExplorer is simultaneously:
- A file I/O manager (load, save, read)
- A UI component (renderFileExplorer, renderFileContent)
- An icon manager (loadIcons, getIconForFile, getIcon)
- An undo/redo manager (per-file undo managers)
- A file dialog controller (handleFileDialog, openFolderDialog)
- An external change detector (checkForExternalFileChanges)
It has a 40-line inline getIconForFile() function in the header that hardcodes special filenames:
if (fileName == "CMakeLists.txt" || fileName == "cmake") { ... }
if (fileName == ".clangd" || fileName == ".clang-format") { ... }
if (fileName == "Dockerfile") { ... }
else if (fileName == ".gitignore") { ... }This is data pretending to be code. It should be a lookup table or config file.
MAX_FILE_SIZE is a const size_t computed with integer multiplication:
const size_t MAX_FILE_SIZE = 1 * 1024 * 1024 * 1; // 1mbThe * 1 on each side does nothing. It reads like someone copy-pasted a template and didn't clean up.
Files: premake5.lua (227 lines), CMakeLists.txt (400+ lines)
Both build systems define the same set of files, includes, defines, and exclusions. Change one, forget the other. The premake5.lua has 17 warnings disabled on MSVC — these aren't suppressions of harmless vendor warnings, they include signed/unsigned mismatch (4018, 4389), narrowing conversions (4244, 4267, 4838), and unreachable code (4702). These are legitimate warnings being silenced.
Throughout the entire Ned↔LuckyEngine integration, all data flows as strings:
- Entity IDs:
std::string(should be a typed UUID) - Component names:
std::string(should be an enum) - Property names:
std::string(should be typed) - Property values:
std::string(should be variant or typed union) - Return values:
std::string(should beResult<T, Error>) - Tool parameters:
std::unordered_map<std::string, std::string>(should be typed structs)
The setEntityPropertyCallback takes 4 strings and returns a string:
std::function<std::string(const std::string&, const std::string&,
const std::string&, const std::string&)> setEntityPropertyCallback;What are those four strings? You have to read the comment above to know (entityID, componentName, propertyName, valueString). If you pass them in the wrong order, you get a runtime error or silent corruption.
EditorStatehasstd::mutex colorsMutexfor syntax colors, but cursor position, selection, and file content have no synchronization.AIAgent::messageshasmessagesMutex, but callers must manually lock it. ThehasPendingEdits()method onEditorDifflocks its own mutex, butgetPendingEdits()returns a raw reference with a note "caller must hold the mutex!" — manual lock discipline that will inevitably be violated.OpenRouterhasstatic std::atomic<bool> curl_initializedandstatic std::mutex g_curlInitMutex— static state for global CURL initialization.MainThreadDispatcheris the only properly designed concurrent component — it uses a mutex-protected queue withstd::promisefor blocking cross-thread calls.- Zero documentation on thread-safety contracts anywhere. No
// Thread-safeor// Must be called from main threadannotations.
- NedContext members:
std::unique_ptr<T>— correct for owned components - Settings:
static Settings* = new Settings()— leaked, never deleted - Dummy components:
static Terminal dummyTerminal— static locals, never destroyed (well, at atexit, in unspecified order) - EditorDiff in AIAgent: Direct member variable (not a pointer) — rigid coupling
- AIAgentRender in AIAgent: Raw pointer
AIAgentRender* renderer— ownership unclear - FileExplorer:
UndoRedoManager *currentUndoManager = nullptr— raw pointer intostd::map<std::string, UndoRedoManager>— invalidated if the map rehashes - NedContext::scriptBuildOutput:
const char* scriptBuildOutput = nullptr; // Points to host-owned string, not owned by NedContext— raw pointer to someone else's memory. If the host frees it, dangling pointer.
File: ai_agent.h — line 56
char inputBuffer[10000] = {0};A fixed C-style buffer in a C++20 codebase. Public. No bounds checking. No dynamic resizing. If a user pastes 10,001 characters — undefined behavior or silent truncation. std::string exists. std::array exists. ImGui::InputTextMultiline with callbacks exists.
The codebase has good file-level comments but terrible architectural documentation:
// Make messages public so it can be accessed from agent_request.cpp and rendering— this comment admits the design is wrong instead of fixing it// TODO: Replace these with direct GetNedContext()->getX() calls throughout codebase— this TODO has been here for the entire life of these macros// TEMPORARY: Use static dummy to debug crash— "temporary" memory leak// Note: Unreal's ImGui uses ABGR format, so we swap R and B— hardcoded color constants for a specific platform, no#ifdef, applied globally// Excluded from buildcomments on components that still have getters, still compile, and still get called
classDiagram
class INedHost {
<<interface>>
+GetEntityList() EntityList
+GetEntityProperties(EntityID) EntityProperties
+SetEntityProperty(EntityID, ComponentID, PropertyID, Value) Result
+CreateEntity(CreateEntityParams) Result~EntityID~
+SelectEntity(EntityID) Result
+CaptureViewport() Image
+ListAssets(AssetFilter) AssetList
+CreateMaterial(MaterialParams) Result~AssetHandle~
+CreateGameObject(GameObjectParams) Result~EntityID~
}
class NedEditor {
-config: NedConfig
-host: INedHost*
-textEditor: TextEditor
-aiAgent: AIAssistant
-fileManager: FileManager
-uiLayout: UILayout
+initialize(config, host)
+render()
+shutdown()
}
class NedConfig {
+textureManager: ITextureManager*
+resourcePath: string
+layout: LayoutConfig
}
class LayoutConfig {
+mode: LayoutMode
+showSidebar: bool
+showAgentPane: bool
+initialSplitRatio: float
}
class TextEditor {
-document: Document
-viewport: EditorViewport
-inputHandler: InputHandler
-highlighter: SyntaxHighlighter
-gitIntegration: GitIntegration
+render(width: float)
+loadFile(path: string) Result
+saveFile() Result
}
class Document {
-content: string
-lineStarts: vector~int~
-undoHistory: UndoStack
-cursor: CursorState
-selection: SelectionState
+insert(pos, text)
+erase(range)
+getText() string_view
+getLine(n) string_view
}
class CursorState {
-primary: CursorPosition
-secondary: vector~CursorPosition~
-preferredColumn: int
}
class EditorViewport {
-scrollPos: Vec2
-scrollAnimation: ScrollAnimation
-visibleRange: LineRange
+render(document, font)
+ensureCursorVisible()
+scrollTo(line)
}
class InputHandler {
-keybinds: KeybindMap
+handleKeyboard(document, viewport)
+handleMouse(document, viewport)
}
class SyntaxHighlighter {
-treeSitter: TreeSitterParser
-pendingHighlight: future~TokenList~
-cancelFlag: atomic~bool~
+highlightAsync(content, language) future~TokenList~
+cancel()
}
class AIAssistant {
-conversation: Conversation
-apiClient: LLMClient
-toolExecutor: ToolExecutor
-diffReviewer: DiffReviewer
+sendMessage(text)
+render(width)
+stopGeneration()
}
class Conversation {
-messages: vector~Message~
-mutex: mutex
+addMessage(Message)
+getMessages() span~const Message~
+clear()
}
class LLMClient {
-apiKey: string
-model: string
+streamCompletion(messages, onToken, onComplete)
+cancel()
}
class ToolExecutor {
-tools: map~string, ITool*~
+registerTool(ITool)
+execute(ToolCall) ToolResult
}
class ITool {
<<interface>>
+name() string
+description() string
+parameters() ParameterSchema
+execute(TypedParams) ToolResult
}
class ToolResult {
+success: bool
+data: variant
+error: string
}
class FileManager {
-currentFile: string
-fileMonitor: FileMonitor
-iconRegistry: IconRegistry
+loadFile(path) Result
+saveFile(path, content) Result
+renderExplorer(width)
}
class UILayout {
-mode: LayoutMode
-splitters: vector~SplitterState~
+render(textEditor, aiAgent, fileManager)
+getSplitPositions() SplitPositions
}
NedEditor --> INedHost
NedEditor --> NedConfig
NedEditor *-- TextEditor
NedEditor *-- AIAssistant
NedEditor *-- FileManager
NedEditor *-- UILayout
TextEditor *-- Document
TextEditor *-- EditorViewport
TextEditor *-- InputHandler
TextEditor *-- SyntaxHighlighter
Document *-- CursorState
AIAssistant *-- Conversation
AIAssistant *-- LLMClient
AIAssistant *-- ToolExecutor
ToolExecutor --> ITool
ITool ..> ToolResult
NedConfig *-- LayoutConfig
1. Kill all globals. No extern singletons. No #define macros. No thread_local context pointer. Every component receives its dependencies through constructors. The NedEditor is the root owner. Sub-components receive references to what they need — nothing more.
2. Replace callbacks with an interface. The 30 std::function callbacks become one INedHost interface. LuckyEngine implements it. Ned calls methods on it. Compile-time type checking. Impossible to forget a method. Parameter structs instead of 15-argument functions:
struct CreateGameObjectParams {
std::string name;
std::string shape;
Vec3 position;
Vec3 scale;
Color albedo;
float metalness;
float roughness;
bool addCollider;
};3. Document owns editor state. No shared mutable EditorState struct. The Document class owns its content, cursor, selection, and line data. Sub-components (viewport, input handler, highlighter) operate on the Document through its API, not by writing to global fields.
4. Typed tool results. ToolResult uses std::variant or a typed union instead of returning std::string for everything. Success and error are distinguishable at compile time.
5. No dummy instances. If a component isn't available, the accessor returns nullptr or std::optional. Callers check explicitly. No silent no-ops.
6. Separate config from runtime state. NedConfig is immutable after construction. Layout positions are tracked in UILayout, not in the config struct.
7. RAII thread safety. Conversation encapsulates its mutex — callers can't forget to lock because there's no raw access to the vector. Public API locks internally.
This isn't a rewrite-from-scratch proposal. A realistic migration:
-
Phase 1: INedHost interface — Extract the 30 callbacks into an interface. Keep the existing implementations behind it. This fixes the worst coupling immediately. NedContext shrinks from 326 lines to ~80.
-
Phase 2: Document class — Merge
EditorStatefields into aDocumentclass. Start by makingDocumentownfileContent,cursor_index,selection_start,selection_end,editor_content_lines. Editor sub-components takeDocument&instead of reading globals. This breaks the singleton coupling. -
Phase 3: Kill the macros — Replace
gEditor,gSettings, etc. with proper dependency injection. Each class receives what it needs in its constructor. -
Phase 4: Fix ownership — Delete the Settings leak. Replace dummy instances with optional/null. Give
AIAgentRendera proper owner. -
Phase 5: Typed tool system — Replace
string→stringmaps withToolCallstructs andToolResultvariants.
Each phase is independently shippable and testable. No big-bang rewrite required.
Ned Architecture Review — Response
This is a point-by-point response to the architecture review. Some of the criticisms are valid and worth acting on. Others misunderstand the constraints or contain factual errors. This response tries to be honest about both.
Context the Review Doesn't Account For
Before the individual points, two constraints shape most of Ned's architecture:
Cross-engine embedding. Ned has been embedded in both Unreal Engine and LuckyEngine (Hazel). Many decisions — callbacks over interfaces, the context object, thread-local pointers — exist to keep Ned compilable with zero
#includeof any host engine header. Solutions that are mentioned introduce compile-time coupling to the host.ImGui immediate-mode. Ned is built on ImGui, where components naturally combine state and rendering in the same function. Several critiques apply retained-mode OOP patterns (separate render from state, split a component into 6 single-responsibility classes) that fight this paradigm. That doesn't excuse all structural problems, but it does mean the "correct" decomposition looks different than in a typical OOP codebase.
Factual Corrections
A few claims in the review are wrong and should be corrected:
std::maprehash (#14): The review claimsUndoRedoManager*into astd::mapis "invalidated if the map rehashes."std::mapis a red-black tree — it doesn't rehash. Pointers tostd::mapvalues are stable across insertions.inputBuffer[10000](#15): The review claims "no bounds checking, undefined behavior." This buffer is passed toImGui::InputTextMultilinewhich takes the buffer size as a parameter and handles bounds internally. Overflow is truncated by ImGui, not UB.std::bad_function_call. In practice, callbacks are checked before invocation in the MCP tool handlers (e.g.,if (context && context->getEntityListCallback)). Not every callback is required — hosts set only the ones they need.Point-by-Point
1. NedContext — "God Object"
The review is right that NedContext is large and the 30
std::functionfields with no grouping are messy. The 23 identical lazy-init getters are real boilerplate that should be templated or macro'd.Where the review goes wrong: calling it a "god object" when it's functioning as a service locator — a pattern appropriate for plugin architectures where the library can't know its host at compile time.
The 15-parameter
createGameObjectCallbackis an intentional aggregate. Without it, creating a visible object with collision requires 5-6 separate ECS calls. The parameters map directly to those operations. That said, a parameter struct would improve readability without adding real complexity.2. Global Macros
The
#definemacros are convenience shortcuts forGetNedContext()->getX(). The review overstates the runtime cost (it's a single pointer read + one-time lazy init), but the namespace collision concern is legitimate. These are#definemacros in an embeddable library — any host project with agEditororgSettingssymbol gets silently broken.Action: Replace with namespaced
inlinefunctions. The existing TODO acknowledges this.3. Dummy Instances
The review calls these "the worst option." They're a standard stub pattern for compiling out optional dependencies (libgit2, curl, terminal, LSP) without
#ifdefat every call site. The review's proposed alternatives —std::optional(null checks at 100+ call sites), removing the API (breaks compilation), nullptr (less safe than stubs) — are all worse in practice.No action needed.
4. Callback Architecture
The review proposes replacing 30
std::functioncallbacks with anINedHostabstract interface. This would create a compile-time header dependency from Ned to the host engine, breaking cross-engine embedding. Callbacks are the correct decoupling mechanism here.However, the review is right about one thing: the same 30 signatures written in NedEmbedOptions, NedContext, and HazelAgentInterface is real maintenance burden. If someone updates a signature in one place and forgets another, it's a bug.
Action: Consolidate into a single shared callback struct that both NedEmbedOptions and NedContext use, eliminating the duplication.
5. NedEmbedOptions
The review claims 70+ fields — actual count is ~60, but the point stands: the struct mixes construction-time configuration with runtime-mutated state (
cameraViewportfields). That violates the semantic contract of an "options" struct.Action: Split into immutable config and mutable runtime state.
6. Editor Singletons and EditorState
This is the review's strongest point.
EditorStateis a ~35-40 field mutable struct with a single mutex (on colors only), accessed by 8+ extern singleton components. The lack of access control means any subsystem can write any field at any time. This is where real bugs come from.The review overstates the multi-instance concern (Ned runs as one instance per process), and the extern editor sub-components are internal implementation details that don't need NedContext's lifecycle management. But the shared mutable state problem is genuine.
Action: Reduce
EditorStatesurface area — group related fields, add targeted synchronization, consider making sub-components operate through a narrower API rather than direct field access.7. AI Agent Public Fields
The review criticizes public fields and
friend classon AIAgent. Hazel usesfriend class118 times across its own codebase (Entity, Scene, Mesh, etc.) — this is a project convention for tightly coupled internals, not a code smell. Public fields match ImGui's own patterns where immediate-mode rendering needs direct state access.The
inputBuffercriticism is factually wrong — see corrections above.No action needed beyond the thread safety improvements in #13.
8. MCP String Typing
Tool calls originate from LLM-generated JSON. The parameters are strings at the system boundary because that's what the models produce. Each tool handler internally parses to the correct types. Typed dispatch at this boundary would just add another serialization layer.
The review is right that
AngelScriptServerbeing special-cased with its ownunique_ptron the Manager is inconsistent.Action: Clean up AngelScriptServer or remove it.
9. Settings Leak
Valid.
static Settings* staticSettings = new Settings()with nodeleteis a real bug. The "TEMPORARY" comment has outlived its purpose. The root cause (likelythispointer corruption from init ordering) should be investigated. Failing that,static Settings instance(not pointer) avoids the leak.Action: Fix the root cause or replace with a proper static instance.
10. FileExplorer Mixed Concerns
In ImGui, a component naturally handles its own state, rendering, and input. Splitting FileExplorer into more classes would create objects that all reference each other and all render in the same frame — more complex, not less.
The review is right that
getIconForFile()with hardcoded filename checks should be a data table.Minor action: Extract icon mapping to a lookup table.
11. Build System
Both Premake and CMake exist because different host engines use different build systems.
The 17 suppressed MSVC warnings are worth auditing — some (signed/unsigned, narrowing) catch real bugs and shouldn't be blanket-suppressed, however i was asked to add these supressions to clean up logs.
Action: Audit warning suppressions and re-enable the legitimate ones.
12. String-Based Integration
The string interface exists because Ned communicates with LLMs via JSON. The real type checking happens inside the host engine's callback implementations where strings are parsed into engine types (UUID, glm::vec3, etc.). Adding typed intermediaries at the boundary is serialization boilerplate.
No action needed.
13. Thread Safety
Valid.
messagesMutexis locked in 31 places with no annotations on which methods are thread-safe vs main-thread-only.EditorDiff::getPendingEditsMutex()returns a raw mutex reference with "caller must hold the mutex!" — this discipline will eventually be violated.MainThreadDispatcheris correctly designed and its pattern should be the standard.Action: Add thread-safety annotations throughout. Wrap
messages+messagesMutexin an accessor class with lock-on-access semantics. DocumentMainThreadDispatcheras the canonical concurrent pattern.14. Memory Management
The review lists different ownership patterns and declares all wrong. In reality:
unique_ptr<T>— correct (the review's own word)const char*to host-owned string — documented non-owning pointer, standard practiceThe
std::maprehash claim is factually wrong (see corrections above). The Settings leak is a real bug (see #9).No action needed beyond fixing #9.
15. Input Buffer
See factual corrections. ImGui handles bounds checking via the buffer size parameter.
char[]is what ImGui's input API expects. Not a bug.No action needed.
16. Comment Quality
The review cherry-picks honest TODO and TEMPORARY comments and treats them as evidence of bad engineering. Developers documenting known tech debt is good practice — the alternative (pretending everything is perfect) is worse.
The Unreal ABGR color comment with no
#ifdefis a legitimate concern if applied globally.Minor action: Guard platform-specific color handling with
#ifdef.On the Proposed Architecture
The review's class diagram proposes a clean-room redesign. Some of its principles are sound in isolation, but the proposal doesn't account for the constraints it would break:
INedHostinterface — creates compile-time dependency from Ned to host engineDocumentclass — sound idea, but adding an API layer between EditorState and render code fights ImGui's paradigm. The right version of this is reducing EditorState's surface area, not wrapping itThe migration path is more realistic than the class diagram, but Phase 1 (INedHost interface) is the wrong starting point for the reasons above. The right starting point is fixing the things that cause actual bugs: Settings leak, thread safety, EditorState mutation surface area.
Summary
#definewith namespacedinlinefunctionsstd::optional,#ifdefat 100+ call sites) are worsefriend classis a Hazel-wide convention (118 uses); public fields match ImGui immediate-mode patternsInputTextMultilinetakes buffer size and handles bounds internally;char[]is the expected APIThe review identifies real problems but wraps them in enough overstatement and factual errors that it's easy to dismiss wholesale. Roughly a third of its points deserve action. The proposed redesign is not viable as presented, but the underlying instinct (reduce shared mutable state, improve type safety at internal boundaries, eliminate duplication) is sound when applied within the actual constraints.