(feat): Lua Scripting ported from OTA RME - #996
Conversation
…eneration tools
Implements a sandboxed Lua 5.2 scripting environment with extensive editor API bindings, UI dialog system, and procedural generation libraries (noise, algorithms, geometry). Adds script manager UI, documentation, and example scripts.
- `source/lua/*.cpp/h` — implement Lua engine, sandbox, and API bindings for editor objects (app, map, tile, item, ui)
- `source/ext/fast_noise_lite.h` — add FastNoiseLite library for procedural noise generation
- `source/lua/lua_api_{noise,algo,geo}.cpp` — expose procedural generation APIs (FBM, cellular automata, erosion, bezier, etc.)
- `source/lua/lua_dialog.cpp` — implement chainable Lua UI dialog system with custom widgets
- `source/lua/lua_script_manager.cpp` — manage script discovery, execution, and event system
- `source/lua/lua_scripts_window.cpp` — add Script Manager UI panel for debugging and execution
- `scripts/*.lua` — add example scripts (hello world, FPS counter, terrain generator, linter types)
- `scripts/README.md` — document Lua API reference and usage examples
Added full Lua scripting support to RME including script manager, event system, map overlays, and procedural generation APIs. Integrates sol2 bindings, HTTP client (cpr), and JSON support with CI/CD workflows for Linux/Windows builds.
Co-authored-by: karolak6612 <88726580+karolak6612@users.noreply.github.com>
fix: resolve remaining code review comments
fix: replace invalid getMapOverlay with collectMapOverlayCommands
feat(lua): modernize scripting system and update dependencies
Added cpr HTTP library dependency and refactored Lua scripting infrastructure with improved error logging, modernized include paths, and switched JSON library from json_spirit to nlohmann/json. Created legacy ItemType adapter for backward compatibility, enhanced tile operations with deepCopy and select/deselect methods, and added script management menu handlers.
fix(lua_dialog): prevent crashes and improve sizer layout handling
Added null checks for controllers in map_display.cpp to prevent crashes. Fixed wxStaticBoxSizer child management and enhanced getSizerFlags to handle wxEXPAND conflicts with alignment flags based on sizer orientation. Added mouse event re-binding in MapPreviewCanvas to override base class bindings, improved LuaListBox selection validation, changed non-modal dialog cleanup from Veto to Destroy, and registered garbage collection meta_function for proper LuaDialog cleanup.
feat(lua): add comprehensive test framework and improve Lua API
Added 11 new test files covering all Lua API modules (app, map, tile, item, noise, geo, algo, image, ui, http) with a custom test framework. Updated C++ bindings to support test requirements: added app.setBrush, fixed mapView callbacks with sol::this_state, renamed brushes.getList to getNames (returns names not objects), changed tile.position to return {x,y,z} table, and added SCRIPT_DIR to package.path for local module resolution. Improved script scanning to handle manifest.lua at current directory level and exclude test_framework.lua.
feat(tests): add comprehensive Lua API test suite and fix brush/selection APIs
Added 16 new test files covering all Lua APIs (Position, Selection, Creature, Brush, Color, JSON, HTTP, Noise, Algo, Geo, Dialog, Items) with ~1000+ assertions. Updated test runners with categorized execution (core/extended/compatibility). Fixed C++ APIs: Brushes.get now validates input, Selection bounds/minPosition/maxPosition handle empty selections correctly, Brush.canDraw uses proper Map* casting.
feat(scripts): add sand tomb maze generator for RME Redux editor
Ported otMapGenPublic maze generation logic to work in RME Redux editor with full transaction support. Added 52 new files including core classes (AutoBorder, Brush, CaveGroundMapper, RoomBuilder, WallAutoBorder, Detailer, ElevationBuilder), TSP pathfinding tools, data definitions for sand tomb theme (borders, brushes, shapes, elevation schemas), and compatibility layer for RME Redux API integration.
Co-Authored-By: karolak6612 <88726580+karolak6612@users.noreply.github.com>
|
🤖 Hi @karolak6612, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a sandboxed Lua scripting subsystem with engine, Sol2 bindings, many Lua APIs (map/tile/item/image/dialog/http/noise/algo/geo), a script manager/UI (Script Manager pane, dynamic Scripts menu), overlay rendering integration, sample scripts/docs, build deps (lua/sol2/cpr), and non-interactive batch build behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MainFrame
participant ScriptMgr as LuaScriptManager
participant Engine as LuaEngine
participant Script
participant Editor
User->>MainFrame: Open Script Manager / Reload Scripts
MainFrame->>ScriptMgr: reloadScripts() / discoverScripts()
ScriptMgr->>Engine: initialize() / registerAll()
ScriptMgr->>Engine: executeFile(script.path)
Engine->>Script: load & run (sandboxed)
Script->>Editor: call app/map APIs (transactions, overlays, events)
Script->>ScriptMgr: register overlay / addEventListener / logOutput
ScriptMgr->>MainFrame: refresh menus/UI / update Scripts window
sequenceDiagram
participant MapView
participant ScriptMgr as LuaScriptManager
participant OverlayLua as Lua overlay (ondraw)
participant Drawer as LuaOverlayDrawer
participant Renderer
MapView->>ScriptMgr: collectMapOverlayCommands(viewInfo)
ScriptMgr->>OverlayLua: call ondraw(viewInfo) with SCRIPT_DIR context
OverlayLua-->>ScriptMgr: return MapOverlayCommand[]
ScriptMgr-->>Drawer: pass commands vector
Drawer->>Renderer: MapToScreen / draw rect/line/text/sprite
Renderer->>ScriptMgr: mouse hover -> updateMapOverlayHover(...)
ScriptMgr->>OverlayLua: call onhover(...) -> tooltip/highlight
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
|
🤖 Hi @karolak6612, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
|
Warning Gemini encountered an error creating the summary. You can try again by commenting |
|
/gemini review |
|
🤖 Hi @karolak6612, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (23)
config.toml-71-71 (1)
71-71:⚠️ Potential issue | 🟠 MajorUser-specific screenshot directory should be cleared.
Same issue as above—this path is user-specific and should be empty or use a relative/placeholder path.
🔧 Proposed fix
-screenshot_directory = 'C:\Users\karol\Pictures\RME\' +screenshot_directory = ''🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config.toml` at line 71, The screenshot_directory setting in config.toml currently contains a user-specific absolute path ('screenshot_directory'), which should be cleared or replaced with a neutral value; update the screenshot_directory entry to be empty, a relative path, or a placeholder (e.g., '' or './screenshots') so the repository contains no personal filesystem references and consumers can configure their own path.config.toml-30-30 (1)
30-30:⚠️ Potential issue | 🟠 MajorUser-specific path should not be committed.
This line contains a developer's local filesystem path (
C:\Users\karol\...). User-specific paths in committed configuration files will break for other users and may expose filesystem structure. Consider using an empty string or a placeholder path.🔧 Proposed fix
-recent_files = [ 'C:\Users\karol\Downloads\karmia-tfs(1)\karmia.otbm' ] +recent_files = [ ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config.toml` at line 30, The committed config contains a user-specific path in the recent_files entry; update the recent_files value in config.toml (the recent_files key) to remove the local absolute path and replace it with a neutral value (e.g., an empty array or a placeholder path) so no developer-specific filesystem paths are stored in the repo.config.toml-449-449 (1)
449-449:⚠️ Potential issue | 🟠 MajorUser-specific client path should be cleared.
This
clientPathcontains a developer's local path. Clear it for the default configuration.🔧 Proposed fix
[[clients]] -clientPath = 'C:\Users\karol\Desktop\Clients\1098' +clientPath = '' default = false name = '10.98'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config.toml` at line 449, Remove the developer-specific local path from the default config by clearing the clientPath value (the config key "clientPath" in config.toml) — replace 'C:\Users\karol\Desktop\Clients\1098' with an empty string or a neutral placeholder (e.g., "" or "<path-to-client>") so the default config contains no user-specific file system references.source/lua/lua_api_brush.cpp-106-107 (1)
106-107:⚠️ Potential issue | 🟠 MajorUse a
static_castfor base-class conversion.
reinterpret_cast<BaseMap*>(map)bypasses pointer adjustment and will cause undefined behavior ifBaseMapis not at offset 0 in the inheritance layout. SinceMapinherits directly fromBaseMap, the cast should usestatic_cast<BaseMap*>(map)instead.Suggested fix
- return b && map && b->canDraw(reinterpret_cast<BaseMap*>(map), Position(x, y, z)); + return b && map && b->canDraw(static_cast<BaseMap*>(map), Position(x, y, z));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_brush.cpp` around lines 106 - 107, The lambda bound as "canDraw" uses reinterpret_cast<BaseMap*>(map) which can cause UB; change the cast to static_cast<BaseMap*>(map) inside the lambda passed to the "canDraw" binding so the conversion from Map* to BaseMap* correctly applies pointer adjustment (update the lambda in the "canDraw" binding where Brush*, Map*, BaseMap*, Position are referenced).source/lua/lua_api_geo.cpp-136-186 (1)
136-186:⚠️ Potential issue | 🟠 MajorClamp
stepsto a positive value before computingt.Both Bezier helpers divide by
numSteps. Passingsteps = 0currently yields invalid coordinates, and negative values silently skip generation.🛠️ Proposed fix
- int numSteps = steps.value_or(20); + int numSteps = std::max(1, steps.value_or(20));Apply the same change in both
bezierCurveandbezierCurve3d.Also applies to: 190-243
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_geo.cpp` around lines 136 - 186, The code must clamp the incoming steps value to a positive minimum before using it to compute t to avoid division by zero or skipping points; in both bezierCurve and bezierCurve3d replace the current numSteps assignment with a clamp such as int numSteps = std::max(1, steps.value_or(20)); (or equivalent logic to force at least 1) so negative or zero steps become 1 and the subsequent t = i / numSteps computation is always valid.source/lua/lua_script.cpp-114-146 (1)
114-146:⚠️ Potential issue | 🟠 MajorRaw substring matching will misparse valid
manifest.luafiles.
getValue()only recognizes four exact spellings and searches the whole file blindly. Tabs/extra spaces can makemainand metadata disappear, while commented or longer keys can be matched accidentally, which then silently falls back to defaults. Please parse key/value pairs token-wise or line-wise instead of using rawfind()over the full file.Also applies to: 176-199
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_script.cpp` around lines 114 - 146, The getValue lambda currently does raw substring matches across content and will misparse valid manifest.lua (e.g., extra spaces/tabs, commented lines, longer keys containing the target), so replace the blind find() approach in getValue with a line-wise/token-wise parser: iterate content line by line, trim whitespace, skip lines starting with -- (comments), split each non-comment line into a key token and a value token (ensuring the key matches exactly, not as a substring), accept values quoted with either single or double quotes and unescape them, and return the matched value; apply the same fix to the other duplicate parser at the later block (the code at lines 176-199) so both use exact key matching, whitespace-tolerant parsing, and comment handling instead of the four hard-coded pattern finds.source/lua/lua_api_geo.cpp-874-900 (1)
874-900:⚠️ Potential issue | 🟠 MajorReject non-positive
minDistancebefore building the Poisson grid.
cellSizeis derived directly fromminDistance.0drives division by zero, and negative inputs can turn into invalid or enormous grid sizes a few lines later.🛠️ Proposed fix
geoTable.set_function("poissonDiskSampling", [](int x1, int y1, int x2, int y2, float minDistance, sol::optional<sol::table> options, sol::this_state s) -> sol::table { sol::state_view lua(s); sol::table result = lua.create_table(); + if (minDistance <= 0.0f) { + return result; // or raise a Lua error + } int seed = static_cast<int>(time(nullptr));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_geo.cpp` around lines 874 - 900, The poissonDiskSampling lambda does not validate minDistance and can divide by zero or produce invalid grid sizes; before computing cellSize and grid dimensions inside the poissonDiskSampling function, check that minDistance is > 0 and return an empty/result table or throw a Lua error if not; reference the symbols minDistance, cellSize, gridWidth, gridHeight and perform the validation right after reading options (and before computing cellSize = minDistance / std::sqrt(2.0f)), ensuring you handle the failure path consistently (e.g. return result or call luaL_error via sol) so downstream math cannot divide by zero or produce absurd grid sizes.source/lua/lua_api_noise.cpp-138-143 (1)
138-143:⚠️ Potential issue | 🟠 Major
"euclidean"should map toCellularDistanceFunction_Euclidean, notEuclideanSq.The FastNoiseLite enum defines both
CellularDistanceFunction_EuclideanandCellularDistanceFunction_EuclideanSqas distinct distance metrics. Currently, both the"euclidean"and"euclideanSq"string options map toCellularDistanceFunction_EuclideanSq, preventing callers from accessing the plain Euclidean distance function.Proposed fix
- if (distFn == "euclidean" || distFn == "euclideanSq") { - noise.SetCellularDistanceFunction(FastNoiseLite::CellularDistanceFunction_EuclideanSq); + if (distFn == "euclidean") { + noise.SetCellularDistanceFunction(FastNoiseLite::CellularDistanceFunction_Euclidean); + } else if (distFn == "euclideanSq") { + noise.SetCellularDistanceFunction(FastNoiseLite::CellularDistanceFunction_EuclideanSq); } else if (distFn == "manhattan") {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_noise.cpp` around lines 138 - 143, The mapping for the "euclidean" distFn is wrong: update the conditional that calls noise.SetCellularDistanceFunction so that when distFn == "euclidean" it passes FastNoiseLite::CellularDistanceFunction_Euclidean (leave "euclideanSq" mapping to CellularDistanceFunction_EuclideanSq), i.e. change the branch handling distFn == "euclidean" to use CellularDistanceFunction_Euclidean while keeping the existing branches for "euclideanSq", "manhattan", and "hybrid" intact; refer to the distFn variable and the noise.SetCellularDistanceFunction calls when making the change.scripts/terrain_generator_demo.lua-103-107 (1)
103-107:⚠️ Potential issue | 🟠 Major
setGroundTilestacks terrain instead of replacing it.
tile:addItem(tileId)appends another item to the tile. Re-running any generator over the same area will keep piling up water/ground/wall items instead of producing a single terrain state. Use the ground setter/replacer here, or clear the previous ground before adding the new one.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/terrain_generator_demo.lua` around lines 103 - 107, The setGroundTile function is currently stacking terrain because it uses tile:addItem(tileId); change it to replace the ground instead of appending by using the tile's ground setter or by clearing existing ground before adding the new one (locate setGroundTile, app.map:getOrCreateTile, tile:addItem) — e.g., call the tile method that sets/replaces ground or perform tile:clearGround() then add the new terrain so re-running generators won’t accumulate items.scripts/terrain_generator_demo.lua-135-145 (1)
135-145:⚠️ Potential issue | 🟠 MajorThese handlers update the wrong config key for the generators.
The generators read
cfg.seed, but these handlers write the UI seed intoconfig.seedand then passconfig.island/config.cave/config.dungeon, none of which containsseed. Right now those generators fall back toos.time(), so the seed inputs and persisted seed are not actually honored.Also applies to: 216-221, 255-260, 513-514, 553-553, 587-587
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/terrain_generator_demo.lua` around lines 135 - 145, The UI handlers are writing the seed and generator settings into the wrong table (they set values on config.* but the generators read from cfg.*), so change those handlers to write into cfg.seed and the corresponding cfg.island / cfg.cave / cfg.dungeon tables (instead of config.island / config.cave / config.dungeon) and ensure any save/persist calls use cfg not config; update all occurrences mentioned (around the blocks that set seed and generator configs where seed is later read in the generation loop and functions like noise.fbm) so the seed input and persisted generator settings are actually used by the generators.source/lua/lua_api_app.cpp-428-445 (1)
428-445:⚠️ Potential issue | 🟠 MajorValidate the resolved storage path, not just
name.
SCRIPT_DIRis pulled from Lua state and can be reassigned by the script. Right nownameis checked, butSCRIPT_DIR = "../../.."still letsapp.storage("config")escape the script directory. Canonicalize the combined path and enforce an engine-owned base directory before exposingload/save/clear.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_app.cpp` around lines 428 - 445, The current check only validates the raw name; instead canonicalize the combined path and validate it against an engine-controlled base directory before exposing it via storage["path"]. After building path from scriptDir + "/" + filename in lua_api_app.cpp (the variables scriptDir, filename, path and the app.storage call), replace the simple checks with: join scriptDir and filename, resolve the combined path using std::filesystem::canonical or weakly_canonical to normalize symlinks and "..", and then ensure the resolved path is within a trusted engine base directory (e.g., compare that resolvedPath.string().rfind(engineBaseDir, 0) == 0 or use relative path checking). If the resolved path is outside the engine base, block and return lua.create_table(); otherwise set storage["path"] to the canonicalized resolved path.source/lua/lua_api_http.cpp-125-132 (1)
125-132:⚠️ Potential issue | 🟠 MajorURL safety check is bypassable (SSRF risk).
The
isUrlSafefunction can be bypassed using alternative localhost representations:
0.0.0.0,0x7f000001(hex IP),2130706433(decimal IP)127.0.0.2through127.255.255.255(entire 127.0.0.0/8 block)- IPv6 variations:
[0:0:0:0:0:0:0:1],[::ffff:127.0.0.1]- DNS rebinding attacks or local DNS entries pointing to 127.0.0.1
Consider using a URL parsing library to extract and validate the host, then resolve it to check if it's a private/loopback IP.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_http.cpp` around lines 125 - 132, isUrlSafe currently does a naive substring check and is bypassable; update isUrlSafe to properly parse the URL host (use a URL parsing library or std::regex to extract the host), then resolve/parse that host into one or more IP addresses (getaddrinfo or equivalent) and verify none fall into loopback/private ranges (127.0.0.0/8, 0.0.0.0, IPv4-mapped IPv6 like ::ffff:127.0.0.1, ::1, unique local/private IPv6, RFC1918 ranges, etc.); reject on DNS names that resolve to local addresses and also treat numeric/hex/decimal IPv4 forms equivalently by converting to binary before checking—make these checks inside isUrlSafe so URL inputs like hex/decimal IPs, 127.*.*, IPv6 variants, and DNS-rebinding-resolved loopback addresses are correctly blocked.source/lua/lua_scripts_window.cpp-265-271 (1)
265-271:⚠️ Potential issue | 🟠 MajorPotential command injection via
scriptsPath.The scripts folder path is inserted directly into a shell command string. If the path contains shell metacharacters (quotes, backticks,
$, etc.), this could lead to command injection.🐛 Proposed fix: Use wxLaunchDefaultApplication or escape the path
-#ifdef _WIN32 - wxExecute("explorer \"" + scriptsPath + "\"", wxEXEC_ASYNC); -#elif defined(__APPLE__) - wxExecute("open \"" + scriptsPath + "\"", wxEXEC_ASYNC); -#else - wxExecute("xdg-open \"" + scriptsPath + "\"", wxEXEC_ASYNC); -#endif + wxLaunchDefaultApplication(scriptsPath);
wxLaunchDefaultApplicationhandles path escaping properly and is cross-platform.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_scripts_window.cpp` around lines 265 - 271, The code currently builds a shell command with scriptsPath and calls wxExecute (wxExecute("explorer \"" + scriptsPath + "\"", ...)), which allows command injection; replace that pattern by calling wxLaunchDefaultApplication(scriptsPath) (or wxLaunchDefaultApplication(wxString::FromUTF8(scriptsPath.c_str())) as appropriate) instead of composing shell commands, and remove platform-specific wxExecute branches (keep scriptsPath variable and any surrounding logic in lua_scripts_window.cpp). If wxLaunchDefaultApplication is unsuitable on a platform, ensure you properly escape/quote the path using wxFileName or equivalent API rather than string-concatenating into wxExecute and preserve async behavior if needed.source/lua/lua_engine.cpp-244-247 (1)
244-247:⚠️ Potential issue | 🟠 Major
package.pathgrows unboundedly on repeated script execution.Each call to
executeFileprepends the script directory topackage.pathwithout checking if it's already present. Running multiple scripts or re-running scripts will causepackage.pathto grow indefinitely, potentially impactingrequireperformance and memory.🐛 Proposed fix: Store and restore original package.path
+// In LuaEngine class, add member: +// std::string originalPackagePath; +// In initialize(), after opening libraries: +// originalPackagePath = lua["package"]["path"]; bool LuaEngine::executeFile(const std::string& filepath) { // ... lua["SCRIPT_DIR"] = scriptDir; - // Add SCRIPT_DIR to package.path so 'require' can find local modules - std::string path = lua["package"]["path"]; - std::string newPath = scriptDir + "/?.lua;" + path; + // Set package.path with script dir prepended to original path + std::string newPath = scriptDir + "/?.lua;" + originalPackagePath; lua["package"]["path"] = newPath;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_engine.cpp` around lines 244 - 247, The code in executeFile prepends scriptDir+"/?.lua;" to lua["package"]["path"] every run which makes package.path grow unbounded; to fix, capture the original lua["package"]["path"] at the start of executeFile (e.g., origPath = lua["package"]["path"]), check whether scriptDir+"/?.lua;" is already present and only prepend if missing, and after script execution restore lua["package"]["path"] to origPath (or ensure no duplicate entries remain); update the logic around variables lua, scriptDir and newPath (used when setting lua["package"]["path"]) to perform these checks and restoration.source/lua/lua_scripts_window.cpp-50-60 (1)
50-60:⚠️ Potential issue | 🟠 MajorPotential use-after-free if window destroyed while callback is pending.
The output callback captures
thisand usesCallAfterfor cross-thread updates. If theLuaScriptsWindowis destroyed while aCallAfterlambda is queued, the lambda will execute with a danglingthispointer.🐛 Proposed fix: Use weak reference or ensure callback is cleared before destruction
One approach is to ensure the callback cannot be invoked after destruction begins:
LuaScriptsWindow::~LuaScriptsWindow() { // Clear the callback first to prevent new invocations g_luaScripts.setOutputCallback(nullptr); + + // Process any pending events that might have been queued + wxTheApp->ProcessPendingEvents(); if (instance == this) { instance = nullptr; } }Alternatively, use
std::weak_ptror a flag checked inside the lambda.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_scripts_window.cpp` around lines 50 - 60, The callback passed to g_luaScripts via g_luaScripts.setOutputCallback captures this and can run after LuaScriptsWindow is destroyed; update the implementation so the lambda cannot access a dangling pointer — e.g., in LuaScriptsWindow ensure the callback is cleared in the destructor (call g_luaScripts.setOutputCallback(nullptr)) or change the capture to a safe weak mechanism: store a std::weak_ptr to the LuaScriptsWindow (or an atomic "alive" flag) when setting the callback and inside the CallAfter lambda check lock() (or the flag) before calling LogMessage; reference g_luaScripts.setOutputCallback, LuaScriptsWindow destructor, LogMessage, wxTheApp->CallAfter and ensure any queued CallAfter lambda verifies the object is still valid before dereferencing this.source/lua/lua_api_algo.cpp-313-327 (1)
313-327:⚠️ Potential issue | 🟠 MajorNormalize the erosion brush and special-case radius
0.The weights built here are later used as absolute multipliers, but they never sum to
1. One erosion step can therefore remove several timesamountToErode, and Line 322 also hits0/0whenerosionRadius == 0.Also applies to: 421-428
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_algo.cpp` around lines 313 - 327, The erosion brush building uses brushWeights that are not normalized (so applying them can erode more than intended) and divides by radius causing 0/0 when erosionRadius == 0; update the loops that populate brushIndices and brushWeights (the variables brushIndices, brushWeights and the loop over radius/ x/ y) to special-case radius==0 by pushing a single entry {0,0} with weight 1.0f, and for radius>0 compute all weights as currently done, then compute their sum and divide each weight by that sum to normalize them to sum == 1.0f (ensuring you skip normalization only for the radius==0 case to avoid division by zero).source/lua/lua_api_image.cpp-84-97 (1)
84-97:⚠️ Potential issue | 🟠 MajorStore the resolved client sprite id for item-backed images.
fromItemSprite()rendersitemType.clientIDbut leavesspriteIdequal to the original item id. Anything that later readsgetSpriteId()—including equality and overlay rendering—will use the wrong sprite for most items.Suggested fix
- LuaImage::LuaImage(int id, bool isItemSprite) : - spriteId(id), spriteSource(true) { + LuaImage::LuaImage(int id, bool isItemSprite) : + spriteId(0), spriteSource(true) { if (isItemSprite) { // Get sprite ID from item type if (g_items.typeExists(id)) { ItemType itemType = g_items.getItemType(id); if (itemType.id != 0) { - loadFromSpriteId(itemType.clientID); + spriteId = itemType.clientID; + loadFromSpriteId(spriteId); } } } else { + spriteId = id; loadFromSpriteId(id); } }Also applies to: 132-139
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_image.cpp` around lines 84 - 97, The LuaImage constructor (LuaImage::LuaImage) currently leaves spriteId set to the original item id when isItemSprite is true, causing getSpriteId() to return the wrong value; update the code path that handles item-backed images (the branch that calls loadFromSpriteId(itemType.clientID)) to also assign spriteId = itemType.clientID so the resolved client sprite id is stored, and apply the same change to the other item-backed loader (the similar logic around lines 132-139 / any fromItemSprite or alternate constructor) so equality, getSpriteId(), and overlay rendering use the correct client sprite id.source/lua/lua_api_algo.cpp-187-209 (1)
187-209:⚠️ Potential issue | 🟠 MajorReject invalid generator sizes before allocating or indexing.
generateCave,voronoi,generateRandomPoints,generateMaze, andgenerateDungeonall trust Lua-supplied dimensions. Negative values underflow into hugestd::vectorsizes,generateRandomPoints(0, ...)builds invalid distributions,generateMaze(1, 1)still callscarve(1, 1)out of bounds, andmaxRoomSize < minRoomSizeleavesgenerateDungeonwith invalid room distributions.Also applies to: 583-645, 654-704, 712-748
source/lua/lua_api_algo.cpp-539-567 (1)
539-567:⚠️ Potential issue | 🟠 MajorValidate
kernelSizebefore deriving the smoothing radius.A negative
kernelSizemakesradiusnegative, so the outer loops start at negative indices and read outsidegrid. Even values are also misleading here:2still executes a3x3kernel.Minimal guard
if (options) { sol::table opts = *options; iterations = opts.get_or(std::string("iterations"), 1); kernelSize = opts.get_or(std::string("kernelSize"), 3); } + + if (kernelSize <= 0 || kernelSize % 2 == 0) { + throw sol::error("kernelSize must be a positive odd number"); + } auto grid = tableToFloatGrid(inputGrid, width, height);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_algo.cpp` around lines 539 - 567, The code uses kernelSize to compute radius and then indexes grid without validating kernelSize; ensure kernelSize is a positive odd integer before computing radius in the smoothing function that uses tableToFloatGrid, iterations, kernelSize, radius, newGrid, grid: clamp kernelSize to a minimum of 1 (or default 3), and if kernelSize is even increment it by 1 to make it odd (e.g., if kernelSize < 1 set kernelSize = 3; if kernelSize % 2 == 0 then kernelSize += 1), then compute radius = kernelSize / 2 and proceed so the outer loops (x/y from radius to width-height-radius) never start at negative indices or read out of bounds.source/lua/lua_api_algo.cpp-398-437 (1)
398-437:⚠️ Potential issue | 🟠 MajorFix the droplet speed update on downhill steps.
Here
deltaHeightisnewHeight - oldHeight, so downhill motion makes it negative. Line 436 therefore decreases speed and can drive thesqrtargument below zero, producing NaNs on steep descents.Minimal correction
- speed = std::sqrt(speed * speed + deltaHeight * gravity); + speed = std::sqrt(std::max(0.0f, speed * speed - deltaHeight * gravity));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_algo.cpp` around lines 398 - 437, The update to speed uses deltaHeight = newHeight - oldHeight which is negative on downhill steps, so replace the sqrt argument with speed*speed + (-deltaHeight) * gravity (i.e., add the potential energy when descending) and guard the sqrt with std::max(0.0f, ...) to avoid negative values; change the line that sets speed (currently using std::sqrt(speed * speed + deltaHeight * gravity)) to use std::sqrt(std::max(0.0f, speed*speed + (-deltaHeight) * gravity)) referencing the variables speed, deltaHeight, and gravity.source/lua/lua_api_image.cpp-49-74 (1)
49-74:⚠️ Potential issue | 🟠 MajorCanonicalize and component-check the image sandbox paths.
The allow-list is prefix-based, so a sibling like
.../scripts_evil/foo.pngstill passes, andfs::absolute()leaves symlink escapes intact. If the intent is “scripts/data only,” relative paths also need to be anchored to one of those roots beforeLoadFile.Possible hardening
- fs::path scriptsPath = fs::absolute(LuaScriptManager::getInstance().getScriptsDirectory()); - fs::path dataPath = fs::absolute(FileSystem::GetDataDirectory().ToStdString()); - fs::path absPath = fs::absolute(p); - - std::string absStr = absPath.string(); - std::string scriptsStr = scriptsPath.string(); - std::string dataStr = dataPath.string(); - - bool allowed = false; - if (absStr.find(scriptsStr) == 0) { - allowed = true; - } - if (absStr.find(dataStr) == 0) { - allowed = true; - } + fs::path scriptsPath = fs::weakly_canonical(LuaScriptManager::getInstance().getScriptsDirectory()); + fs::path dataPath = fs::weakly_canonical(FileSystem::GetDataDirectory().ToStdString()); + fs::path absPath = fs::weakly_canonical(p); + + auto isWithin = [](const fs::path& candidate, const fs::path& base) { + fs::path rel = candidate.lexically_relative(base); + return !rel.empty() && *rel.begin() != ".."; + }; + + bool allowed = isWithin(absPath, scriptsPath) || isWithin(absPath, dataPath);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_image.cpp` around lines 49 - 74, Replace the simple prefix check on fs::absolute paths with canonicalized, symlink-resolving checks and also anchor relative paths to allowed roots: call std::filesystem::canonical (or weakly_canonical with try/catch) on scriptsPath, dataPath and on the candidate path (absPath) to resolve symlinks and .. escapes, then verify the canonical candidate string starts with the canonical scripts/data strings; for relative inputs, first join the relative path to each allowed root (e.g., scriptsPath / path and dataPath / path), canonicalize those candidates and only allow if at least one canonicalized candidate is contained under the corresponding canonical root; update the allowed boolean logic around the variables scriptsPath, dataPath, absPath and handle filesystem exceptions to deny on error.source/lua/lua_script_manager.cpp-41-71 (1)
41-71:⚠️ Potential issue | 🟠 MajorOnly flip
initializedafter discovery succeeds.Any exception after
engine.initialize()currently leaves a partially started engine behind, and exceptions after Line 57 also leaveinitialized == true. The nextinitialize()can then report success without a clean startup.Suggested recovery path
try { // Initialize the Lua engine if (!engine.initialize()) { lastError = "Failed to initialize Lua engine: " + engine.getLastError(); spdlog::error("LuaScriptManager::initialize - {}", lastError); return false; } // Register all APIs registerAPIs(); - - initialized = true; // Discover scripts discoverScripts(); + initialized = true; return true; } catch (const std::exception& e) { + scripts.clear(); + clearAllCallbacks(); + engine.shutdown(); + initialized = false; lastError = "Exception during Lua initialization: " + std::string(e.what()); spdlog::error("LuaScriptManager::initialize - Caught std::exception: {}", lastError); return false; } catch (...) { + scripts.clear(); + clearAllCallbacks(); + engine.shutdown(); + initialized = false; lastError = "Unknown exception during Lua initialization"; spdlog::error("LuaScriptManager::initialize - Caught unknown exception"); return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_script_manager.cpp` around lines 41 - 71, The initialize logic in LuaScriptManager::initialize sets the initialized flag before discoverScripts() completes, leaving a partially-started engine on exceptions; change the flow so initialized is only set to true after discoverScripts() completes successfully (i.e., after engine.initialize(), registerAPIs(), and discoverScripts() all succeed), and on any failure or caught exception clear/leave initialized false and perform necessary cleanup (e.g., shut down or reset engine) while updating lastError and logging via spdlog; reference the methods and symbols engine.initialize(), registerAPIs(), discoverScripts(), initialized, lastError, and LuaScriptManager::initialize when making the change.source/lua/lua_script_manager.h-108-149 (1)
108-149:⚠️ Potential issue | 🟠 MajorCapture event arguments once before dispatching to multiple listeners.
The
emitandemitCancellabletemplates forward the same argument pack viastd::forward<Args>(args)...to each listener in the loop. With multiple listeners registered for the same event, if a caller passes an rvalue or moved object, the first callback may destructively consume it, leaving subsequent listeners with a moved-from value. Materialize the arguments once before the loop, then pass stable lvalues to each listener.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_script_manager.h` around lines 108 - 149, The issue is that emit and emitCancellable forward the same Args... pack to each listener, allowing the first listener to move-from rvalues and break later listeners; fix by materializing the arguments once into stable values before the loop (e.g., capture as a std::tuple of decayed types or a vector of sol::object equivalents) and then invoke each listener by expanding that captured tuple into lvalues for the callback; update the emit and emitCancellable implementations (referencing emit, emitCancellable, EventListener and eventListeners) to create the captured_args tuple once and use std::apply (or equivalent) to call listener.callback with those preserved lvalue arguments while keeping existing error handling and return semantics.
🟡 Minor comments (11)
source/rendering/ui/map_display.cpp-412-414 (1)
412-414:⚠️ Potential issue | 🟡 MinorGood defensive null checks, but apply consistently to properties handlers.
The null-safety guards for
selection_controlleranddrawing_controllerare a good improvement. However,OnMousePropertiesClick(line 534) andOnMousePropertiesRelease(line 555) still callselection_controllerunconditionally, creating an inconsistent pattern.🛡️ Suggested fix for OnMousePropertiesClick
void MapCanvas::OnMousePropertiesClick(wxMouseEvent& event) { SetFocus(); int mouse_map_x, mouse_map_y; ScreenToMap(event.GetX(), event.GetY(), &mouse_map_x, &mouse_map_y); Tile* tile = editor.map.getTile(mouse_map_x, mouse_map_y, floor); if (g_gui.IsDrawingMode()) { g_gui.SetSelectionMode(); } - selection_controller->HandlePropertiesClick(Position(mouse_map_x, mouse_map_y, floor), event.ShiftDown(), event.ControlDown(), event.AltDown()); + if (selection_controller) { + selection_controller->HandlePropertiesClick(Position(mouse_map_x, mouse_map_y, floor), event.ShiftDown(), event.ControlDown(), event.AltDown()); + }Also applies to: 459-465, 495-501
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/ui/map_display.cpp` around lines 412 - 414, Add consistent null-safety checks for selection_controller and drawing_controller in the properties mouse handlers: update OnMousePropertiesClick, OnMousePropertiesRelease and the other handlers around the indicated regions (functions that call selection_controller at lines ~459-465 and ~495-501) to first verify the pointer is non-null before invoking methods like HandleClick, HandleDoubleClick or any drawing_controller calls; if null, skip the call. Locate calls to selection_controller->... and drawing_controller->... in those functions and wrap them in the same if (selection_controller) { ... } / if (drawing_controller) { ... } pattern used earlier so all property handlers are protected from null dereference.source/lua/lua_api_selection.cpp-94-101 (1)
94-101:⚠️ Potential issue | 🟡 MinorSame issue:
boundsproperty partially ignoresselparameter.The
boundsproperty checksselfor null but then callsgetSelectionBounds(ts)which internally callsgetCurrentSelection(). Apply the same fix as suggested fortiles.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_selection.cpp` around lines 94 - 101, The bounds property currently checks Selection* sel but then ignores it by calling getSelectionBounds(ts) which uses the global/current selection; update the bounds binding to use the provided sel: modify the lambda for "bounds" to call a getSelectionBounds overload that accepts the Selection* (or add such an overload) and pass sel (and ts) so the returned table reflects the given selection rather than the current selection; reference the "bounds" property lambda, the Selection* sel parameter, and getSelectionBounds to locate and change the call.source/lua/lua_api_selection.cpp-89-92 (1)
89-92:⚠️ Potential issue | 🟡 MinorProperty
tilesignores itsSelection* selparameter.The
tilesproperty lambda receivesSelection* selbutgetSelectionTilescallsgetCurrentSelection()internally, ignoring the passed pointer. This could cause inconsistencies if a Selection object is accessed after the active editor changes.🐛 Proposed fix to use the passed Selection pointer
- // Get tiles as a Lua table - static sol::table getSelectionTiles(sol::this_state ts) { + // Get tiles as a Lua table + static sol::table getSelectionTiles(Selection* sel, sol::this_state ts) { sol::state_view lua(ts); sol::table result = lua.create_table(); - Selection* sel = getCurrentSelection(); if (!sel) { return result; } // ... rest unchanged }Then update the property:
"tiles", sol::property([](Selection* sel, sol::this_state ts) { - return getSelectionTiles(ts); + return getSelectionTiles(sel, ts); }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_selection.cpp` around lines 89 - 92, The tiles property lambda for "tiles" currently ignores its Selection* sel argument and calls getSelectionTiles() which uses getCurrentSelection(); change it to use the provided sel instead: update the call site in the property (the lambda bound in lua_api_selection.cpp) to pass sel into a variant of getSelectionTiles that accepts a Selection* (or add an overload getSelectionTiles(sol::this_state, Selection*) and forward sel), and update the getSelectionTiles implementation to operate on the passed Selection* rather than calling getCurrentSelection(), ensuring the selection data returned matches the Selection instance provided to the lambda.scripts/README.md-70-83 (1)
70-83:⚠️ Potential issue | 🟡 MinorFix the metadata tag typo.
The bullet list says
@Tile, but the example immediately below uses@Title. Anyone copying the docs will end up with inconsistent script headers.Suggested edit
-* `@Tile: Script Name` - Sets the display name. +* `@Title: Script Name` - Sets the display name.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/README.md` around lines 70 - 83, Fix the typo in the metadata tag list: replace the incorrect `@Tile: Script Name` entry with `@Title: Script Name` so it matches the example and prevents inconsistent script headers; update the displayed symbol in the bullet list where `@Tile` appears (reference: the metadata tags section and the `@Title` usage in the example block).source/lua/lua_api_map.cpp-82-92 (1)
82-92:⚠️ Potential issue | 🟡 MinorSkip dead spawn entries instead of aborting the iterator.
If
map->getTile(pos)returnsnullptrfor one spawn entry,next()ends the whole iteration and later spawns are never yielded. Keep scanning until you find a live tile or reachendIter.Suggested fix
Tile* next() { if (!map) { return nullptr; } - if (iter != endIter) { - Position pos = *iter; - ++iter; - return map->getTile(pos); - } + while (iter != endIter) { + const Position pos = *iter; + ++iter; + if (Tile* tile = map->getTile(pos)) { + return tile; + } + } return nullptr; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_map.cpp` around lines 82 - 92, The iterator Tile* next() prematurely returns nullptr when map->getTile(pos) yields nullptr; change next() (in lua_api_map.cpp) to loop advancing iter until iter==endIter or map->getTile(pos) returns a non-null Tile* so dead spawn entries are skipped; specifically, inside next() use a while loop over iter/endIter, capture Position pos = *iter then ++iter and call map->getTile(pos) and if non-null return it, otherwise continue, finally return nullptr if no live tile found.source/lua/lua_api_noise.cpp-375-407 (1)
375-407:⚠️ Potential issue | 🟡 MinorGuard zero-width ranges in
map()andsmoothstep().Both helpers divide by a caller-supplied span. Passing equal endpoints currently produces
inf/naninstead of a deterministic fallback.🛠️ Proposed fix
noiseTable.set_function("map", [](float value, float inMin, float inMax, float outMin, float outMax) -> float { + if (inMin == inMax) { + return outMin; + } float t = (value - inMin) / (inMax - inMin); return outMin + t * (outMax - outMin); });noiseTable.set_function("smoothstep", [](float edge0, float edge1, float x) -> float { + if (edge0 == edge1) { + return x < edge0 ? 0.0f : 1.0f; + } float t = (x - edge0) / (edge1 - edge0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_noise.cpp` around lines 375 - 407, Guard against zero-length input spans in the noise helper functions: in the noiseTable.set_function "map" lambda, check if (inMax - inMin) == 0 and return a deterministic fallback (e.g., outMin or (outMin+outMax)/2) instead of dividing by zero; in the noiseTable.set_function "smoothstep" lambda, check if (edge1 - edge0) == 0 and return a deterministic value (e.g., 0.0f when x <= edge0 and 1.0f when x > edge0) before computing t to avoid inf/nan.source/lua/lua_scripts_window.cpp-297-306 (1)
297-306:⚠️ Potential issue | 🟡 Minor
OnScriptCheckToggleis defined but never triggered.This method is implemented but there's no event binding for it. The event table at lines 29-36 doesn't include a binding for checkbox toggle events, so this code is unreachable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_scripts_window.cpp` around lines 297 - 306, OnScriptCheckToggle is never bound so it never runs; add an event binding for the list checkbox toggle to call it. Update the class event table or call Connect/Bind for script_list to handle the checkbox toggle events (e.g., bind wxEVT_LIST_ITEM_CHECKED and wxEVT_LIST_ITEM_UNCHECKED or the platform's checkbox event) and point them to LuaScriptsWindow::OnScriptCheckToggle; ensure the binding references script_list, and keep the existing UpdateScriptState, g_luaScripts.setScriptEnabled and g_luaScripts.isScriptEnabled logic intact.source/lua/lua_api_item.cpp-96-107 (1)
96-107:⚠️ Potential issue | 🟡 Minor
getNameandgetDescriptionmethods takeint idbut are bound to Item usertype.These methods are registered as part of the
Itemusertype, but they take an item ID parameter rather than operating onthis. This is confusing API design - Lua users would expectitem:getName()to return the item's name, notitem:getName(someOtherId).These lookups already exist in the
Itemsnamespace (line 214-219). Consider removing these from the Item usertype to avoid confusion.🐛 Proposed fix: Remove redundant methods from Item usertype
// Methods "clone", [](const Item& item) { return item.deepCopy(); }, "rotate", &Item::doRotate, - - "getName", [](int id) -> std::string { - if (g_items.typeExists(id)) { - return g_items.getItemType(id).name; - } - return ""; - }, - "getDescription", [](int id) -> std::string { - if (g_items.typeExists(id)) { - return g_items.getItemType(id).description; - } - return ""; - },The
Items.getName(id)function at line 214 already provides this functionality.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_item.cpp` around lines 96 - 107, The Item usertype bindings for "getName" and "getDescription" are wrong because they accept an int id instead of operating on the Item instance; remove these two method registrations from the Item usertype in lua_api_item.cpp so Lua code uses the existing Items.getName(id) / Items.getDescription(id) helpers (or implement instance methods that use the item's own id if instance behavior is desired); also search for and update any callsites that rely on item:getName(id) to use Items.getName(id) or item:getName() as appropriate.scripts/linter.lua-453-455 (1)
453-455:⚠️ Potential issue | 🟡 MinorDuplicate
@typeannotation forSCRIPT_DIR.Lines 453 and 454 both have
---@type stringannotations forSCRIPT_DIR. Remove the duplicate.📝 Proposed fix
-- Global variables set by the engine ----@type string The directory containing the currently executing script. Use this to load resources relative to your script. ---@type string The directory containing the currently executing script. Use this to load resources relative to your script. SCRIPT_DIR = ""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/linter.lua` around lines 453 - 455, Remove the duplicate doc annotation for SCRIPT_DIR: keep a single `---@type string` annotation immediately above the `SCRIPT_DIR = ""` declaration and delete the extra identical `---@type string` line so only one type comment annotates the SCRIPT_DIR variable.source/lua/lua_api_tile.cpp-505-508 (1)
505-508:⚠️ Potential issue | 🟡 MinorReturn
nilfor non-positive Lua indices.This wrapper advertises 1-based indexing but still forwards
0/-1asindex - 1. Guard the binding and short-circuit tonullptrinstead of delegating an invalid index.Small wrapper fix
"getItemAt", [](Tile* tile, int index) -> Item* { - if (!tile) return nullptr; + if (!tile || index <= 0) return nullptr; // Lua uses 1-based indexing return tile->getItemAt(index - 1); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_tile.cpp` around lines 505 - 508, The Lua binding for "getItemAt" currently subtracts 1 and forwards non-positive Lua indices to Tile::getItemAt; change the wrapper so it first checks if index <= 0 and returns nullptr (Lua nil) immediately, otherwise call tile->getItemAt(index - 1). Update the lambda bound to "getItemAt" (Tile* tile, int index) to perform this guard and keep the existing null check for tile.source/lua/lua_script_manager.cpp-186-194 (1)
186-194:⚠️ Potential issue | 🟡 MinorRemove the matching “show” toggle when an overlay is deleted.
removeMapOverlay()only erases the drawable entry. The menu toggle inmapOverlayShowssurvives, so the UI can keep exposing an overlay that no longer exists.Possible follow-up
bool LuaScriptManager::removeMapOverlay(const std::string& id) { + mapOverlayShows.erase( + std::remove_if(mapOverlayShows.begin(), mapOverlayShows.end(), + [&](const MapOverlayShowItem& item) { return item.overlayId == id; }), + mapOverlayShows.end()); + for (auto it = mapOverlays.begin(); it != mapOverlays.end(); ++it) { if (it->id == id) { mapOverlays.erase(it); + if (g_gui.root) { + g_gui.UpdateMenubar(); + } return true; } } return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_script_manager.cpp` around lines 186 - 194, removeMapOverlay currently erases the drawable from mapOverlays but leaves the corresponding show toggle in mapOverlayShows causing stale UI entries; update LuaScriptManager::removeMapOverlay to also remove the matching entry from mapOverlayShows (iterate mapOverlayShows and erase the element whose id matches the removed id, or use std::erase_if) after removing from mapOverlays so the toggle is cleaned up consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b28ea08e-1fd2-4ce5-b515-fc0fabebe269
📒 Files selected for processing (92)
CMakeLists.txtbuild_ninja.batconfig.tomldata/menubar.xmlscripts/README.mdscripts/fps_counter.luascripts/hello_world/hello_world.luascripts/hello_world/manifest.luascripts/linter.luascripts/terrain_generator_demo.luasource/CMakeLists.txtsource/app/application.cppsource/app/preferences/preferences_layout.cppsource/brushes/brush.hsource/editor/action.hsource/editor/action_queue.cppsource/editor/action_queue.hsource/editor/editor.hsource/editor/persistence/editor_persistence.cppsource/editor/selection.cppsource/ext/fast_noise_lite.hsource/game/creatures.hsource/game/items.hsource/io/filehandle.hsource/lua/lua_api.cppsource/lua/lua_api.hsource/lua/lua_api_algo.cppsource/lua/lua_api_algo.hsource/lua/lua_api_app.cppsource/lua/lua_api_app.hsource/lua/lua_api_brush.cppsource/lua/lua_api_brush.hsource/lua/lua_api_color.cppsource/lua/lua_api_color.hsource/lua/lua_api_creature.cppsource/lua/lua_api_creature.hsource/lua/lua_api_geo.cppsource/lua/lua_api_geo.hsource/lua/lua_api_http.cppsource/lua/lua_api_http.hsource/lua/lua_api_image.cppsource/lua/lua_api_image.hsource/lua/lua_api_item.cppsource/lua/lua_api_item.hsource/lua/lua_api_json.cppsource/lua/lua_api_json.hsource/lua/lua_api_map.cppsource/lua/lua_api_map.hsource/lua/lua_api_noise.cppsource/lua/lua_api_noise.hsource/lua/lua_api_position.cppsource/lua/lua_api_position.hsource/lua/lua_api_selection.cppsource/lua/lua_api_selection.hsource/lua/lua_api_tile.cppsource/lua/lua_api_tile.hsource/lua/lua_dialog.cppsource/lua/lua_dialog.hsource/lua/lua_engine.cppsource/lua/lua_engine.hsource/lua/lua_script.cppsource/lua/lua_script.hsource/lua/lua_script_manager.cppsource/lua/lua_script_manager.hsource/lua/lua_scripts_window.cppsource/lua/lua_scripts_window.hsource/map/tile.cppsource/map/tile.hsource/rendering/core/coordinate_mapper.cppsource/rendering/core/coordinate_mapper.hsource/rendering/core/map_overlay.hsource/rendering/drawers/overlays/lua_overlay_drawer.cppsource/rendering/drawers/overlays/lua_overlay_drawer.hsource/rendering/map_drawer.cppsource/rendering/map_drawer.hsource/rendering/ui/map_display.cppsource/rendering/ui/map_display.hsource/rendering/ui/map_menu_handler.cppsource/rendering/ui/map_menu_handler.hsource/ui/common_windows.hsource/ui/gui.cppsource/ui/gui_ids.hsource/ui/main_frame.cppsource/ui/main_menubar.cppsource/ui/main_menubar.hsource/ui/map_popup_menu.cppsource/ui/menubar/menubar_action_manager.cppsource/ui/menubar/script_menu_handler.cppsource/ui/menubar/script_menu_handler.hsource/ui/menubar_loader.cppsource/ui/menubar_loader.hvcpkg.json
💤 Files with no reviewable changes (1)
- build_ninja.bat
| #define SOL_ALL_SAFETIES_ON 1 | ||
| #include <sol/sol.hpp> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find every sol2 include site and safety-macro definition to verify include order is consistent.
rg -n '#\s*define\s+SOL_ALL_SAFETIES_ON|#\s*include\s*<sol/sol.hpp>' .Repository: karolak6612/remeres-map-editor-redux
Length of output: 1879
🏁 Script executed:
#!/bin/bash
# Check if lua_api.h is included by other headers that also define SOL_ALL_SAFETIES_ON
echo "=== lua_api.h includes in header files ==="
rg -n '#\s*include.*lua_api\.h' source/lua/ --type cpp
echo -e "\n=== Direct sol/sol.hpp includes in .cpp files ==="
rg -n '#\s*include\s*<sol/sol\.hpp>' source/ --type cpp
echo -e "\n=== Check if headers include each other ==="
for file in source/lua/lua_api*.h; do
if [ -f "$file" ]; then
includes=$(rg '#\s*include.*lua_api.*\.h' "$file" | head -3)
if [ -n "$includes" ]; then
echo "File: $file"
echo "$includes"
fi
fi
doneRepository: karolak6612/remeres-map-editor-redux
Length of output: 1582
🏁 Script executed:
#!/bin/bash
# Check if the headers that include lua_api.h do so BEFORE or AFTER their own includes
echo "=== lua_api_noise.h full header ==="
head -30 source/lua/lua_api_noise.h
echo -e "\n=== lua_api_geo.h full header ==="
head -30 source/lua/lua_api_geo.h
echo -e "\n=== lua_api_algo.h full header ==="
head -30 source/lua/lua_api_algo.h
echo -e "\n=== lua_api_json.h full header (checking if it includes lua_api.h) ==="
head -30 source/lua/lua_api_json.hRepository: karolak6612/remeres-map-editor-redux
Length of output: 4049
Consolidate SOL_ALL_SAFETIES_ON into a single config header, or use a compiler definition.
The current approach is inconsistent and broken. While lua_api_noise.h, lua_api_geo.h, and lua_api_algo.h correctly depend on lua_api.h to get the macro, most other headers duplicate it independently (lua_api_tile.h, lua_api_selection.h, lua_api_brush.h, etc.). Worse, lua_api_json.h includes <sol/sol.hpp> directly without defining SOL_ALL_SAFETIES_ON at all, compiling sol2 with safety disabled in that translation unit while all others enable it. This creates undefined behavior and subtle bugs across the Lua API.
Move the macro definition to a dedicated config header (e.g., lua_sol_config.h) that must be included before any sol2 header, or define it via compiler flags. Then remove all duplicates.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/lua/lua_api.h` around lines 21 - 22, Consolidate the
SOL_ALL_SAFETIES_ON definition into a single include (create lua_sol_config.h)
and ensure every translation unit includes it before any sol2 header;
specifically remove duplicate `#defines` from headers like lua_api.h and others
(lua_api_tile.h, lua_api_selection.h, lua_api_brush.h) and update lua_api_json.h
which currently includes <sol/sol.hpp> without the macro to include
lua_sol_config.h first (or alternatively add the macro as a compiler flag), so
that SOL_ALL_SAFETIES_ON is defined exactly once and always present before any
inclusion of <sol/sol.hpp>.
Added comprehensive Lua security hardening with path canonicalization and private IP blocking, improved null-safety checks throughout terrain generator script, centralized sol2 config, and fixed multiple API bugs in erosion, noise, selection, and event handling.
|
🤖 Hi @karolak6612, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
|
🤖 I'm sorry @karolak6612, but I was unable to process your request. Please see the logs for more details. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
/gemini review |
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (3)
source/lua/lua_api_http.cpp (2)
453-457: Minor: Non-atomic chunk draining.The
while (hasChunks())/getNextChunk()pattern has a benign TOCTOU window—if chunks arrive between calls, they're picked up; if another consumer drains between calls,getNextChunk()safely returns an empty string. This works correctly but could be slightly more efficient with a single locked drain operation.💡 Optional: Single-lock drain method
Add to
StreamSession:std::string drainAllChunks() { std::lock_guard<std::mutex> lock(mutex_); std::string result; while (!chunks_.empty()) { result += chunks_.front(); chunks_.pop(); } return result; }Then in
httpStreamRead:- std::string data; - while (session->hasChunks()) { - data += session->getNextChunk(); - } + std::string data = session->drainAllChunks();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_http.cpp` around lines 453 - 457, Replace the non-atomic chunk draining loop in httpStreamRead that repeatedly calls StreamSession::hasChunks() and StreamSession::getNextChunk() with a single locked drain operation: add a StreamSession::drainAllChunks() method that locks the session mutex, concatenates and pops all chunks into one std::string, and returns it; then call drainAllChunks() from httpStreamRead instead of the while(hasChunks()) loop to remove the TOCTOU window and improve efficiency.
383-405: Consider documenting detached thread behavior.Using
.detach()means there's no mechanism to cancel in-flight streaming requests, and threads may be abruptly terminated on application shutdown. This is acceptable for a map editor, but users should be aware that:
- Closing a session doesn't cancel the underlying HTTP request
- Application exit during active streams may produce incomplete requests
For future improvement, consider
std::jthreadwith stop tokens or tracking futures for graceful shutdown.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_http.cpp` around lines 383 - 405, Add a clear comment above the detached std::thread that explains the implications of using .detach(): note that the lambda that launches the cpr::Post (capturing session, url, body, headers) cannot be cancelled once started, closing a Session (session->setError/session->setFinished) does not stop the in-flight HTTP request, and application shutdown may abort active requests leading to incomplete streams; also mention a future improvement option to replace std::thread with std::jthread/stop_token or track futures to enable graceful shutdown and cancellation.source/lua/lua_api_geo.cpp (1)
298-332: Mark cells visited when enqueuing infloodFill.A cell stays at
oldValueuntil it is popped, so neighboring cells can enqueue the same coordinate several times. On large uniform grids that inflates the queue and slows the fill noticeably. Mark the cell as filled/visited beforepush()so each position enters the queue once.♻️ Possible fix
- std::queue<std::pair<int, int>> queue; - queue.push({ sx, sy }); + std::queue<std::pair<int, int>> queue; + queue.push({ sx, sy }); + grid[sy][sx] = newValue; @@ - if (grid[cy][cx] != oldValue) { - continue; - } - - grid[cy][cx] = newValue; - for (int i = 0; i < numDirs; ++i) { int nx = cx + dx[i]; int ny = cy + dy[i]; if (nx >= 0 && nx < width && ny >= 0 && ny < height && grid[ny][nx] == oldValue) { + grid[ny][nx] = newValue; queue.push({ nx, ny }); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_geo.cpp` around lines 298 - 332, In the flood fill loop for function floodFill, avoid enqueuing the same cell multiple times by marking a cell as visited when you push it: when you consider a neighbor (nx, ny) that equals oldValue, immediately set grid[ny][nx] = newValue (or another visited marker) before queue.push({nx, ny}) so it won’t be enqueued again; keep the existing check that skips cells not equal to oldValue when popping. Update the logic around queue, grid, oldValue, newValue, numDirs and the dx/dy neighbor loop (respecting eightConnected) to perform the visit-marking on enqueue instead of on pop.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/terrain_generator_demo.lua`:
- Around line 153-158: In the radial falloff block (island shape) where
distFromCenter, falloff and n are computed, clamp the computed falloff to a
minimum of 0 before applying it to n so negative falloff can't invert n; replace
the current falloff usage by taking falloff = max(0, 1 - distFromCenter *
distFromCenter) (use math.max in Lua) and then multiply n by that clamped
falloff.
In `@source/lua/lua_api_algo.cpp`:
- Around line 680-715: The maze generation must reject sizes under 3×3 to avoid
out-of-bounds access: before calling carve(1, 1) (after the odd-dimension
normalization and grid allocation), check if width < 3 or height < 3 and fail
fast or return an empty/invalid result to the caller; ensure this early-return
happens before any access to grid or invoking the recursive lambda carve to
prevent writing to grid[1][1] when the allocated grid is too small.
- Around line 279-315: Options are not validated before they're used to size
containers or construct distributions; clamp/validate parsed values like
erosionRadius, iterations, seed, and room size bounds immediately after reading
from opts (e.g., validate erosionRadius >= 0 and cap to a reasonable max before
creating brushIndices/brushWeights and computing erosionRadius * 2 + 1), and
ensure minRoomSize and maxRoomSize are normalized (minRoomSize > 0 and
maxRoomSize >= minRoomSize) before any uniform_int_distribution is constructed;
apply these checks where options are read (the block that sets iterations,
erosionRadius, inertia, etc.), and only then proceed to use std::mt19937,
std::uniform_real_distribution, tableToFloatGrid, and the
brushIndices/brushWeights construction so no negative values lead to huge size_t
conversions or invalid distribution constructors.
- Around line 187-209: The Lua-facing functions generateCave, voronoi,
generateRandomPoints, and generateDungeon must validate that width and height
are positive before using them to size vectors or construct RNG distributions;
add an upfront check in each (e.g., at the start of the lambda) that rejects
non-positive sizes by throwing a Lua error (sol::error) or returning an explicit
error table/message, so you never call vector constructors with negative sizes
or create distributions like uniform_int_distribution(0, -1); ensure the
validation covers both width and height (and any other size-like params used for
distribution bounds) and short-circuits the function before any RNG or container
allocation occurs.
In `@source/lua/lua_api_app.cpp`:
- Around line 433-489: The path-check uses string prefix comparisons
(candidate.string().find(root.string()) == 0) which can be bypassed by sibling
directories or symlinks; change the containment test in the app.storage code
that builds roots (scriptsPath, dataPath, execPath, fs::path(scriptDir)) to use
filesystem-aware comparison (e.g. fs::path::lexically_relative or compare path
components) to ensure candidate is truly inside root before setting
allowed/fullPath (replace the string prefix checks for both absolute and
relative branches and the roots loop with a lexically_relative or component-wise
containment check).
In `@source/lua/lua_api_geo.cpp`:
- Around line 898-903: The sampling range is off-by-one: compute inclusive
width/height so maxX/maxY are reachable by changing width =
static_cast<float>(maxX - minX + 1) and height = static_cast<float>(maxY - minY
+ 1), then recompute cellSize, gridWidth, gridHeight from those corrected
dimensions in poissonDiskSampling and the other similar blocks (the occurrences
around the blocks you noted at 910-915, 940-945, 987-988); alternatively, if you
prefer to keep width as max-min, change any candidate acceptance checks that use
“< width” / “< height” to “<= width” / “<= height” consistently—make the fix in
the functions/methods where width, height, cellSize, gridWidth, gridHeight and
the candidate bounds checks are computed so the sampling becomes inclusive of
maxX/maxY.
In `@source/lua/lua_api_http.cpp`:
- Around line 119-122: g_streamSessions holds StreamSession objects indefinitely
and g_nextSessionId is a 32-bit int that can overflow; update lifecycle
management by (1) changing g_nextSessionId to a wider type (e.g., uint64_t) and
adding collision/unique-id checks when allocating new IDs, and (2) implementing
automatic cleanup of finished sessions: in streamRead (and any other
read/advance functions) detect when a StreamSession (class StreamSession) has
reached EOF/finished and remove its entry from g_streamSessions under
g_sessionsMutex (and release the shared_ptr) so sessions are not leaked if
streamClose is not called; keep streamClose as explicit API but add this
defensive cleanup and consider a periodic sweeper if needed.
In `@source/lua/lua_api_image.cpp`:
- Around line 43-79: The current prefix string checks (using
LuaScriptManager::getInstance().getScriptsDirectory(),
FileSystem::GetDataDirectory(), FileSystem::GetExecDirectory() and comparing
fullPath.string().find(root.string()) == 0) are unsafe and can be bypassed via
sibling names or symlinks; replace them with component-wise containment checks
using canonicalized paths and ancestor traversal. Specifically, canonicalize
both the root (scriptsPath/dataPath/execPath) and the resolved path using
std::filesystem::canonical (or equivalent) to fully resolve symlinks, then test
containment by walking the resolved path's parent_path chain (or repeatedly
comparing with root and its parents) to ensure the root is an actual ancestor of
fullPath (instead of string prefix matching); apply this logic in the absolute
path branch (where fs::path p is used) and in the relative branch where
candidate is computed, set fullPath and allowed only when the canonical ancestor
check succeeds and keep the existing exception handling.
In `@source/lua/lua_api_item.cpp`:
- Around line 56-63: The setters for the numeric item properties ("count" lambda
that calls Item::setSubtype, the "subtype" setter using Item::setSubtype,
"actionId" -> Item::setActionID, "uniqueId" -> Item::setUniqueID, and "tier" ->
Item::setTier) currently static_cast<uint16_t> without validation and will
silently wrap out-of-range values; update each setter to first validate the
incoming int is within 0..65535 and if not throw a Lua error (use the project's
Lua error mechanism) instead of performing the static_cast, ensuring you check
the same range before calling Item::setSubtype, Item::setActionID,
Item::setUniqueID, or Item::setTier.
In `@source/lua/lua_api_map.cpp`:
- Around line 135-150: The Lua binding lambda for "getOrCreateTile" trusts raw
numeric args and can create tiles at invalid coordinates; validate and
bound-check coordinates before constructing Position and calling
Map::getOrCreateTile. In the getOrCreateTile lambda (source/lua/lua_api_map.cpp)
ensure both the 1-arg Position path and the 3-number path check that x,y,z are
within allowed ranges (non-negative and <= map/world limits) — use any existing
helpers like map->isValidPosition(pos) or your global/map min/max constants, and
if out of range throw a sol::error describing the invalid coordinates instead of
proceeding. Ensure the same validation is applied regardless of input form
before returning map->getOrCreateTile(pos).
In `@source/lua/lua_api_noise.cpp`:
- Around line 377-379: Guard against zero-width input ranges in the noise
helpers: in the noise.map lambda (noiseTable.set_function("map", ...)) check if
(inMax - inMin) is zero and handle it deterministically (e.g., return outMin
when value <= inMin, return outMax when value >= inMax, or raise a Lua error)
instead of dividing by zero; likewise in the noise.smoothstep implementation
check if (edge1 - edge0) is zero and return a stable boundary (e.g., 0.0 for x <
edge0 and 1.0 for x >= edge1) or raise an error so no inf/nan is returned to
Lua. Ensure both functions use the same chosen deterministic behavior and update
their return paths and any comments accordingly.
In `@source/lua/lua_dialog.h`:
- Around line 73-77: Dockable and non-blocking LuaDialog instances can be
garbage-collected while visible because their dockPanel is parented to
g_gui.root and Show() can return before close; to fix, pin LuaDialog instances
on the C++ side (e.g., in a global registry/map) when created in
LuaDialog::LuaDialog(...) or when Show() is called for non-blocking dialogs, and
only remove/unpin them in LuaDialog::OnClose (or when the AUI pane teardown
completes) after calling dockPanel->Destroy()/pane removal; ensure Destroy() and
the LuaDialog destructor do not run while the AUI manager still references the
pane by keeping the LuaDialog alive until pane teardown is finished.
In `@source/lua/lua_engine.cpp`:
- Around line 113-143: The dofile binding currently returns a bool, breaking Lua
semantics; update the lua["dofile"] lambda to return and forward the actual
chunk return values (not just success) by changing its return type to a
sol-compatible variadic result (e.g., sol::variadic_results or
sol::protected_function_result) and have it call and return the results from the
file-execution helper; to do this, modify executeFile (or add a new wrapper) so
executeFile returns those sol results (instead of bool) and then have the
lua["dofile"] lambda call this->executeFile(fullPath) and directly return its
results, while preserving the existing checks (SCRIPT_DIR, absolute path, "..",
and "./" trimming) in the lua["dofile"] lambda.
- Around line 39-50: Remove sol::lib::io from the lua.open_libraries call (i.e.,
stop opening io at creation via lua.open_libraries) and instead explicitly clear
any cached or preload entries for restricted libraries by setting
package.loaded["io"] = nil and package.preload["io"] = nil (and do the same for
"os" and any other banned libs) after creating the state; also replace the
package.path assignment logic (currently prepending to package.path) with a full
overwrite of package.path using a curated allowlist string so no host-default
search paths remain. Update references around lua.open_libraries, the package
setup block (package.loaded/package.preload handling), and the package.path
manipulation to implement these changes.
In `@source/lua/lua_engine.h`:
- Around line 65-95: Both safeCall(Func&& func) and safeCallVoid(Func&& func)
currently only set lastError on failure and never clear it on success; update
both functions (safeCall and safeCallVoid) so that on any successful path before
returning true you clear lastError (e.g., assign empty string or reset state) —
specifically clear lastError when result.valid() is true in safeCall and clear
lastError after func() completes successfully in safeCallVoid so getLastError()
always reflects the most recent call.
In `@source/lua/lua_script_manager.cpp`:
- Around line 361-377: The Lua API currently emits MapOverlayCommand::Type::Text
via ctx["text"] (in lua_script_manager.cpp) but lua_overlay_drawer.cpp does not
handle Type::Text so nothing is drawn; either stop accepting text commands or
add rendering support. Fix by updating lua_overlay_drawer.cpp to handle
MapOverlayCommand::Type::Text in the overlay draw loop: read cmd.text,
cmd.x/cmd.y/cmd.z, cmd.screen_space and cmd.color from the MapOverlayCommand and
call the project's text-drawing routine (use the same text/font/color/transform
semantics as existing sprites/rects/lines), respecting screen vs world
coordinates and z-order; alternatively, if you prefer to reject until supported,
change ctx["text"] to not push MapOverlayCommand::Type::Text and return an
error/log when scripts call it (modify the lambda that creates cmd in
lua_script_manager.cpp).
In `@source/lua/lua_script_manager.h`:
- Around line 89-105: The manager currently stores only raw sol::function
callbacks (ContextMenuItem.callback, EventListener.callback, etc.), causing
path-sensitive helpers to resolve against the last-executed script; update the
registration structs and APIs to capture the registering script context (e.g.,
ownerScriptDir or ownerScriptId) when calling registerContextMenuItem and
addEventListener and store it in ContextMenuItem and EventListener, then ensure
every invocation of those callbacks re-establishes the captured SCRIPT_DIR /
script context in the shared Lua state (set the saved SCRIPT_DIR before calling
the sol::function and restore the previous value after) so callbacks run with
the registering script’s directory.
In `@source/lua/lua_script.cpp`:
- Around line 208-217: The autorun parsing currently treats any occurrence of
"true" in the manifest or content as enabling autorun; update the logic in the
getValue("autorun") handling and the fallback content check (references:
getValue, autorun variable, content string) to match only exact boolean tokens
("true" or "false") rather than substrings — i.e., when val is non-empty accept
only val == "true" or val == "false" (and set autorun=true only for the exact
"true"), and for the content fallback search parse for word-boundary matches
like "autorun = true" / "autorun=true" using token-aware checks (e.g., regex or
explicit scanning for identifiers and '=' with optional whitespace) so commented
lines or occurrences like "not true" do not trigger autorun; apply the same
exact-token logic to the other similar block noted around the 327-330 area.
In `@source/lua/lua_scripts_window.cpp`:
- Around line 29-36: The list control for LuaScriptsWindow is created without
checkbox support and the event table never binds the checkbox events, so
OnScriptCheckToggle is never invoked; modify the wxListCtrl creation (the code
that currently uses wxLC_REPORT | wxLC_SINGLE_SEL) to include wxLC_CHECKBOX so
items show checkboxes, and add EVT_LIST_ITEM_CHECKED and EVT_LIST_ITEM_UNCHECKED
(or EVT_LIST_ITEM_CHECKED/UNCHECKED pair) to the BEGIN_EVENT_TABLE binding them
to LuaScriptsWindow::OnScriptCheckToggle so the Status column becomes
interactive and the toggle handler is reachable.
---
Nitpick comments:
In `@source/lua/lua_api_geo.cpp`:
- Around line 298-332: In the flood fill loop for function floodFill, avoid
enqueuing the same cell multiple times by marking a cell as visited when you
push it: when you consider a neighbor (nx, ny) that equals oldValue, immediately
set grid[ny][nx] = newValue (or another visited marker) before queue.push({nx,
ny}) so it won’t be enqueued again; keep the existing check that skips cells not
equal to oldValue when popping. Update the logic around queue, grid, oldValue,
newValue, numDirs and the dx/dy neighbor loop (respecting eightConnected) to
perform the visit-marking on enqueue instead of on pop.
In `@source/lua/lua_api_http.cpp`:
- Around line 453-457: Replace the non-atomic chunk draining loop in
httpStreamRead that repeatedly calls StreamSession::hasChunks() and
StreamSession::getNextChunk() with a single locked drain operation: add a
StreamSession::drainAllChunks() method that locks the session mutex,
concatenates and pops all chunks into one std::string, and returns it; then call
drainAllChunks() from httpStreamRead instead of the while(hasChunks()) loop to
remove the TOCTOU window and improve efficiency.
- Around line 383-405: Add a clear comment above the detached std::thread that
explains the implications of using .detach(): note that the lambda that launches
the cpr::Post (capturing session, url, body, headers) cannot be cancelled once
started, closing a Session (session->setError/session->setFinished) does not
stop the in-flight HTTP request, and application shutdown may abort active
requests leading to incomplete streams; also mention a future improvement option
to replace std::thread with std::jthread/stop_token or track futures to enable
graceful shutdown and cancellation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c3989487-7dcb-47bc-b1d2-ee7c5b50acaf
📒 Files selected for processing (32)
scripts/terrain_generator_demo.luasource/lua/lua_api.hsource/lua/lua_api_algo.cppsource/lua/lua_api_app.cppsource/lua/lua_api_app.hsource/lua/lua_api_brush.cppsource/lua/lua_api_brush.hsource/lua/lua_api_color.hsource/lua/lua_api_creature.hsource/lua/lua_api_geo.cppsource/lua/lua_api_http.cppsource/lua/lua_api_http.hsource/lua/lua_api_image.cppsource/lua/lua_api_image.hsource/lua/lua_api_item.cppsource/lua/lua_api_item.hsource/lua/lua_api_json.hsource/lua/lua_api_map.cppsource/lua/lua_api_map.hsource/lua/lua_api_noise.cppsource/lua/lua_api_position.hsource/lua/lua_api_selection.cppsource/lua/lua_api_selection.hsource/lua/lua_api_tile.hsource/lua/lua_dialog.hsource/lua/lua_engine.cppsource/lua/lua_engine.hsource/lua/lua_script.cppsource/lua/lua_script_manager.cppsource/lua/lua_script_manager.hsource/lua/lua_scripts_window.cppsource/lua/lua_sol_config.h
✅ Files skipped from review due to trivial changes (14)
- source/lua/lua_api_app.h
- source/lua/lua_sol_config.h
- source/lua/lua_api_http.h
- source/lua/lua_api_json.h
- source/lua/lua_api_tile.h
- source/lua/lua_api_item.h
- source/lua/lua_api_map.h
- source/lua/lua_api_creature.h
- source/lua/lua_api_position.h
- source/lua/lua_api_selection.h
- source/lua/lua_api_brush.h
- source/lua/lua_api_color.h
- source/lua/lua_api.h
- source/lua/lua_api_image.h
|
🤖 Hi @karolak6612, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
|
https://github.com/coderabbitai full review |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (8)
source/lua/lua_script_manager.h (1)
137-161:⚠️ Potential issue | 🟠 MajorRestore
SCRIPT_DIRinemitCancellable().This path still invokes callbacks under whatever script context was left by the previous execution, so cancellable listeners resolve
dofile(),require(), andapp.storage()against the wrong directory. Mirror the same save/set/restore logic thatemit()already uses, including the early-break path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_script_manager.h` around lines 137 - 161, emitCancellable currently invokes listener callbacks without restoring the Lua SCRIPT_DIR, so callbacks use whatever script context was left by prior code; update emitCancellable (the function that uses initialized, eventListeners and listener.callback) to mirror the save/set/restore pattern used in emit(): save the current SCRIPT_DIR before iterating, set SCRIPT_DIR to the event's script directory (or the manager's current script dir) while calling callbacks, and always restore the original SCRIPT_DIR on all exit paths including when you break early due to a consumed boolean; ensure error handling via logOutput remains unchanged and that the early-break path still restores SCRIPT_DIR before returning.source/lua/lua_api_http.cpp (2)
55-64:⚠️ Potential issue | 🟠 Major
streamClose()does not actually stop the stream.Erasing the session from
g_streamSessionsonly drops the registry entry. The detached worker keeps itsshared_ptr, andappendChunk()has no closed-state check, so a closed stream can continue downloading and buffering until timeout or EOF. Add a closed/cancelled flag and make the write callback returnfalseas soon as the session is closed.Also applies to: 418-439, 520-527
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_http.cpp` around lines 55 - 64, streamClose currently only erases the registry entry but doesn't stop the detached worker or prevent further buffering; add a closed/cancelled flag to the StreamSession (e.g., bool closed_ protected by mutex_), set it in streamClose (and notify cv_), update appendChunk(const std::string& chunk) to check closed_ under the same mutex and return false immediately if closed, and update the HTTP write callback that calls appendChunk to check session->isClosed() (or rely on appendChunk's new behavior) and return false as soon as the session is closed so the transfer stops; make analogous changes in the other mentioned blocks (lines ~418-439 and ~520-527) where chunks are appended or write callbacks are used.
230-248:⚠️ Potential issue | 🔴 CriticalThe SSRF guard is still TOCTOU because requests use the original hostname.
isUrlSafe()resolves the host up front, but the subsequentcpr::Get/Postcall performs a fresh DNS lookup forurl. A rebinding attacker can pass the first resolution and send the real request to loopback/private space on the second. Use the validated IP for the connection path (preserving the originalHostheader), or restrict this API to literal/allowlisted destinations.Also applies to: 268-291, 383-429
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_http.cpp` around lines 230 - 248, The SSRF TOCTOU is caused by calling cpr::Get/cpr::Post with the original URL after validating via isUrlSafe(); change the request to connect to the validated IP instead while preserving the original Host header and path/port: extract the original host, port and path from the URL, build a request URL using safeIp (and port if present), and set headers["Host"] = original_host (unless caller already provided Host) before calling cpr::Get/cpr::Post; apply the same fix for the other call sites in the file (the blocks around lines 268-291 and 383-429) so DNS resolution used for safety is the address actually contacted.source/lua/lua_api_geo.cpp (3)
679-714:⚠️ Potential issue | 🟡 MinorSkip shared endpoints between polygon segments.
Each Bresenham edge includes both endpoints, so every polygon vertex is emitted twice at the join with the next edge. That makes
geo.polygon()results contain duplicate vertex tiles.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_geo.cpp` around lines 679 - 714, When rasterizing polygon edges with the Bresenham loop over verts you produce duplicate shared endpoints (each edge emits both endpoints). Modify the inner emission so that for all edges except the very first one you do not emit the starting vertex twice: inside the for-loop over verts (using x1,y1,x2,y2,x,y,err,e2) skip creating/adding the first point of the edge when i>0 (e.g. only add the point if !(i>0 && x==x1 && y==y1)), so result and index only receive a vertex once at each polygon join.
541-558:⚠️ Potential issue | 🟡 MinorDeduplicate mirrored ellipse points before appending.
addEllipsePoints()still pushes all four reflections unconditionally, sogeo.ellipse(..., { filled = false })returns duplicate coordinates wheneverex == 0orey == 0. Outline consumers that place or count tiles will double-process symmetry-axis points.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_geo.cpp` around lines 541 - 558, The addEllipsePoints lambda currently always appends four mirrored points causing duplicates when ex==0 or ey==0; change it to deduplicate by checking ex and ey: if ex==0 && ey==0 push only one point (cx,cy); else if ex==0 push the two unique vertical points (cx,cy+ey) and (cx,cy-ey); else if ey==0 push the two unique horizontal points (cx+ex,cy) and (cx-ex,cy); otherwise keep the existing four-way push; update uses of result and index inside addEllipsePoints accordingly (reference addEllipsePoints, result, index, cx, cy, ex, ey, and sol::table creation).
896-897:⚠️ Potential issue | 🟠 MajorThe
+0.001fworkaround still heavily under-samples the max row/column.After the float sample is truncated back to
int, the interval that maps tomaxX/maxYis only0.001units wide while every other tile gets a full1.0. The upper boundary is technically reachable now, but it is still effectively biased out of the distribution. Use an inclusive domain ofmax - min + 1with matching grid math, or sample integer output cells directly.Also applies to: 908-909, 938-939, 985-986
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_geo.cpp` around lines 896 - 897, Replace the fragile "+0.001f" hack when computing width/height with an inclusive-domain approach: compute width = float(maxX - minX + 1) and height = float(maxY - minY + 1), and update the sampling math that converts the normalized float to an integer cell so it maps uniformly into [minX..maxX] and [minY..maxY] (e.g. use floor(u * (maxX - minX + 1)) or sample integer cells directly: sampledX = minX + static_cast<int>(floor(u * (maxX - minX + 1))) and similarly for Y). Apply this change to the occurrences around the width/height variables (the blocks using width, height, maxX, minX, maxY, minY noted in the diff and the other similar locations referenced).source/lua/lua_script.cpp (1)
115-181:⚠️ Potential issue | 🟠 MajorInline manifest booleans still miss
autorun = true.
getValue("autorun")only handles quoted values, so normal boolean entries fall through to the regex. Because that regex is anchored to the start of the line, manifests likereturn { main = "tool.lua", autorun = true }still default toautorun = false.Also applies to: 211-235
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_script.cpp` around lines 115 - 181, getValue only extracts quoted string values so unquoted booleans like autorun = true are missed; after finding eqPos in the getValue lambda, if the first non-space char after '=' is not a quote, read the unquoted token by advancing to the next delimiter (comma, newline, '}', or end-of-file), trim trailing whitespace/commas, and return that token (so "true"/"false" or numeric literals are handled); apply the same fix to the duplicate parsing block (the other getValue-like lambda) so both locations correctly return unquoted boolean values.source/lua/lua_script_manager.cpp (1)
486-551:⚠️ Potential issue | 🟠 Major
onhovercallbacks run without restoringSCRIPT_DIR.The overlay stores
ownerScriptDirat registration (line 170), butupdateMapOverlayHover()doesn't setSCRIPT_DIRbefore invokingonhover. Compare withcollectMapOverlayCommands()which correctly saves/restoresSCRIPT_DIRat lines 336-343. Scripts using relativedofile()orapp.storage()in hover callbacks will resolve paths against the wrong directory.🛠️ Proposed fix
+ std::string oldScriptDir = engine.getState()["SCRIPT_DIR"].get_or(std::string("")); for (const auto& overlay : activeOverlays) { try { + engine.getState()["SCRIPT_DIR"] = overlay.ownerScriptDir; sol::object result = overlay.onhover(info); if (!result.valid() || result.is<sol::nil_t>()) { continue; } // ... rest of handler ... } catch (const sol::error& e) { logOutput("Overlay hover '" + overlay.id + "' error: " + std::string(e.what()), true); } } + engine.getState()["SCRIPT_DIR"] = oldScriptDir; mapOverlayHover.valid = any;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_script_manager.cpp` around lines 486 - 551, updateMapOverlayHover() invokes overlay.onhover without setting/restoring SCRIPT_DIR, so hover callbacks run with the wrong working script directory; mirror the pattern used in collectMapOverlayCommands() by saving the current SCRIPT_DIR, setting SCRIPT_DIR to overlay.ownerScriptDir before calling overlay.onhover (and catching exceptions as now), then restoring the original SCRIPT_DIR afterward; reference functions/variables: updateMapOverlayHover(), overlay.onhover, overlay.ownerScriptDir, SCRIPT_DIR, and collectMapOverlayCommands() to locate and apply the change.
🧹 Nitpick comments (1)
source/lua/lua_script_manager.cpp (1)
131-158: Consider clamping color component values.
parseColor()reads integer values from Lua tables without bounds checking. While wxColor typically clamps values internally, explicitly clamping to 0-255 would provide clearer error behavior for invalid script input.♻️ Optional defensive fix
+static int clampColorComponent(int value) { + return std::max(0, std::min(255, value)); +} + static wxColor parseColor(const sol::object& obj, const wxColor& fallback) { // ... existing code ... - return wxColor(r, g, b, a); + return wxColor(clampColorComponent(r), clampColorComponent(g), + clampColorComponent(b), clampColorComponent(a)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_script_manager.cpp` around lines 131 - 158, parseColor reads integer components from the Lua table and returns wxColor without explicit bounds checking; update parseColor to clamp each component (r, g, b, a) to the 0–255 range after you obtain them (including values read by tbl.get_or(...) and indexed accesses) to prevent out-of-range values being passed to wxColor—use std::clamp or an equivalent min/max clamp for r, g, b, a before constructing wxColor(r,g,b,a).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/lua/lua_api_algo.cpp`:
- Around line 198-206: The implicit seeding uses time(nullptr) (e.g., local
variable seed in the
generateCave/erode/generateRandomPoints/generateMaze/generateDungeon callers)
which can produce identical generators within the same second; change the
implicit path to derive seed from std::random_device (or use a single shared
std::mt19937 engine) instead of time(nullptr) while preserving the explicit seed
parameter for reproducible runs. Locate the seed initialization in functions
referenced (generateCave, erode, generateRandomPoints, generateMaze,
generateDungeon and the similar blocks around the other noted ranges) and
replace/time-branch the default to e.g. random_device-generated seed or obtain
the RNG from a shared_engine helper, and only use the passed-in seed when
provided.
In `@source/lua/lua_api_app.cpp`:
- Around line 375-387: The transaction wrapper ignores the provided name
parameter so script-created undo entries are generic; update the transaction
handling in the static transaction(const std::string& name, sol::function func)
function to apply the label to the LuaTransaction before committing (or when
beginning) so history stores the provided name: after obtaining LuaTransaction&
trans = LuaTransaction::getInstance() and before trans.commit(), call the
appropriate API to set the label (e.g. trans.setLabel(name) or pass name into
trans.begin(name) if supported) so Editor.getHistory() shows the supplied name
for the Lua action.
In `@source/lua/lua_script_manager.cpp`:
- Around line 584-599: discoverScripts() currently calls engine.shutdown() then
engine.initialize() but never re-registers the Lua host APIs, causing scripts to
lack app/map/selection/etc.; after the engine.initialize() call in
LuaScriptManager::discoverScripts() invoke registerAPIs() (the same function
used after initialization elsewhere) before scanning/loading scripts (i.e.
before or immediately after getScriptsDirectory()/scanDirectory() and before
runAutoScripts()) so the Lua environment has the app/map/selection APIs
available to discovered scripts.
- Around line 268-287: The ontoggle handler in
LuaScriptManager::setMapOverlayShowEnabled invokes item.ontoggle without
restoring SCRIPT_DIR to the overlay owner's directory; update the function to
save the current SCRIPT_DIR, set SCRIPT_DIR = item.ownerScriptDir before calling
item.ontoggle(enabled), and then restore the previous SCRIPT_DIR afterward
(ensure restoration happens even if item.ontoggle throws, e.g., via
try/catch/finally-like pattern or RAII). Make these changes in the
mapOverlayShows loop where item.ontoggle is invoked so relative path resolution
inside ontoggle uses item.ownerScriptDir.
In `@source/lua/lua_scripts_window.cpp`:
- Around line 53-75: The output callback access in g_luaScripts races between
setOutputCallback() and logOutput(): protect the manager's outputCallback to
avoid use-after-free/crash. In the LuaScripts manager, add a mutex (or
equivalent) to guard the outputCallback field, have setOutputCallback() lock and
assign the callback, and have logOutput() lock, copy the outputCallback into a
local std::function (or shared_ptr to it), unlock, then check/invoke the local
copy; leave the LuaScriptsWindow code unchanged except for using
setOutputCallback() as before (symbols: g_luaScripts, setOutputCallback(),
logOutput(), outputCallback, LuaScriptsWindow::~LuaScriptsWindow).
---
Duplicate comments:
In `@source/lua/lua_api_geo.cpp`:
- Around line 679-714: When rasterizing polygon edges with the Bresenham loop
over verts you produce duplicate shared endpoints (each edge emits both
endpoints). Modify the inner emission so that for all edges except the very
first one you do not emit the starting vertex twice: inside the for-loop over
verts (using x1,y1,x2,y2,x,y,err,e2) skip creating/adding the first point of the
edge when i>0 (e.g. only add the point if !(i>0 && x==x1 && y==y1)), so result
and index only receive a vertex once at each polygon join.
- Around line 541-558: The addEllipsePoints lambda currently always appends four
mirrored points causing duplicates when ex==0 or ey==0; change it to deduplicate
by checking ex and ey: if ex==0 && ey==0 push only one point (cx,cy); else if
ex==0 push the two unique vertical points (cx,cy+ey) and (cx,cy-ey); else if
ey==0 push the two unique horizontal points (cx+ex,cy) and (cx-ex,cy); otherwise
keep the existing four-way push; update uses of result and index inside
addEllipsePoints accordingly (reference addEllipsePoints, result, index, cx, cy,
ex, ey, and sol::table creation).
- Around line 896-897: Replace the fragile "+0.001f" hack when computing
width/height with an inclusive-domain approach: compute width = float(maxX -
minX + 1) and height = float(maxY - minY + 1), and update the sampling math that
converts the normalized float to an integer cell so it maps uniformly into
[minX..maxX] and [minY..maxY] (e.g. use floor(u * (maxX - minX + 1)) or sample
integer cells directly: sampledX = minX + static_cast<int>(floor(u * (maxX -
minX + 1))) and similarly for Y). Apply this change to the occurrences around
the width/height variables (the blocks using width, height, maxX, minX, maxY,
minY noted in the diff and the other similar locations referenced).
In `@source/lua/lua_api_http.cpp`:
- Around line 55-64: streamClose currently only erases the registry entry but
doesn't stop the detached worker or prevent further buffering; add a
closed/cancelled flag to the StreamSession (e.g., bool closed_ protected by
mutex_), set it in streamClose (and notify cv_), update appendChunk(const
std::string& chunk) to check closed_ under the same mutex and return false
immediately if closed, and update the HTTP write callback that calls appendChunk
to check session->isClosed() (or rely on appendChunk's new behavior) and return
false as soon as the session is closed so the transfer stops; make analogous
changes in the other mentioned blocks (lines ~418-439 and ~520-527) where chunks
are appended or write callbacks are used.
- Around line 230-248: The SSRF TOCTOU is caused by calling cpr::Get/cpr::Post
with the original URL after validating via isUrlSafe(); change the request to
connect to the validated IP instead while preserving the original Host header
and path/port: extract the original host, port and path from the URL, build a
request URL using safeIp (and port if present), and set headers["Host"] =
original_host (unless caller already provided Host) before calling
cpr::Get/cpr::Post; apply the same fix for the other call sites in the file (the
blocks around lines 268-291 and 383-429) so DNS resolution used for safety is
the address actually contacted.
In `@source/lua/lua_script_manager.cpp`:
- Around line 486-551: updateMapOverlayHover() invokes overlay.onhover without
setting/restoring SCRIPT_DIR, so hover callbacks run with the wrong working
script directory; mirror the pattern used in collectMapOverlayCommands() by
saving the current SCRIPT_DIR, setting SCRIPT_DIR to overlay.ownerScriptDir
before calling overlay.onhover (and catching exceptions as now), then restoring
the original SCRIPT_DIR afterward; reference functions/variables:
updateMapOverlayHover(), overlay.onhover, overlay.ownerScriptDir, SCRIPT_DIR,
and collectMapOverlayCommands() to locate and apply the change.
In `@source/lua/lua_script_manager.h`:
- Around line 137-161: emitCancellable currently invokes listener callbacks
without restoring the Lua SCRIPT_DIR, so callbacks use whatever script context
was left by prior code; update emitCancellable (the function that uses
initialized, eventListeners and listener.callback) to mirror the
save/set/restore pattern used in emit(): save the current SCRIPT_DIR before
iterating, set SCRIPT_DIR to the event's script directory (or the manager's
current script dir) while calling callbacks, and always restore the original
SCRIPT_DIR on all exit paths including when you break early due to a consumed
boolean; ensure error handling via logOutput remains unchanged and that the
early-break path still restores SCRIPT_DIR before returning.
In `@source/lua/lua_script.cpp`:
- Around line 115-181: getValue only extracts quoted string values so unquoted
booleans like autorun = true are missed; after finding eqPos in the getValue
lambda, if the first non-space char after '=' is not a quote, read the unquoted
token by advancing to the next delimiter (comma, newline, '}', or end-of-file),
trim trailing whitespace/commas, and return that token (so "true"/"false" or
numeric literals are handled); apply the same fix to the duplicate parsing block
(the other getValue-like lambda) so both locations correctly return unquoted
boolean values.
---
Nitpick comments:
In `@source/lua/lua_script_manager.cpp`:
- Around line 131-158: parseColor reads integer components from the Lua table
and returns wxColor without explicit bounds checking; update parseColor to clamp
each component (r, g, b, a) to the 0–255 range after you obtain them (including
values read by tbl.get_or(...) and indexed accesses) to prevent out-of-range
values being passed to wxColor—use std::clamp or an equivalent min/max clamp for
r, g, b, a before constructing wxColor(r,g,b,a).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6ddb72fd-62a3-4e7a-8bad-9252e20c3adc
📒 Files selected for processing (11)
source/lua/lua_api_algo.cppsource/lua/lua_api_app.cppsource/lua/lua_api_geo.cppsource/lua/lua_api_http.cppsource/lua/lua_dialog.cppsource/lua/lua_engine.cppsource/lua/lua_script.cppsource/lua/lua_script.hsource/lua/lua_script_manager.cppsource/lua/lua_script_manager.hsource/lua/lua_scripts_window.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- source/lua/lua_script.h
Rewrote hello_world demo into a 5-tab UI showcase with color palette, layout patterns, and HTTP demo. Replaced wxListCtrl with wxDataViewListCtrl in script manager, added themed toolbar with icons and context menu. Improved script callback context preservation, thread-safe logging, and menubar check-item synchronization. Enhanced HTTP API with DNS bypass, algo APIs with better RNG, and geo APIs with edge-case fixes.
|
🤖 Hi @karolak6612, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
This Pull Request introduces a comprehensive Lua scripting system ported from OTA RME. The implementation is robust, well-integrated with the existing Remere's Map Editor codebase (including undo/redo support), and follows good engineering practices for script management and security.
🔍 General Feedback
- Well-Structured API: The split between core manager logic and individual API files (Map, Tile, Item, etc.) is clean and maintainable.
- Undo/Redo Integration: The use of
markTileForUndoand the customLuaTransactionsystem demonstrates a deep understanding of the editor's internals. - Security Considerations: Efforts to sandbox file access and restrict HTTP requests to safe IP ranges are commendable.
- Comprehensive Documentation: The
scripts/README.mdprovides excellent documentation for script authors. - Consistent Style: The new code adheres well to the project's coding standards.
| nlohmann::json luaToJson(const sol::object& obj) { | ||
| switch (obj.get_type()) { | ||
| case sol::type::nil: | ||
| return nullptr; | ||
| case sol::type::boolean: | ||
| return obj.as<bool>(); | ||
| case sol::type::number: { | ||
| double d = obj.as<double>(); | ||
| if (d == static_cast<int64_t>(d)) { | ||
| return static_cast<int64_t>(d); | ||
| } | ||
| return d; | ||
| } | ||
| case sol::type::string: | ||
| return obj.as<std::string>(); | ||
| case sol::type::table: { | ||
| sol::table t = obj.as<sol::table>(); | ||
|
|
||
| // Determine if it's an array or object | ||
| bool isArray = true; | ||
| size_t maxKey = 0; | ||
| size_t count = 0; | ||
|
|
||
| for (auto& pair : t) { | ||
| count++; | ||
| if (pair.first.get_type() == sol::type::number) { | ||
| double k = pair.first.as<double>(); | ||
| if (k >= 1 && k == static_cast<size_t>(k)) { | ||
| size_t idx = static_cast<size_t>(k); | ||
| if (idx > maxKey) { | ||
| maxKey = idx; | ||
| } | ||
| } else { | ||
| isArray = false; | ||
| break; | ||
| } | ||
| } else { | ||
| isArray = false; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (isArray && count > 0 && maxKey == count) { | ||
| nlohmann::json arr = nlohmann::json::array(); | ||
| for (size_t i = 1; i <= count; ++i) { | ||
| if (t[i].valid()) { | ||
| arr.push_back(luaToJson(t[i])); | ||
| } else { | ||
| arr.push_back(nullptr); | ||
| } | ||
| } | ||
| return arr; | ||
| } else { | ||
| nlohmann::json objVal = nlohmann::json::object(); | ||
| for (auto& pair : t) { | ||
| std::string key; | ||
| if (pair.first.get_type() == sol::type::string) { | ||
| key = pair.first.as<std::string>(); | ||
| } else if (pair.first.get_type() == sol::type::number) { | ||
| key = std::to_string(pair.first.as<int64_t>()); | ||
| } else { | ||
| continue; | ||
| } | ||
| objVal[key] = luaToJson(pair.second); | ||
| } | ||
| return objVal; | ||
| } | ||
| } | ||
| default: | ||
| return nullptr; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟠 The luaToJson function lacks cycle detection. Encoding a cyclic Lua table into JSON will result in a stack overflow and crash the editor.
| nlohmann::json luaToJson(const sol::object& obj) { | |
| switch (obj.get_type()) { | |
| case sol::type::nil: | |
| return nullptr; | |
| case sol::type::boolean: | |
| return obj.as<bool>(); | |
| case sol::type::number: { | |
| double d = obj.as<double>(); | |
| if (d == static_cast<int64_t>(d)) { | |
| return static_cast<int64_t>(d); | |
| } | |
| return d; | |
| } | |
| case sol::type::string: | |
| return obj.as<std::string>(); | |
| case sol::type::table: { | |
| sol::table t = obj.as<sol::table>(); | |
| // Determine if it's an array or object | |
| bool isArray = true; | |
| size_t maxKey = 0; | |
| size_t count = 0; | |
| for (auto& pair : t) { | |
| count++; | |
| if (pair.first.get_type() == sol::type::number) { | |
| double k = pair.first.as<double>(); | |
| if (k >= 1 && k == static_cast<size_t>(k)) { | |
| size_t idx = static_cast<size_t>(k); | |
| if (idx > maxKey) { | |
| maxKey = idx; | |
| } | |
| } else { | |
| isArray = false; | |
| break; | |
| } | |
| } else { | |
| isArray = false; | |
| break; | |
| } | |
| } | |
| if (isArray && count > 0 && maxKey == count) { | |
| nlohmann::json arr = nlohmann::json::array(); | |
| for (size_t i = 1; i <= count; ++i) { | |
| if (t[i].valid()) { | |
| arr.push_back(luaToJson(t[i])); | |
| } else { | |
| arr.push_back(nullptr); | |
| } | |
| } | |
| return arr; | |
| } else { | |
| nlohmann::json objVal = nlohmann::json::object(); | |
| for (auto& pair : t) { | |
| std::string key; | |
| if (pair.first.get_type() == sol::type::string) { | |
| key = pair.first.as<std::string>(); | |
| } else if (pair.first.get_type() == sol::type::number) { | |
| key = std::to_string(pair.first.as<int64_t>()); | |
| } else { | |
| continue; | |
| } | |
| objVal[key] = luaToJson(pair.second); | |
| } | |
| return objVal; | |
| } | |
| } | |
| default: | |
| return nullptr; | |
| } | |
| } | |
| // Convert Lua Object to nlohmann::json | |
| nlohmann::json luaToJson(const sol::object& obj, std::set<const void*>& visited) { | |
| switch (obj.get_type()) { | |
| case sol::type::nil: | |
| return nullptr; | |
| case sol::type::boolean: | |
| return obj.as<bool>(); | |
| case sol::type::number: { | |
| double d = obj.as<double>(); | |
| if (d == static_cast<int64_t>(d)) { | |
| return static_cast<int64_t>(d); | |
| } | |
| return d; | |
| } | |
| case sol::type::string: | |
| return obj.as<std::string>(); | |
| case sol::type::table: { | |
| sol::table t = obj.as<sol::table>(); | |
| const void* ptr = t.pointer(); | |
| if (visited.count(ptr)) { | |
| return nullptr; // or throw error | |
| } | |
| visited.insert(ptr); | |
| // Determine if it's an array or object | |
| bool isArray = true; | |
| size_t maxKey = 0; | |
| size_t count = 0; | |
| for (auto& pair : t) { | |
| count++; | |
| if (pair.first.get_type() == sol::type::number) { | |
| double k = pair.first.as<double>(); | |
| if (k >= 1 && k == static_cast<size_t>(k)) { | |
| size_t idx = static_cast<size_t>(k); | |
| if (idx > maxKey) { | |
| maxKey = idx; | |
| } | |
| } else { | |
| isArray = false; | |
| break; | |
| } | |
| } else { | |
| isArray = false; | |
| break; | |
| } | |
| } | |
| nlohmann::json result; | |
| if (isArray && count > 0 && maxKey == count) { | |
| result = nlohmann::json::array(); | |
| for (size_t i = 1; i <= count; ++i) { | |
| if (t[i].valid()) { | |
| result.push_back(luaToJson(t[i], visited)); | |
| } else { | |
| result.push_back(nullptr); | |
| } | |
| } | |
| } else { | |
| result = nlohmann::json::object(); | |
| for (auto& pair : t) { | |
| std::string key; | |
| if (pair.first.get_type() == sol::type::string) { | |
| key = pair.first.as<std::string>(); | |
| } else if (pair.first.get_type() == sol::type::number) { | |
| key = std::to_string(pair.first.as<int64_t>()); | |
| } else { | |
| continue; | |
| } | |
| result[key] = luaToJson(pair.second, visited); | |
| } | |
| } | |
| visited.erase(ptr); | |
| return result; | |
| } | |
| default: | |
| return nullptr; | |
| } | |
| } |
| struct LuaToJson { | ||
| std::set<const void*> visited; | ||
| nlohmann::json convert(sol::object obj) { | ||
| if (obj.is<bool>()) return obj.as<bool>(); | ||
| if (obj.is<int>()) return obj.as<int>(); | ||
| if (obj.is<double>()) return obj.as<double>(); | ||
| if (obj.is<std::string>()) return obj.as<std::string>(); | ||
| if (obj.is<sol::nil_t>()) return nullptr; | ||
|
|
||
| if (obj.is<sol::table>()) { | ||
| sol::table tbl = obj.as<sol::table>(); | ||
| const void* ptr = tbl.pointer(); | ||
| if (visited.count(ptr)) return nullptr; | ||
| visited.insert(ptr); | ||
|
|
||
| nlohmann::json result; | ||
| bool isArray = true; | ||
| size_t maxKey = 0, count = 0; | ||
|
|
||
| for (auto& pair : tbl) { | ||
| if (pair.first.is<size_t>()) { | ||
| size_t k = pair.first.as<size_t>(); | ||
| if (k > maxKey) maxKey = k; | ||
| count++; | ||
| } else { | ||
| isArray = false; break; | ||
| } | ||
| } | ||
|
|
||
| if (isArray && maxKey == count && maxKey > 0) { | ||
| result = nlohmann::json::array(); | ||
| for (size_t i = 1; i <= maxKey; ++i) result.push_back(convert(tbl[i])); | ||
| } else { | ||
| result = nlohmann::json::object(); | ||
| for (auto& pair : tbl) { | ||
| std::string key; | ||
| if (pair.first.is<std::string>()) key = pair.first.as<std::string>(); | ||
| else if (pair.first.is<int>()) key = std::to_string(pair.first.as<int>()); | ||
| else continue; | ||
| result[key] = convert(pair.second); | ||
| } | ||
| } | ||
| visited.erase(ptr); | ||
| return result; | ||
| } | ||
| return nullptr; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🟡 This JSON conversion logic is duplicated from lua_api_json.cpp. It's better to centralize this in one place. Additionally, the implementation here in lua_api_http.cpp correctly handles cycle detection, while the one in lua_api_json.cpp does not. They should be unified.
| cpr::Response response = cpr::Get( | ||
| cpr::Url { "http://" + safeIp + path }, | ||
| headers, | ||
| cpr::Header { { "Host", host } }, | ||
| cpr::Timeout { 10000 } | ||
| ); |
There was a problem hiding this comment.
🟡 The protocol is hardcoded to http:// here, which prevents the use of HTTPS even if the user specifies an https:// URL. This might be surprising and less secure than allowing both protocols (if SSL support is compiled into CPR).
| cpr::Response response = cpr::Get( | |
| cpr::Url { "http://" + safeIp + path }, | |
| headers, | |
| cpr::Header { { "Host", host } }, | |
| cpr::Timeout { 10000 } | |
| ); | |
| cpr::Response response = cpr::Get( | |
| cpr::Url { (url.find("https://") == 0 ? "https://" : "http://") + safeIp + path }, | |
| headers, | |
| cpr::Header { { "Host", host } }, | |
| cpr::Timeout { 10000 } | |
| ); |
| app["yield"] = []() { | ||
| if (wxTheApp && !LuaTransaction::getInstance().isActive()) { | ||
| wxTheApp->Yield(true); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🟡 Using wxTheApp->Yield(true) can be risky as it allows for re-entrancy. While the check for an active transaction mitigates some risks, other event handlers (like UI button clicks or timer events) could still trigger while yield() is running, potentially leading to unexpected states. Consider documenting this behavior for script authors.
| static sol::table storageForScript(sol::this_state ts, const std::string& name) { | ||
| sol::state_view lua(ts); | ||
| sol::table storage = lua.create_table(); | ||
|
|
||
| namespace fs = std::filesystem; | ||
|
|
||
| // Determine potential base directories | ||
| fs::path scriptsPath = fs::weakly_canonical(LuaScriptManager::getInstance().getScriptsDirectory()); | ||
| fs::path dataPath = fs::weakly_canonical(FileSystem::GetDataDirectory().ToStdString()); | ||
| fs::path execPath = fs::weakly_canonical(FileSystem::GetExecDirectory().ToStdString()); | ||
|
|
||
| std::string scriptDir = lua["SCRIPT_DIR"].get_or(std::string("")); | ||
|
|
||
| // Ensure scriptDir is safe and canonical | ||
| fs::path canonicalScriptDir = fs::weakly_canonical(scriptDir); | ||
| auto scriptDirRel = canonicalScriptDir.lexically_relative(scriptsPath); | ||
| if (scriptDirRel.empty() || scriptDirRel.string().find("..") != std::string::npos) { | ||
| scriptDir = scriptsPath.string(); | ||
| } else { | ||
| scriptDir = canonicalScriptDir.string(); | ||
| } | ||
|
|
||
| fs::path fullPath; | ||
| bool allowed = false; | ||
|
|
||
| try { | ||
| fs::path p(name); | ||
| if (p.is_absolute()) { | ||
| fullPath = fs::weakly_canonical(p); | ||
| // Check if absolute path is within allowed roots | ||
| std::vector<fs::path> allowedRoots = { scriptsPath, dataPath, execPath }; | ||
| for (const auto& root : allowedRoots) { | ||
| auto relative = fullPath.lexically_relative(root); | ||
| if (!relative.empty() && relative.string().find("..") == std::string::npos) { | ||
| allowed = true; | ||
| break; | ||
| } | ||
| } | ||
| } else { | ||
| // For relative paths, try anchoring to each root (scriptDir first) | ||
| std::vector<fs::path> roots = { fs::path(scriptDir), scriptsPath, dataPath, execPath }; | ||
| for (const auto& root : roots) { | ||
| fs::path candidate = fs::weakly_canonical(root / p); | ||
| auto relative = candidate.lexically_relative(root); | ||
| if (!relative.empty() && relative.string().find("..") == std::string::npos) { | ||
| fullPath = candidate; | ||
| allowed = true; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } catch (...) { | ||
| printf("[Lua Security] Failed to canonicalize path in app.storage: %s\n", name.c_str()); | ||
| return lua.create_table(); | ||
| } | ||
|
|
||
| if (!allowed) { | ||
| printf("[Lua Security] Blocked unsafe path in app.storage: %s\n", name.c_str()); | ||
| return lua.create_table(); | ||
| } | ||
|
|
||
| std::string path = fullPath.string(); | ||
| storage["path"] = path; | ||
|
|
||
| storage["load"] = [path](sol::this_state ts2, sol::object) -> sol::object { | ||
| sol::state_view lua(ts2); | ||
| std::ifstream file(path); | ||
| if (!file.is_open()) { | ||
| return sol::make_object(lua, sol::nil); | ||
| } | ||
|
|
||
| std::stringstream buffer; | ||
| buffer << file.rdbuf(); | ||
| file.close(); | ||
|
|
||
| std::string content = buffer.str(); | ||
| if (content.empty()) { | ||
| return sol::make_object(lua, sol::nil); | ||
| } | ||
|
|
||
| sol::table json = lua["json"]; | ||
| if (!json.valid() || !json["decode"].valid()) { | ||
| return sol::make_object(lua, sol::nil); | ||
| } | ||
|
|
||
| try { | ||
| sol::function decode = json["decode"]; | ||
| sol::protected_function_result result = decode(content); | ||
| if (!result.valid()) { | ||
| return sol::make_object(lua, sol::nil); | ||
| } | ||
| sol::object decoded = result; | ||
| return sol::make_object(lua, decoded); | ||
| } catch (const sol::error&) { | ||
| return sol::make_object(lua, sol::nil); | ||
| } | ||
| }; | ||
|
|
||
| storage["save"] = [path](sol::this_state ts2, sol::object first, sol::object second) -> bool { | ||
| sol::state_view lua(ts2); | ||
| std::string content; | ||
|
|
||
| sol::object data = (second.valid() && !second.is<sol::nil_t>()) ? second : first; | ||
| if (!data.valid() || data.is<sol::nil_t>()) { | ||
| return false; | ||
| } | ||
|
|
||
| if (data.is<std::string>()) { | ||
| content = data.as<std::string>(); | ||
| } else { | ||
| sol::table json = lua["json"]; | ||
| if (!json.valid() || !json["encode_pretty"].valid()) { | ||
| return false; | ||
| } | ||
| try { | ||
| sol::function encode = json["encode_pretty"]; | ||
| sol::protected_function_result result = encode(data); | ||
| if (!result.valid()) { | ||
| return false; | ||
| } | ||
| content = result.get<std::string>(); | ||
| } catch (const sol::error&) { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| std::ofstream file(path, std::ios::trunc); | ||
| if (!file.is_open()) { | ||
| return false; | ||
| } | ||
| file << content; | ||
| file.close(); | ||
| return true; | ||
| }; | ||
|
|
||
| storage["clear"] = [path](sol::object) -> bool { | ||
| return std::remove(path.c_str()) == 0; | ||
| }; | ||
|
|
||
| return storage; | ||
| } |
There was a problem hiding this comment.
🟡 The path validation logic in storageForScript is quite complex and manually handles relative and absolute paths. Using std::filesystem::weakly_canonical is a good practice, but consider also checking for symlinks to prevent them from pointing outside of the allowed roots if that's a concern in the target environment.
Additionally, the storage["load"] function (line 504) doesn't check the file size before loading it into memory, which could lead to large memory usage if a script points to a very large file. A reasonable size limit would be prudent.
| scripts.clear(); | ||
| clearAllCallbacks(); | ||
|
|
||
| std::string scriptsDir = getScriptsDirectory(); | ||
| scanDirectory(scriptsDir); | ||
|
|
||
| // Sort scripts by display name | ||
| std::sort(scripts.begin(), scripts.end(), [](const std::unique_ptr<LuaScript>& a, const std::unique_ptr<LuaScript>& b) { | ||
| return a->getDisplayName() < b->getDisplayName(); | ||
| }); |
There was a problem hiding this comment.
🟢 Every call to discoverScripts() (which happens via Reload Scripts) shuts down and re-initializes the entire Lua engine. While this provides a clean state, it could become a performance bottleneck if the number of scripts or the complexity of their registration grows significantly. For now, it's a good way to ensure a fresh environment.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a Lua scripting system to the map editor, enabling automation and custom tool development. It includes a new Lua script manager, a script management UI, and comprehensive API bindings for map manipulation, procedural generation (noise, algorithms, geometry), and HTTP requests. The implementation follows the project's architectural standards, including the use of smart pointers and modern C++ practices. I have reviewed the code and identified a memory management issue in the BSP dungeon generator that should be addressed to comply with the project's style guide regarding ownership.
| std::function<BSPNode*(int, int, int, int, int)> split; | ||
| split = [&](int x, int y, int w, int h, int depth) -> BSPNode* { | ||
| BSPNode* node = new BSPNode { x, y, w, h }; | ||
|
|
||
| if (depth >= maxDepth || w < minRoomSize * 2 || h < minRoomSize * 2) { | ||
| // Check if space is sufficient for a room | ||
| if (w < minRoomSize + 2 || h < minRoomSize + 2) { | ||
| return node; | ||
| } | ||
|
|
||
| // Create room | ||
| std::uniform_int_distribution<int> roomW(minRoomSize, std::min(maxRoomSize, w - 2)); | ||
| std::uniform_int_distribution<int> roomH(minRoomSize, std::min(maxRoomSize, h - 2)); | ||
|
|
||
| int rw = roomW(rng); | ||
| int rh = roomH(rng); | ||
|
|
||
| std::uniform_int_distribution<int> roomX(x + 1, x + w - rw - 1); | ||
| std::uniform_int_distribution<int> roomY(y + 1, y + h - rh - 1); | ||
|
|
||
| node->roomX = roomX(rng); | ||
| node->roomY = roomY(rng); | ||
| node->roomW = rw; | ||
| node->roomH = rh; | ||
| node->hasRoom = true; | ||
|
|
||
| rooms.push_back({ node->roomX, node->roomY, rw, rh }); | ||
|
|
||
| return node; | ||
| } | ||
|
|
||
| // Split | ||
| std::uniform_real_distribution<float> splitDist(0.3f, 0.7f); | ||
| float splitRatio = splitDist(rng); | ||
|
|
||
| bool splitHorizontal = (w < h) || (w == h && rng() % 2 == 0); | ||
|
|
||
| if (splitHorizontal) { | ||
| int splitY = y + static_cast<int>(h * splitRatio); | ||
| node->left = split(x, y, w, splitY - y, depth + 1); | ||
| node->right = split(x, splitY, w, y + h - splitY, depth + 1); | ||
| } else { | ||
| int splitX = x + static_cast<int>(w * splitRatio); | ||
| node->left = split(x, y, splitX - x, h, depth + 1); | ||
| node->right = split(splitX, y, x + w - splitX, h, depth + 1); | ||
| } | ||
|
|
||
| return node; | ||
| }; | ||
|
|
||
| BSPNode* root = split(0, 0, width, height, 0); | ||
|
|
||
| // Carve rooms | ||
| for (const auto& room : rooms) { | ||
| int rx = std::get<0>(room); | ||
| int ry = std::get<1>(room); | ||
| int rw = std::get<2>(room); | ||
| int rh = std::get<3>(room); | ||
|
|
||
| for (int py = ry; py < ry + rh && py < height; ++py) { | ||
| for (int px = rx; px < rx + rw && px < width; ++px) { | ||
| grid[py][px] = 0; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Connect rooms with corridors | ||
| for (size_t i = 1; i < rooms.size(); ++i) { | ||
| int x1 = std::get<0>(rooms[i - 1]) + std::get<2>(rooms[i - 1]) / 2; | ||
| int y1 = std::get<1>(rooms[i - 1]) + std::get<3>(rooms[i - 1]) / 2; | ||
| int x2 = std::get<0>(rooms[i]) + std::get<2>(rooms[i]) / 2; | ||
| int y2 = std::get<1>(rooms[i]) + std::get<3>(rooms[i]) / 2; | ||
|
|
||
| // L-shaped corridor | ||
| if (rng() % 2 == 0) { | ||
| // Horizontal first | ||
| for (int x = std::min(x1, x2); x <= std::max(x1, x2); ++x) { | ||
| if (y1 >= 0 && y1 < height && x >= 0 && x < width) { | ||
| grid[y1][x] = 0; | ||
| } | ||
| } | ||
| for (int y = std::min(y1, y2); y <= std::max(y1, y2); ++y) { | ||
| if (y >= 0 && y < height && x2 >= 0 && x2 < width) { | ||
| grid[y][x2] = 0; | ||
| } | ||
| } | ||
| } else { | ||
| // Vertical first | ||
| for (int y = std::min(y1, y2); y <= std::max(y1, y2); ++y) { | ||
| if (y >= 0 && y < height && x1 >= 0 && x1 < width) { | ||
| grid[y][x1] = 0; | ||
| } | ||
| } | ||
| for (int x = std::min(x1, x2); x <= std::max(x1, x2); ++x) { | ||
| if (y2 >= 0 && y2 < height && x >= 0 && x < width) { | ||
| grid[y2][x] = 0; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Cleanup BSP | ||
| std::function<void(BSPNode*)> deleteBSP = [&](BSPNode* node) { | ||
| if (node) { | ||
| deleteBSP(node->left); | ||
| deleteBSP(node->right); | ||
| delete node; | ||
| } | ||
| }; | ||
| deleteBSP(root); | ||
|
|
There was a problem hiding this comment.
The split function in the BSP dungeon generator uses new BSPNode for manual memory allocation, and a separate deleteBSP function is used for cleanup. This is error-prone and violates the project's style guide, which mandates smart pointers for ownership (rule #118: "Smart pointers for ownership").
To adhere to the style guide and improve safety, please refactor this to use std::unique_ptr<BSPNode>.
Example refactoring:
// In the split function signature
std::function<std::unique_ptr<BSPNode>(int, int, int, int, int)> split;
split = [&](int x, int y, int w, int h, int depth) -> std::unique_ptr<BSPNode> {
auto node = std::make_unique<BSPNode>(...);
// ...
node->left = split(...);
node->right = split(...);
return node;
};
// In the main function
std::unique_ptr<BSPNode> root = split(0, 0, width, height, 0);
// No manual deletion needed, unique_ptr will handle it.References
- The style guide explicitly bans raw
newanddeleteand requires smart pointers for ownership. (link)
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
source/app/application.cpp (1)
175-212:⚠️ Potential issue | 🟠 MajorDon’t call
g_luaScripts.initialize()twice.Line 176 and Line 208 both invoke
g_luaScripts.initialize(). Unless that API is explicitly idempotent, this can double-register bindings/scripts or make the second call fail with an "already initialized" error, which also skipsLoadScriptsMenu(). If early engine setup is required, keep that separate from the post-frame menu wiring.🐛 Proposed fix
@@ - // Initialize Lua scripting system EARLY (before MainFrame) + // Initialize Lua scripting system once, before MainFrame creation. if (!g_luaScripts.initialize()) { spdlog::warn("Failed to initialize Lua scripting: {}", g_luaScripts.getLastError()); } @@ - // Initialize Lua scripting system - if (!g_luaScripts.initialize()) { - spdlog::warn("Failed to initialize Lua scripting: {}", g_luaScripts.getLastError()); - } else if (g_gui.root && g_gui.root->menu_bar) { + // Hook the already-initialized scripting manager into the UI. + if (g_luaScripts.isInitialized() && g_gui.root && g_gui.root->menu_bar) { g_gui.root->menu_bar->LoadScriptsMenu(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/app/application.cpp` around lines 175 - 212, Remove the duplicate initialization of g_luaScripts: call g_luaScripts.initialize() only once (keep the early initialization before creating MainFrame) and remove the second call near the end; on failure log the error via g_luaScripts.getLastError() as already done, and after the GUI is created and menu_bar is available call g_gui.root->menu_bar->LoadScriptsMenu() without re-initializing g_luaScripts. Ensure the code references the single initialization site (g_luaScripts.initialize()) and the deferred menu wiring site (g_gui.root->menu_bar->LoadScriptsMenu()) so bindings/scripts are not double-registered.source/editor/persistence/editor_persistence.cpp (1)
338-356:⚠️ Potential issue | 🟠 MajorRestore the identity mapping for non-conflicting smart-merge houses.
The empty
elseon Line 355 leaveshouse_id_mapwithoutold_id -> old_idwhen the imported house does not conflict.importMap()later resolves house tiles through that map, so those tiles fall back to house0and never get reattached to the importedHouseobject.🧩 Suggested fix
case IMPORT_SMART_MERGE: { if (current_house) { // Compare and insert/merge depending on parameters if (current_house->name == imported_house->name && current_house->townid == imported_house->townid) { // Just add to map house_id_map[imported_house->getID()] = current_house->getID(); skip = true; Position newexit = oldexit + offset; if (newexit.isValid()) { current_house->setExit(&editor.map, newexit); } } else { // Conflict! Find a newd id and replace old uint32_t new_id = editor.map.houses.getEmptyID(); house_id_map[imported_house->getID()] = new_id; imported_house->setID(new_id); } } else { + house_id_map[imported_house->getID()] = imported_house->getID(); } break; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/editor/persistence/editor_persistence.cpp` around lines 338 - 356, When handling IMPORT_SMART_MERGE in the switch, the empty else branch (the one for when current_house is null) fails to insert an identity mapping into house_id_map, so subsequent importMap() resolution falls back to house 0; fix by adding house_id_map[imported_house->getID()] = imported_house->getID() (i.e. map old_id -> old_id) in that else branch so non-conflicting imported House objects preserve their IDs during import.
♻️ Duplicate comments (20)
source/game/creatures.h (1)
29-29:⚠️ Potential issue | 🟠 MajorDeduplicate
FileNamealias to prevent cross-header redefinition conflicts.Line 29 reintroduces the same
using FileName = wxFileName;pattern that was previously flagged. If multiple headers define it in global scope and are included together, this can break compilation.Proposed fix (local, self-contained)
-using FileName = wxFileName; @@ - bool loadFromXML(const FileName& filename, bool standard, wxString& error, std::vector<std::string>& warnings); - bool importXMLFromOT(const FileName& filename, wxString& error, std::vector<std::string>& warnings); + bool loadFromXML(const wxFileName& filename, bool standard, wxString& error, std::vector<std::string>& warnings); + bool importXMLFromOT(const wxFileName& filename, wxString& error, std::vector<std::string>& warnings); - bool saveToXML(const FileName& filename); + bool saveToXML(const wxFileName& filename); @@ - static CreatureType* loadFromOTXML(const FileName& filename, pugi::xml_document& node, std::vector<std::string>& warnings); + static CreatureType* loadFromOTXML(const wxFileName& filename, pugi::xml_document& node, std::vector<std::string>& warnings);#!/bin/bash # Verify whether FileName alias is still duplicated across headers rg -nP --type-add 'cpp:*.h' --type=cpp '\busing\s+FileName\s*=\s*wxFileName\s*;'Expected result: exactly one canonical definition (or none, if fully removed).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/game/creatures.h` at line 29, This header reintroduces the global alias "using FileName = wxFileName;" which must be deduplicated; remove the alias from this file (creatures.h) and either include the single canonical header that defines FileName or update local uses to reference wxFileName directly so there is exactly one definition of the alias across the codebase; search for occurrences of "FileName" and "wxFileName" to ensure callers are updated or that they include the canonical header instead.source/lua/lua_api_selection.cpp (1)
109-117:⚠️ Potential issue | 🟠 MajorDon't expose unowned
start/finishpairs to Lua.
start()is a no-op when the selection is already busy, butfinish()will close any busy selection. A script can therefore end an outer selection transaction it never started, and any Lua error between the two still leaves the lifecycle unbalanced. A single scopedtransaction(fn)binding is much safer here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_selection.cpp` around lines 109 - 117, The current Lua exposure of Selection::start and Selection::finish is unsafe because scripts can call finish on a selection they didn't start and errors can leave the selection busy; remove or stop exporting the unpaired "start" and "finish" lambdas and instead provide a single scoped binding "transaction" (or similar) that takes a Lua function, calls sel->start(Selection::INTERNAL) if not already busy, invokes the Lua callback, and always calls sel->finish(Selection::INTERNAL) in a finally/cleanup path (or via RAII) to guarantee balanced lifecycle; reference the existing Selection class and the current lambdas for "start"/"finish" when locating where to replace the bindings.source/rendering/core/map_overlay.h (1)
4-6:⚠️ Potential issue | 🟠 MajorInclude
<cstdint>in this header.
MapOverlayCommandexposesuint32_t sprite_id, but the header never includes the declaration for that type. Right now it relies on transitive includes.🛠️ Proposed fix
+#include <cstdint> `#include` <string> `#include` <vector> `#include` <wx/colour.h>Also applies to: 45-45
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/core/map_overlay.h` around lines 4 - 6, MapOverlayCommand declares a uint32_t member (sprite_id) but the header omits the <cstdint> include and currently relies on transitive includes; add `#include` <cstdint> to source/rendering/core/map_overlay.h (alongside the existing <string>, <vector>, <wx/colour.h>) so uint32_t is defined where MapOverlayCommand is declared and avoid relying on transitive includes.scripts/linter.lua (1)
452-455:⚠️ Potential issue | 🟡 MinorRemove the duplicated
SCRIPT_DIRannotation.Line 454 repeats Line 453 verbatim, so hover/docs will show the same declaration twice.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/linter.lua` around lines 452 - 455, The file declares SCRIPT_DIR with two identical EmmyLua annotations; remove the duplicate annotation line so only one "@type string The directory containing the currently executing script. Use this to load resources relative to your script." comment remains above the SCRIPT_DIR = "" declaration (reference: SCRIPT_DIR).source/lua/lua_api_json.cpp (1)
50-121:⚠️ Potential issue | 🟠 MajorMake JSON encoding reject cycles and non-serializable Lua values.
luaToJsonstill walks nested tables without tracking visited tables, and the fallback paths still serialize unsupported values asnullor drop unsupported keys. A cyclic table can recurse until the editor blows the stack, and script data gets silently corrupted instead of surfacing an error.Also applies to: 127-134
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_json.cpp` around lines 50 - 121, The luaToJson function must detect cyclic tables and fail on non-serializable Lua values instead of silently emitting nulls or dropping keys: add a recursive visited-tracking parameter (e.g., std::unordered_set<const void*> &visited or std::unordered_set<sol::object> by pointer/id) to luaToJson and its recursive calls (update luaToJson overload/signature), mark a table as visited when entering and remove on exit, and throw a descriptive exception (e.g., std::runtime_error) if the same table is seen again (cycle) or when encountering unsupported types (functions, threads, userdata) instead of returning nullptr or skipping keys; update the array/object branches and the default/fallback paths (including the similar code around the referenced lines 127-134) to use the new visited logic and to propagate/throw errors for unsupported values.source/lua/lua_api_image.cpp (2)
243-251:⚠️ Potential issue | 🟡 MinorTransformed images still collapse to
==.
resize()/scale()clearfilePathandspriteSource, butoperator==still treats every non-sprite image as file-backed and compares onlyfilePath. Two resized images therefore both compare as"" == "", even when their sizes or contents differ.Also applies to: 284-291
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_image.cpp` around lines 243 - 251, operator== currently treats any image with an empty filePath as identical ("" == ""), causing resized/scaled images from LuaImage::resize and LuaImage::scale to collapse; update the equality logic (operator==) so that when filePath is empty or spriteSource is false it compares intrinsic image content instead of only filePath — for example check image.IsOk(), compare dimensions (image.GetWidth/GetHeight) and either pixel buffers or a quick hash of image.GetData() to determine equality; this change will also cover the other transformed-image paths (e.g., the scale() variant).
66-79:⚠️ Potential issue | 🟠 MajorRelative image lookups still skip the active package directory.
This branch only probes
{scripts,data,exec}. A script inscripts/foo/callingImage("icon.png")still resolvesscripts/icon.pnginstead ofscripts/foo/icon.png, so package-local assets fail unless authors hardcode broader paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_image.cpp` around lines 66 - 79, The relative-path resolution loop in lua_api_image.cpp builds roots = { scriptsPath, dataPath, execPath } but omits the active package (or current script) directory, so Image("icon.png") in scripts/foo/ won't check scripts/foo/icon.png; update the roots vector used in the for-loop (the variable roots) to include the active package/script directory (e.g., packagePath or the script's parent directory) before scriptsPath, then reuse the same candidate = fs::weakly_canonical(root / p) / relative / fs::is_regular_file checks so fullPath and allowed are set when the package-local file is found (ensure you reference the same symbols: roots, scriptsPath, dataPath, execPath, p, candidate, fullPath, allowed).source/rendering/drawers/overlays/lua_overlay_drawer.cpp (1)
41-42:⚠️ Potential issue | 🟠 MajorCollect overlay commands once per frame.
Draw()andDrawUI()each callcollectMapOverlayCommands()for the same view, so every Luaondrawcallback runs twice per frame. That doubles the work and makes stateful overlays easy to desync between the primitive pass and the text pass.Also applies to: 122-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/drawers/overlays/lua_overlay_drawer.cpp` around lines 41 - 42, Collecting map overlay commands is being done twice (in Draw() and DrawUI()) by calling g_luaScripts.collectMapOverlayCommands(viewInfo, commands) which causes ondraw callbacks to run twice; change this so commands are collected once per frame and reused. Implement a per-frame cache (e.g., a member std::vector<MapOverlayCommand> cachedCommands and a frame-id or a boolean clearedEachFrame) inside the LuaOverlayDrawer class, call g_luaScripts.collectMapOverlayCommands(viewInfo, cachedCommands) only once at the start of the frame, clear/update the cache at frame boundaries, and have both Draw() and DrawUI() consume cachedCommands instead of calling collectMapOverlayCommands() again (ensure you preserve ordering/semantics and reset cachedCommands when the view changes).source/lua/lua_api_geo.cpp (2)
700-713:⚠️ Potential issue | 🟡 MinorThe closing vertex is still emitted twice.
Skipping the first point of each later edge removes the internal duplicates, but the last edge still writes the polygon's first vertex again at its endpoint. Scripts that place or count outline tiles will process that corner twice.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_geo.cpp` around lines 700 - 713, The loop currently emits the polygon's starting vertex again when the last edge reaches the initial point; fix by recording the first emitted vertex coordinates (e.g., firstX, firstY when you add the very first point into result) and, before pushing any subsequent point inside the while in the block that uses isFirstEdge/firstPoint, skip adding the point if (x == firstX && y == firstY) so the closing vertex is not duplicated; use the existing variables (firstPoint, isFirstEdge, x, y, result, index) to locate and implement this check.
908-909:⚠️ Potential issue | 🟠 Major
poissonDiskSampling()still excludes the upper bound.
width/heightare stillmax - min, candidates are clipped to that half-open span, and the export truncates withstatic_cast<int>. In practice that yieldsmin..max-1for almost every sample, somaxX/maxYare effectively unreachable.Also applies to: 950-951, 997-998
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_geo.cpp` around lines 908 - 909, poissonDiskSampling() currently computes float width/height as maxX-minX and then truncates to int, which excludes the upper bound; change the span to include the upper bound by using width = (maxX - minX) + 1.0f and height = (maxY - minY) + 1.0f (adjust the other occurrences at the identical patterns around the function), and when converting float coordinates/counts to int avoid silent truncation by using an explicit rounding strategy (e.g., std::lround or std::ceil as appropriate) instead of plain static_cast<int> so maxX/maxY become reachable.source/lua/lua_api_tile.cpp (2)
58-72:⚠️ Potential issue | 🟠 MajorReject out-of-range Lua integers before the
uint16_tcast.Negative or oversized
itemId/countvalues still wrap during the cast, which lets scripts create bogus item IDs and subtypes.Possible fix
static Item* addItemToTile(Tile* tile, int itemId, sol::optional<int> countOpt) { if (!tile) { throw sol::error("Invalid tile"); } + if (itemId < 0 || itemId > 65535) { + throw sol::error("Tile:addItem: itemId must be between 0 and 65535"); + } + if (countOpt && (*countOpt < 0 || *countOpt > 65535)) { + throw sol::error("Tile:addItem: count must be between 0 and 65535"); + } // Mark tile for undo before modification markTileForUndo(tile);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_tile.cpp` around lines 58 - 72, The addItemToTile helper currently casts Lua integers to uint16_t which allows negatives/large values to wrap; before calling Item::Create or item->setSubtype, validate that itemId is within 0..65535 and, if countOpt has a value, that *countOpt is within 0..65535; if any value is out of range throw a sol::error describing the invalid argument. Keep the markTileForUndo, Item::Create and item->setSubtype calls intact but perform these range checks first (refer to addItemToTile, Item::Create, and setSubtype to locate the code).
220-247:⚠️ Potential issue | 🟠 MajorValidate ground assignments before writing them into
tile->ground.This setter still accepts any
Item*/ integer ID and writes it straight into the ground slot. That allows non-ground items — and oversized IDs after the narrowing cast — to produce malformed tiles that later rendering/save code has to deal with.scripts/README.md (1)
70-75:⚠️ Potential issue | 🟡 MinorFix the metadata tag name in this list.
lua_script.cpprecognizes@Title:for the display name. Documenting@Tile:here means copy-pasted headers won’t set the script title.Possible fix
-* `@Tile: Script Name` - Sets the display name. +* `@Title: Script Name` - Sets the display name.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/README.md` around lines 70 - 75, Update the README entry that lists metadata tags to use the correct tag name: replace the incorrect `@Tile:` with `@Title:` so it matches what `lua_script.cpp` recognizes for the display name; ensure the `@Title:` example and any surrounding text reflect the correct spelling to prevent copy-paste errors when creating script headers.source/ui/menubar/script_menu_handler.cpp (1)
98-99:⚠️ Potential issue | 🟡 MinorUse inclusive counts for these ID ranges.
SCRIPTS_LAST - SCRIPTS_FIRSTandSHOW_CUSTOM_LAST - SHOW_CUSTOM_FIRSTare max offsets, not counts. As written, the last valid slot in each range is never populated.Possible fix
- const size_t maxScriptCount = static_cast<size_t>(SCRIPTS_LAST - SCRIPTS_FIRST); - const size_t maxShowCount = static_cast<size_t>(SHOW_CUSTOM_LAST - SHOW_CUSTOM_FIRST); + const size_t maxScriptCount = static_cast<size_t>(SCRIPTS_LAST - SCRIPTS_FIRST + 1); + const size_t maxShowCount = static_cast<size_t>(SHOW_CUSTOM_LAST - SHOW_CUSTOM_FIRST + 1);Also applies to: 103-103, 126-126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/ui/menubar/script_menu_handler.cpp` around lines 98 - 99, The size calculations for maxScriptCount and maxShowCount use exclusive offsets and omit the last ID; change their definitions to use inclusive counts by adding 1 (e.g., maxScriptCount = static_cast<size_t>(SCRIPTS_LAST - SCRIPTS_FIRST + 1) and similarly for maxShowCount using SHOW_CUSTOM_LAST and SHOW_CUSTOM_FIRST), and apply the same +1 fix to the other occurrences of these range computations in the file (the other spots that compute counts from SCRIPTS_* and SHOW_CUSTOM_*).source/lua/lua_engine.cpp (1)
40-50:⚠️ Potential issue | 🔴 CriticalDon't open
ioif the sandbox is supposed to block file access.Line 49 opens
sol::lib::io, but Line 100 only clears_G.io. Lua can still recover that library throughrequire("io")/package.loaded["io"], which gives scripts arbitrary file I/O again. Removesol::lib::iofromopen_libraries()and clear the cached/preloadedioentry as part of sandbox setup.In Lua 5.4, if the `io` library is opened and `_G.io` is later set to nil, does `require("io")` still return the cached library from `package.loaded`?Also applies to: 99-113
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_engine.cpp` around lines 40 - 50, The sandbox currently opens sol::lib::io in lua.open_libraries but only clears _G.io later, leaving package.loaded/require able to restore file I/O; remove sol::lib::io from lua.open_libraries and, as part of sandbox setup code (the block that clears _G.io), also explicitly clear package.loaded["io"] and package.preload["io"] (and any other related entries like "os" if needed) so require("io") cannot return a cached library; update the same logic applied around the existing _G.io-clearing code to remove these cache/preload entries.source/lua/lua_script_manager.h (1)
91-107:⚠️ Potential issue | 🟠 Major
ownerScriptDiris too coarse to identify the owning script.Standalone scripts in the same folder all register the same directory, so later checks cannot tell which script actually owns a listener, overlay, or show item. Disabling
scripts/a.luawill also hit registrations fromscripts/b.luaif they share that folder. Carry the script'suniqueId/relative path alongsideSCRIPT_DIRand key cleanup/enabled checks off that instead.Also applies to: 181-195
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_script_manager.h` around lines 91 - 107, The ownerScriptDir field is too coarse to identify which script registered items; update the ContextMenuItem and EventListener structs (and the analogous structs around lines 181-195) to carry a script-unique identifier (e.g., ownerScriptId or ownerScriptRelativePath) in addition to or instead of ownerScriptDir, and change registerContextMenuItem (and the matching registration functions) to populate this unique id from the script context; finally, update all cleanup, disable/enable checks, and any comparisons that currently use ownerScriptDir to use the new unique identifier so registrations from different files in the same folder are distinguished.source/lua/lua_api_http.cpp (3)
256-267:⚠️ Potential issue | 🔴 CriticalPreserve the original scheme and port when pinning the validated IP.
Lines 263 and 312 always build
http://<safeIp><path>. That silently downgradeshttps://URLs to plain HTTP and drops explicit ports like:8443/:8080, so secure endpoints either fail or get contacted over the wrong transport.Also applies to: 305-317
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_http.cpp` around lines 256 - 267, The code always prefixes the pinned IP with "http://" losing the original URL's scheme and any explicit port; update the URL-rewrite logic to detect and preserve the original scheme and port from the input url (using the existing parsing around hostStart/pathStart/host/path) and then build cpr::Url using "<scheme>://<safeIp><optionalPort><path>" while still sending the original Host header; apply the same fix to the duplicate block around the other occurrence (lines ~305-317) so HTTPS and explicit ports like :8443 are preserved.
420-432:⚠️ Potential issue | 🟠 MajorThe stream limit does not bound live worker threads.
MAX_STREAM_SESSIONSonly guardsg_streamSessions.size(), buthttpStreamClose()erases the entry before the detached request thread exits. A script can looppostStream()+streamClose()and accumulate far more than 16 live requests until the 30s timeout expires.Also applies to: 546-552
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_http.cpp` around lines 420 - 432, The current limit checks only g_streamSessions.size() so postStream()+streamClose() can churn sessions while detached request threads remain alive; fix by bounding live worker threads too: introduce an atomic/int like g_activeStreamCount (or repurpose g_streamSessions to track active thread state) and increment it when allocating a session in postStream()/the session-creation block that uses g_nextSessionId and g_streamSessions, reject new sessions if g_activeStreamCount >= MAX_STREAM_SESSIONS, and ensure the detached request thread decrements g_activeStreamCount (and only then erases/cleans the session) in its final cleanup path (also update httpStreamClose() to not erase the session before the thread finishes or to signal and wait/transfer cleanup responsibility to the thread).
409-455:⚠️ Potential issue | 🔴 Critical
postStream()validatessafeIpand then ignores it.The worker still calls
cpr::Post(cpr::Url{ url }, ...), so the actual request is free to resolve the hostname again. That reintroduces the DNS-rebinding/SSRF problem thesafeIpcheck was meant to prevent. Build the streaming request against the validated address the same way as the sync path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_http.cpp` around lines 409 - 455, postStream() validates the target with isUrlSafe(url) but then calls cpr::Post using the original url, reintroducing SSRF; change the streaming thread to use the validated safeIp when building the cpr::Url passed to cpr::Post and preserve the original hostname via the Host header so virtual-hosted services still work. Specifically, inside the lambda used for the thread (the one that creates writeCallback and calls cpr::Post), replace usage of cpr::Url{ url } with a URL built from safeIp (including port if present) and ensure headers["Host"] (or "host") is set to the original host extracted from url before handing headers to cpr::Post; update any parsing logic consistent with the sync path (same approach used by the sync request code) and keep StreamSession, writeCallback and cpr::Timeout unchanged.source/lua/lua_script_manager.cpp (1)
763-784:⚠️ Potential issue | 🟠 MajorDisabling a script does not deactivate its existing registrations.
This path flips the enabled flag and overlay booleans, but it never removes that script's
contextMenuItemsoreventListeners. After a disable/enable cycle, those registrations stay live and the re-execution at Lines 780-784 can add another copy on top.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_script_manager.cpp` around lines 763 - 784, When disabling in LuaScriptManager::setScriptEnabled, also remove the script's existing registrations so they don't accumulate; locate the script directory via scripts[index]->getDirectory() (as used for mapOverlays/mapOverlayShows) and then erase any entries in contextMenuItems and eventListeners that belong to that script (match by ownerScriptDir or equivalent owner identifier), taking care to remove while iterating safely (use remove_if/erase or iterate with iterator++ before erase) so you don't invalidate iterators; after cleanup you can flip the enabled flag and when enabling call executeScript(index, error) as before to re-register fresh components.
🧹 Nitpick comments (10)
source/editor/action_queue.h (1)
63-63: Add a direct<string>include for this header.
getActionNameintroducesstd::stringin the public interface, so this header should include<string>explicitly instead of relying on transitive includes.♻️ Suggested patch
`#include` <deque> `#include` <vector> `#include` <memory> +#include <string> `#include` "editor/action.h"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/editor/action_queue.h` at line 63, The header declares getActionName(size_t) returning std::string but doesn't include <string>, so add a direct `#include` <string> to source/editor/action_queue.h to avoid relying on transitive includes and ensure the public interface compiles independently; update includes at the top of the file and verify getActionName and any other std::string usages in this header compile without other headers.source/lua/lua_api_json.h (1)
5-6: Use a project-prefixed include guard for consistency.This header uses
LUA_API_JSON_H, while adjacent Lua headers useRME_*guards. Aligning this guard reduces collision risk and keeps style uniform.Also applies to: 16-16
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_json.h` around lines 5 - 6, The header uses the generic include guard macro LUA_API_JSON_H; change it to the project-prefixed form (e.g., RME_LUA_API_JSON_H) to match adjacent headers and avoid collisions, updating the `#ifndef` and `#define` macros and the trailing `#endif` comment to the new macro; ensure any other occurrences (the guard at the bottom / duplicate on line 16) are also updated to the same RME-prefixed macro so they remain consistent.source/editor/selection.cpp (1)
406-411: EmitselectionChangeonly when selection actually mutates.Right now this fires on any eligible
finish()call, including no-op sessions. Consider tracking “selection changed in session” and emitting only when true to avoid unnecessary script callbacks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/editor/selection.cpp` around lines 406 - 411, The emit currently fires on any eligible finish() call even if the selection didn't change; modify the selection session code so finish() (or the SelectionSession/selection handling code) tracks whether the selection actually mutated (e.g., a boolean like selectionChanged or a method SelectionSession::hasChanged()) and only call g_luaScripts.emit("selectionChange") when that flag is true in addition to the existing thread/flag checks (flags & (INTERNAL | SUBTHREAD)) and g_luaScripts.isInitialized(); update finish() to set/reset the flag appropriately so no-op sessions do not trigger the Lua callback.source/ui/gui.cpp (1)
293-295: EmitbrushChangeonly when the selected brush changed.This reduces redundant script notifications and avoids avoidable event churn.
♻️ Proposed refinement
void GUI::SelectBrushInternal(Brush* brush) { + Brush* previous = GetCurrentBrush(); g_brush_manager.SelectBrushInternal(brush); - if (g_luaScripts.isInitialized() && GetCurrentBrush()) { - g_luaScripts.emit("brushChange", GetCurrentBrush()->getName()); + Brush* current = GetCurrentBrush(); + if (g_luaScripts.isInitialized() && current && current != previous) { + g_luaScripts.emit("brushChange", current->getName()); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/ui/gui.cpp` around lines 293 - 295, The code currently emits "brushChange" on every call when g_luaScripts.isInitialized() and GetCurrentBrush() are true; change this to emit only when the selected brush actually changes by caching the previously-selected brush (e.g., a static or member variable storing the last brush pointer or its name) and comparing it to the current brush returned by GetCurrentBrush() (or its getName()) before calling g_luaScripts.emit("brushChange", ...); update the cache whenever the brush changes (including handling null/unset brush cases) so redundant emits are suppressed.source/CMakeLists.txt (1)
706-747: Header files should be inrme_H, notrme_SRC.The Lua API header files (
.h) are added torme_SRCinstead ofrme_H. While this may not cause build failures, it deviates from the established convention in this CMakeLists.txt where headers are listed inrme_Hand sources inrme_SRC.♻️ Move header files to rme_H
Move lines 706-726 (the
.hfiles) to therme_Hsection earlier in the file, keeping only the.cppfiles inrme_SRC:+# Add to rme_H section (around line 357): + ${CMAKE_CURRENT_LIST_DIR}/lua/lua_api.h + ${CMAKE_CURRENT_LIST_DIR}/lua/lua_api_algo.h + ${CMAKE_CURRENT_LIST_DIR}/lua/lua_api_app.h + ${CMAKE_CURRENT_LIST_DIR}/lua/lua_api_brush.h + ${CMAKE_CURRENT_LIST_DIR}/lua/lua_api_color.h + ${CMAKE_CURRENT_LIST_DIR}/lua/lua_api_creature.h + ${CMAKE_CURRENT_LIST_DIR}/lua/lua_api_geo.h + ${CMAKE_CURRENT_LIST_DIR}/lua/lua_api_http.h + ${CMAKE_CURRENT_LIST_DIR}/lua/lua_api_image.h + ${CMAKE_CURRENT_LIST_DIR}/lua/lua_api_item.h + ${CMAKE_CURRENT_LIST_DIR}/lua/lua_api_json.h + ${CMAKE_CURRENT_LIST_DIR}/lua/lua_api_map.h + ${CMAKE_CURRENT_LIST_DIR}/lua/lua_api_noise.h + ${CMAKE_CURRENT_LIST_DIR}/lua/lua_api_position.h + ${CMAKE_CURRENT_LIST_DIR}/lua/lua_api_selection.h + ${CMAKE_CURRENT_LIST_DIR}/lua/lua_api_tile.h + ${CMAKE_CURRENT_LIST_DIR}/lua/lua_dialog.h + ${CMAKE_CURRENT_LIST_DIR}/lua/lua_engine.h + ${CMAKE_CURRENT_LIST_DIR}/lua/lua_script.h + ${CMAKE_CURRENT_LIST_DIR}/lua/lua_script_manager.h + ${CMAKE_CURRENT_LIST_DIR}/lua/lua_scripts_window.h # Keep only .cpp files in rme_SRC: - ${CMAKE_CURRENT_LIST_DIR}/lua/lua_api.h - ${CMAKE_CURRENT_LIST_DIR}/lua/lua_api_algo.h - ... (remove all .h lines) ${CMAKE_CURRENT_LIST_DIR}/lua/lua_api.cpp ${CMAKE_CURRENT_LIST_DIR}/lua/lua_api_algo.cpp ... (keep all .cpp lines)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/CMakeLists.txt` around lines 706 - 747, The listed Lua header files (e.g., lua/lua_api.h, lua/lua_api_algo.h, lua/lua_api_app.h, lua/lua_api_brush.h, lua/lua_api_color.h, lua/lua_api_creature.h, lua/lua_api_geo.h, lua/lua_api_http.h, lua/lua_api_image.h, lua/lua_api_item.h, lua/lua_api_json.h, lua/lua_api_map.h, lua/lua_api_noise.h, lua/lua_api_position.h, lua/lua_api_selection.h, lua/lua_api_tile.h, lua/lua_dialog.h, lua/lua_engine.h, lua/lua_script.h, lua/lua_script_manager.h, lua/lua_scripts_window.h) were mistakenly added to rme_SRC; move those .h entries from the rme_SRC list into the existing rme_H list (keeping project header ordering consistent), leaving only the .cpp files (lua_api.cpp, lua_api_algo.cpp, ... lua_scripts_window.cpp) in rme_SRC so headers are tracked under rme_H and sources remain in rme_SRC.source/ui/menubar/menubar_action_manager.cpp (1)
174-177: Consider adding icons for consistency.The new script menu actions use
MAKE_ACTIONwithout icons, while most other menu actions useMAKE_ACTION_ICON. Consider adding appropriate icons (e.g.,ICON_CODE,ICON_FOLDER,ICON_SYNC) for visual consistency with the rest of the menu system.♻️ Optional: Add icons for script menu items
- MAKE_ACTION(SCRIPTS_MANAGER, wxITEM_NORMAL, OnScriptsManager); - MAKE_ACTION(SCRIPTS_OPEN_FOLDER, wxITEM_NORMAL, OnScriptsOpenFolder); - MAKE_ACTION(SCRIPTS_RELOAD, wxITEM_NORMAL, OnScriptsReload); + MAKE_ACTION_ICON(SCRIPTS_MANAGER, wxITEM_NORMAL, ICON_CODE, OnScriptsManager); + MAKE_ACTION_ICON(SCRIPTS_OPEN_FOLDER, wxITEM_NORMAL, ICON_FOLDER, OnScriptsOpenFolder); + MAKE_ACTION_ICON(SCRIPTS_RELOAD, wxITEM_NORMAL, ICON_SYNC, OnScriptsReload);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/ui/menubar/menubar_action_manager.cpp` around lines 174 - 177, The new script actions (SCRIPTS_MANAGER, SCRIPTS_OPEN_FOLDER, SCRIPTS_RELOAD) are created with MAKE_ACTION and lack icons; change their creation to use MAKE_ACTION_ICON and supply appropriate icon identifiers (for example replace MAKE_ACTION(SCRIPTS_MANAGER, ...) with MAKE_ACTION_ICON(SCRIPTS_MANAGER, ..., ICON_CODE), MAKE_ACTION_ICON(SCRIPTS_OPEN_FOLDER, ..., ICON_FOLDER), and MAKE_ACTION_ICON(SCRIPTS_RELOAD, ..., ICON_SYNC)) so the script menu matches the rest of the menu system's visual conventions.source/ui/main_menubar.cpp (1)
27-28: Duplicate include.Line 28 duplicates the include of
"map/map_search.h"from line 27.✨ Remove duplicate
`#include` "map/map_statistics.h" `#include` "map/map_search.h" -#include "map/map_search.h" `#include` "ui/managers/recent_files_manager.h"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/ui/main_menubar.cpp` around lines 27 - 28, Remove the redundant include of "map/map_search.h" so the header is only included once; locate the duplicated include lines referencing "map/map_search.h" in source/ui/main_menubar.cpp and delete the second occurrence to avoid duplicate include warnings and keep the file tidy.source/lua/lua_api_creature.cpp (2)
60-63: Inconsistent formatting in lambda bodies.The lambdas have unusual line breaks with closing braces on separate lines. Consider formatting consistently with the rest of the file.
✨ Suggested formatting
-"select", [](Creature* c) { if (c){ c->select(); -} }, -"deselect", [](Creature* c) { if (c){ c->deselect(); -} }, +"select", [](Creature* c) { if (c) { c->select(); } }, +"deselect", [](Creature* c) { if (c) { c->deselect(); } },Also applies to: 99-102
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_creature.cpp` around lines 60 - 63, Fix inconsistent lambda formatting in the registration entries for the "select" and "deselect" bindings (and the similar ones around lines 99-102) by normalizing their lambda bodies to match the project's style: put the conditional and call on a single line inside the lambda (e.g., [](Creature* c) { if (c) c->select(); } ) and ensure opening and closing braces are placed consistently with other bindings in lua_api_creature.cpp so all lambda expressions use the same single-line or block style as the surrounding code.
49-52: Consider validatingspawnTimebounds.The
spawnTimesetter accepts any integer value, including negative values. While the getter returns 0 for null creatures, negative spawn times may cause unexpected behavior in downstream systems.🛡️ Proposed validation
"spawnTime", sol::property([](Creature* c) -> int { return c ? c->getSpawnTime() : 0; }, [](Creature* c, int time) { - if (c) { + if (c && time >= 0) { c->setSpawnTime(time); } }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_creature.cpp` around lines 49 - 52, The spawnTime property setter currently accepts any integer (defined in the sol::property lambda around "spawnTime") which allows negative values; update the setter lambda used for "spawnTime" to validate and clamp the input before calling Creature::setSpawnTime (or reject invalid values), e.g. ensure time >= 0 (or enforce a defined min/max) and only call c->setSpawnTime(time) with the validated value; include reference to the sol::property lambda for "spawnTime" and Creature::setSpawnTime when making the change.scripts/hello_world/hello_world.lua (1)
24-24: Hardcoded IP address in example script.The endpoint uses a hardcoded IP
68.71.131.28. While the comment explains this is for plain HTTP demo purposes, consider using a domain name or making this configurable for maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/hello_world/hello_world.lua` at line 24, The QUOTE_ENDPOINT variable in hello_world.lua is hardcoded to an IP (local demo); change it to a configurable value by replacing the literal "http://68.71.131.28/api/fortune" with a domain or an environment/config override (e.g., read from an env var or a config table) so callers can set the endpoint without editing the script; update the QUOTE_ENDPOINT variable initialization (and any related comment) to use the env/config lookup with a sensible default domain-based URL.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4c73c109-948d-4ad1-81cc-3c13df7b3e6b
📒 Files selected for processing (93)
CMakeLists.txtbuild_ninja.batdata/menubar.xmlscripts/README.mdscripts/fps_counter.luascripts/hello_world/hello_world.luascripts/hello_world/manifest.luascripts/linter.luascripts/terrain_generator_demo.luasource/CMakeLists.txtsource/app/application.cppsource/app/preferences/preferences_layout.cppsource/brushes/brush.hsource/editor/action.cppsource/editor/action.hsource/editor/action_queue.cppsource/editor/action_queue.hsource/editor/editor.hsource/editor/persistence/editor_persistence.cppsource/editor/selection.cppsource/ext/fast_noise_lite.hsource/game/creatures.hsource/game/items.hsource/io/filehandle.hsource/lua/lua_api.cppsource/lua/lua_api.hsource/lua/lua_api_algo.cppsource/lua/lua_api_algo.hsource/lua/lua_api_app.cppsource/lua/lua_api_app.hsource/lua/lua_api_brush.cppsource/lua/lua_api_brush.hsource/lua/lua_api_color.cppsource/lua/lua_api_color.hsource/lua/lua_api_creature.cppsource/lua/lua_api_creature.hsource/lua/lua_api_geo.cppsource/lua/lua_api_geo.hsource/lua/lua_api_http.cppsource/lua/lua_api_http.hsource/lua/lua_api_image.cppsource/lua/lua_api_image.hsource/lua/lua_api_item.cppsource/lua/lua_api_item.hsource/lua/lua_api_json.cppsource/lua/lua_api_json.hsource/lua/lua_api_map.cppsource/lua/lua_api_map.hsource/lua/lua_api_noise.cppsource/lua/lua_api_noise.hsource/lua/lua_api_position.cppsource/lua/lua_api_position.hsource/lua/lua_api_selection.cppsource/lua/lua_api_selection.hsource/lua/lua_api_tile.cppsource/lua/lua_api_tile.hsource/lua/lua_dialog.cppsource/lua/lua_dialog.hsource/lua/lua_engine.cppsource/lua/lua_engine.hsource/lua/lua_script.cppsource/lua/lua_script.hsource/lua/lua_script_manager.cppsource/lua/lua_script_manager.hsource/lua/lua_scripts_window.cppsource/lua/lua_scripts_window.hsource/lua/lua_sol_config.hsource/map/tile.cppsource/map/tile.hsource/rendering/core/coordinate_mapper.cppsource/rendering/core/coordinate_mapper.hsource/rendering/core/map_overlay.hsource/rendering/drawers/overlays/lua_overlay_drawer.cppsource/rendering/drawers/overlays/lua_overlay_drawer.hsource/rendering/map_drawer.cppsource/rendering/map_drawer.hsource/rendering/ui/map_display.cppsource/rendering/ui/map_display.hsource/rendering/ui/map_menu_handler.cppsource/rendering/ui/map_menu_handler.hsource/ui/common_windows.hsource/ui/gui.cppsource/ui/gui_ids.hsource/ui/main_frame.cppsource/ui/main_menubar.cppsource/ui/main_menubar.hsource/ui/map_popup_menu.cppsource/ui/menubar/menubar_action_manager.cppsource/ui/menubar/script_menu_handler.cppsource/ui/menubar/script_menu_handler.hsource/ui/menubar_loader.cppsource/ui/menubar_loader.hvcpkg.json
💤 Files with no reviewable changes (1)
- build_ninja.bat
| <menu name="Scripts" special="SCRIPTS"> | ||
| <item name="Script Manager..." action="SCRIPTS_MANAGER" help="Show/hide the Script Manager window."/> | ||
| <separator/> | ||
| <item name="Open Scripts Folder" action="SCRIPTS_OPEN_FOLDER" help="Open the scripts folder in file explorer."/> | ||
| <item name="Reload Scripts" hotkey="Ctrl+Shift+F5" action="SCRIPTS_RELOAD" help="Reload all scripts from disk."/> | ||
| <separator/> | ||
| <!-- Scripts are loaded dynamically below this point --> | ||
| </menu> |
There was a problem hiding this comment.
Avoid a permanent trailing separator in the Scripts menu.
Line 246 is static, so an empty or failed Lua setup still leaves a dangling divider at the bottom of the menu. Move that separator into the runtime population path so it is only added when at least one dynamic script entry is appended.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@data/menubar.xml` around lines 241 - 248, Remove the static trailing
<separator/> from the "Scripts" menu in menubar.xml (the <menu name="Scripts"
special="SCRIPTS"> block that contains the SCRIPTS_MANAGER, SCRIPTS_OPEN_FOLDER
and SCRIPTS_RELOAD items) and instead add that separator in the runtime code
that appends dynamic script entries so the divider is only inserted when at
least one dynamic script is added; update the script-population routine (the
code path that creates/attaches dynamic script items into the "Scripts"/SCRIPTS
menu) to append a separator before the dynamic entries only when the dynamic
list is non-empty.
| | Property/Function | Type | Description | | ||
| | :--- | :--- | :--- | | ||
| | `app.version` | string | The current RME version. | | ||
| | `app.apiVersion` | number | The API version number (currently 2). | | ||
| | `app.map` | [Map](#map) | Returns the currently active Map object (or nil). | |
There was a problem hiding this comment.
Align app.apiVersion docs with the shipped value.
The docs say the current API version is 2, but registerApp() currently exports app["apiVersion"] = 1. Please update one side or the other before release.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/README.md` around lines 124 - 128, The docs and implementation
disagree on app.apiVersion; update either the README table or the export in
registerApp so they match: locate the registerApp function where
app["apiVersion"] is set (currently to 1) and change it to the documented value
(2) OR update the README entry for `app.apiVersion` to reflect the shipped value
(1); ensure the unique symbol app["apiVersion"] in registerApp and the README
table row for `app.apiVersion` are kept consistent.
…ng, and tile handling Major Lua scripting overhaul: added SCRIPT_ID context tracking with ScriptContextGuard for proper script isolation, enhanced security by removing io library and validating HTTP URLs with IP pinning, improved tile change tracking with position caching in Change class and ownedLocation in Tile for reliable undo/redo, added cycle detection to json.encode, optimized overlay rendering with view caching, fixed integer overflows in geo/noise APIs, and refined selection/brush change events to fire only on actual state changes.
|
🤖 Hi @karolak6612, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
…tern - Added ScopedInternalSelectionSession RAII helper to automate selection session lifecycle in lua_api_selection.cpp - Exposed start() and finish() methods to Lua API with SessionFlags overload support - Refactored transaction/clear/add/remove methods to use withInternalSelectionSession helper, eliminating repetitive guard code - Updated Selection::clear() to process pending_adds/pending_removes queues when deferred mode is active - Added isDeferred() getter to Selection header for Lua API finish() logic
|
🤖 Hi @karolak6612, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
📋 Review Summary
This Pull Request introduces full Lua scripting support to Remere's Map Editor Redux. The implementation is comprehensive, covering map operations, tile modifications, item handling, and even advanced features like UI dialogs, HTTP requests, and map overlays.
While the overall architecture is sound and integrates well with the existing RME codebase, there is a critical issue in the transaction system regarding how tiles are snapshotted for undo/redo. The current use of deepCopy() during commit/rollback causes all existing Lua pointers to those tiles to become invalid, which could lead to crashes (use-after-free) if scripts persist references between transactions.
🔍 General Feedback
- Scripting API: The API surface is very rich and well-documented in the new
README.md. - Security: The
app.storagesystem includes good path validation to prevent scripts from accessing files outside allowed directories. - Integration: The Script Manager UI and the dynamic menu integration are polished and feel native to the editor.
- Testing: The added demo scripts (FPS, Hello World, Terrain Generator) are excellent examples for users.
- UI Blocking: Minor concern regarding
app.sleepwhich can freeze the main thread; suggested usingwxMilliSleepor documenting the limitation clearly.
Please address the critical memory safety issue in LuaTransaction::commit() before merging.
| Tile* modifiedTile = editor->getMap()->getTile(pos); | ||
|
|
||
| std::unique_ptr<Tile> modifiedCopy = modifiedTile ? modifiedTile->deepCopy() : std::unique_ptr<Tile>(); | ||
|
|
||
| // Swap the original back into the map, or remove the tile entirely if it did not exist before | ||
| std::unique_ptr<Tile> swappedOut = originalTile ? editor->getMap()->swapTile(pos, std::move(originalTile)) : editor->getMap()->swapTile(pos, std::unique_ptr<Tile>()); | ||
|
|
||
| // Swapped-out state should be cleaned up from metadata and selection | ||
| if (swappedOut) { | ||
| updateTileMetadata(editor, swappedOut.get(), false); | ||
| } | ||
|
|
||
| // Add the current map tile back to metadata if one now exists | ||
| Tile* tileInMap = editor->getMap()->getTile(pos); | ||
| if (tileInMap) { | ||
| updateTileMetadata(editor, tileInMap, true); | ||
| } | ||
|
|
||
| // Create Change with the modified copy (or a null sentinel for deletions) | ||
| action->addChange(std::make_unique<Change>(std::move(modifiedCopy), pos)); |
There was a problem hiding this comment.
🔴 Critical: Potential Use-After-Free and Dangling Pointers
The current implementation of commit() creates a deepCopy() of the modified tile and swaps the original tile back into the map. This causes the following issues:
- The original modified
Tileobject (which Lua scripts may still hold a pointer to) is deleted whenswappedOutgoes out of scope. - The map ends up with a new
Tileobject (the copy), making any existingTile*references in Lua scripts dangling pointers.
Instead of deep copying, you should move the actual modified tile object into the Change list. Since action->commit() will swap it back into the map immediately after, the pointers will remain valid and point to the tile now residing in the map.
| Tile* modifiedTile = editor->getMap()->getTile(pos); | |
| std::unique_ptr<Tile> modifiedCopy = modifiedTile ? modifiedTile->deepCopy() : std::unique_ptr<Tile>(); | |
| // Swap the original back into the map, or remove the tile entirely if it did not exist before | |
| std::unique_ptr<Tile> swappedOut = originalTile ? editor->getMap()->swapTile(pos, std::move(originalTile)) : editor->getMap()->swapTile(pos, std::unique_ptr<Tile>()); | |
| // Swapped-out state should be cleaned up from metadata and selection | |
| if (swappedOut) { | |
| updateTileMetadata(editor, swappedOut.get(), false); | |
| } | |
| // Add the current map tile back to metadata if one now exists | |
| Tile* tileInMap = editor->getMap()->getTile(pos); | |
| if (tileInMap) { | |
| updateTileMetadata(editor, tileInMap, true); | |
| } | |
| // Create Change with the modified copy (or a null sentinel for deletions) | |
| action->addChange(std::make_unique<Change>(std::move(modifiedCopy), pos)); | |
| // Get the current (modified) tile from the map | |
| Tile* modifiedTile = editor->getMap()->getTile(pos); | |
| // Sync metadata for the modified tile being moved out of the map | |
| if (modifiedTile) { | |
| updateTileMetadata(editor, modifiedTile, false); | |
| } | |
| // Swap the original tile back into the map | |
| std::unique_ptr<Tile> modifiedTileUptr = originalTile ? | |
| editor->getMap()->swapTile(pos, std::move(originalTile)) : | |
| editor->getMap()->swapTile(pos, std::unique_ptr<Tile>()); | |
| // Sync metadata for the original tile now back in the map | |
| Tile* tileInMap = editor->getMap()->getTile(pos); | |
| if (tileInMap) { | |
| updateTileMetadata(editor, tileInMap, true); | |
| } | |
| // Create Change with the actual modified tile object | |
| action->addChange(std::make_unique<Change>(std::move(modifiedTileUptr), pos)); |
| app["sleep"] = [](int milliseconds) { | ||
| if (milliseconds > 0 && milliseconds <= 10000) { // Max 10 seconds | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(milliseconds)); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🟡 Medium: UI Thread Blocking
Calling std::this_thread::sleep_for in the main GUI thread will freeze the whole editor's UI. This is generally discouraged.
| app["sleep"] = [](int milliseconds) { | |
| if (milliseconds > 0 && milliseconds <= 10000) { // Max 10 seconds | |
| std::this_thread::sleep_for(std::chrono::milliseconds(milliseconds)); | |
| } | |
| }; | |
| // Sleep for a given number of milliseconds (use sparingly, blocks the UI) | |
| app["sleep"] = [](int milliseconds) { | |
| if (milliseconds > 0 && milliseconds <= 10000) { // Max 10 seconds | |
| wxMilliSleep(milliseconds); | |
| } | |
| }; |
| editor->getMap()->doChange(); | ||
| g_gui.RefreshView(); // Force redraw immediately |
There was a problem hiding this comment.
🟢 Low: Redundant Map Update
addAndCommitAction already commits the action, which in RME Redux usually already calls doChange() and triggers map updates. These calls might be redundant.
| editor->getMap()->doChange(); | |
| g_gui.RefreshView(); // Force redraw immediately | |
| if (action->size() > 0) { | |
| batch->addAndCommitAction(std::move(action)); | |
| editor->addBatch(std::move(batch)); | |
| g_gui.RefreshView(); // Force redraw immediately | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
source/editor/persistence/editor_persistence.cpp (1)
338-357:⚠️ Potential issue | 🟠 MajorRestore the identity mapping for SMART_MERGE inserts.
When
current_house == nullptr, this branch now leaveshouse_id_mapempty for that house.importMap()later only sends moved house tiles through the existingremoveTile()/addTile()bookkeeping when that lookup succeeds, so newly inserted smart-merge houses now skip the house reattachment path during offset moves.🧩 Suggested fix
case IMPORT_SMART_MERGE: { if (current_house) { // Compare and insert/merge depending on parameters if (current_house->name == imported_house->name && current_house->townid == imported_house->townid) { // Just add to map house_id_map[imported_house->getID()] = current_house->getID(); skip = true; Position newexit = oldexit + offset; if (newexit.isValid()) { current_house->setExit(&editor.map, newexit); } } else { // Conflict! Find a newd id and replace old uint32_t new_id = editor.map.houses.getEmptyID(); house_id_map[imported_house->getID()] = new_id; imported_house->setID(new_id); } } else { + house_id_map[imported_house->getID()] = imported_house->getID(); } break; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/editor/persistence/editor_persistence.cpp` around lines 338 - 357, The IMPORT_SMART_MERGE branch fails to populate house_id_map when current_house is null, so newly inserted smart-merge houses are not tracked during importMap() and thus skip removeTile()/addTile() reattachment on offset moves; fix by ensuring that in the else branch for IMPORT_SMART_MERGE you add an identity mapping (house_id_map[imported_house->getID()] = imported_house->getID() or assign a new ID if you call imported_house->setID) so that importMap() can find the mapping; update the else block in editor_persistence.cpp where current_house is checked (symbols: IMPORT_SMART_MERGE, current_house, house_id_map, imported_house, imported_house->getID(), imported_house->setID(), importMap(), removeTile()/addTile()) to insert the appropriate mapping logic.source/editor/selection.cpp (1)
267-333:⚠️ Potential issue | 🟠 MajorTrack any selection mutation, not just tile membership.
selectionChangedonly flips whentilesgains/loses entries,flush()changes the tile vector, orclear()runs. Selecting or deselecting anItem,Spawn, orCreatureon a tile that stays selected still changes the selection state, but it leavestilesunchanged, sofinish()suppressesselectionChangeand Lua observers see stale selection data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/editor/selection.cpp` around lines 267 - 333, The selection state only flips when tile membership changes (see Selection::addInternal, Selection::removeInternal, Selection::flush), but mutations of tile contents (items/creatures/spawns) on already-selected tiles are not tracked; add a small API on Selection (e.g., Selection::markChanged() or Selection::markDirtySelection()) that sets bounds_dirty = true and selectionChanged = true and call it from all tile-content mutation sites (Tile methods that add/remove Item, Spawn, Creature, and any other code paths that mutate selected tile contents) so any content-change on a selected Tile causes observers to see a selection update.source/editor/action.cpp (1)
89-93:⚠️ Potential issue | 🔴 CriticalGuard null tile sentinels in
Change::memsize().
Changecan now legitimately hold a nullstd::unique_ptr<Tile>for tile deletions. This branch still dereferences it, so exact history-size calculation will crash on delete actions.🐛 Possible fix
uint32_t Change::memsize() const { uint32_t mem = sizeof(*this); if (auto* t = std::get_if<std::unique_ptr<Tile>>(&data)) { - mem += (*t)->memsize(); + if (*t) { + mem += (*t)->memsize(); + } } else if (auto* wp = std::get_if<WaypointChangeData>(&data)) { mem += wp->name.capacity(); } else if (auto* house = std::get_if<HouseExitChangeData>(&data)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/editor/action.cpp` around lines 89 - 93, The Change::memsize() implementation dereferences the std::unique_ptr<Tile> inside the variant unconditionally; modify the branch that handles std::unique_ptr<Tile> (the std::get_if<std::unique_ptr<Tile>>(&data) path) to guard against a null pointer before calling (*t)->memsize(): if (*t) is non-null, add (*t)->memsize(), otherwise skip that call (or add whatever fixed cost you want for a deleted tile). Keep the rest of the variant branches unchanged.
♻️ Duplicate comments (13)
source/rendering/ui/map_display.cpp (1)
546-554:⚠️ Potential issue | 🟡 MinorRefresh
last_click_xin the properties-click path.
last_click_abs_xis still built from stale state here, so the first properties click afterReset()uses-1 + start_x, and later clicks can reuse the previous action-click x.🛠️ Suggested fix
selection_controller->HandlePropertiesClick(Position(mouse_map_x, mouse_map_y, floor), event.ShiftDown(), event.ControlDown(), event.AltDown()); + last_click_x = int(event.GetX() * zoom); last_click_y = int(event.GetY() * zoom);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/ui/map_display.cpp` around lines 546 - 554, The properties-click path uses stale last_click_x so the first click after Reset() yields wrong absolute X; update last_click_x from the mouse event before using it in HandlePropertiesClick/abs computation. Specifically, set last_click_x = int(event.GetX() * zoom) (analogous to the existing last_click_y assignment) prior to calling selection_controller->HandlePropertiesClick or before computing last_click_abs_x, so HandlePropertiesClick and the subsequent last_click_abs_x = last_click_x + start_x use the refreshed value; ensure Reset() semantics remain intact.scripts/linter.lua (1)
145-165:⚠️ Potential issue | 🟡 MinorSync the overlay stub with the bundled
fps_counter.luausage.The shipped script calls
app.getTime(),app.mapView.addOverlay(...),app.mapView.registerShow(...),ctx.rect(...),ctx.text(...), and readsctx.view.screenWidth/screenHeight. This stub still omitsgetTime, types those callbacks asselfmethods, and hides the screen-size fields, so the example won’t lint/autocomplete cleanly out of the box.🛠️ Suggested fix
---@class App ---@field version string ---@field map Map|nil @@ ---@field storage fun(name: string): ScriptStorage +---@field getTime fun(): integer ---@field yield fun() Yields to process pending UI events (prevents freeze during long operations) ---@field sleep fun(milliseconds: number) Sleeps for the given milliseconds (max 10000). Blocks UI. app = {} @@ ---@class MapView ----@field addOverlay fun(self: MapView, id: string, options: MapOverlayOptions): boolean ----@field removeOverlay fun(self: MapView, id: string): boolean ----@field setEnabled fun(self: MapView, id: string, enabled: boolean): boolean ----@field registerShow fun(self: MapView, label: string, id: string, options?: MapOverlayShowOptions): boolean +---@field addOverlay fun(id: string, options: MapOverlayOptions): boolean +---@field removeOverlay fun(id: string): boolean +---@field setEnabled fun(id: string, enabled: boolean): boolean +---@field registerShow fun(label: string, id: string, options?: MapOverlayShowOptions): boolean @@ ---@class MapOverlayContext ----@field view {x1: number, y1: number, x2: number, y2: number, z: number, zoom: number} ----@field rect fun(self: MapOverlayContext, opts: MapOverlayRectOptions) ----@field line fun(self: MapOverlayContext, opts: MapOverlayLineOptions) ----@field text fun(self: MapOverlayContext, opts: MapOverlayTextOptions) +---@field view {x1: number, y1: number, x2: number, y2: number, z: number, zoom: number, screenWidth: number, screenHeight: number} +---@field rect fun(opts: MapOverlayRectOptions) +---@field line fun(opts: MapOverlayLineOptions) +---@field text fun(opts: MapOverlayTextOptions)Quick check: open
scripts/fps_counter.luaagainst this stub in LuaLS after the change and the overlay-related diagnostics should disappear.Also applies to: 313-330
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/linter.lua` around lines 145 - 165, The App stub is missing getTime, incorrectly types overlay callbacks as methods (using self), and omits ctx.view.screenWidth/screenHeight plus drawing functions used by fps_counter.lua; update the stub to add a getTime() function, declare mapView.addOverlay and mapView.registerShow callbacks as plain functions (not methods) that accept a ctx/table argument, and include ctx.rect, ctx.text and ctx.view.screenWidth and ctx.view.screenHeight fields so the fps_counter overlay code lints correctly (refer to symbols: getTime, mapView.addOverlay, mapView.registerShow, ctx.rect, ctx.text, ctx.view.screenWidth, ctx.view.screenHeight).scripts/hello_world/hello_world.lua (1)
382-394:⚠️ Potential issue | 🟡 MinorGuard
app.selectionbefore accessingisEmpty.The condition
app.selection.isEmptywill error ifapp.selectionisnil. The "Inspect Selection" button at lines 361-367 correctly guards withif not sel or sel.isEmpty, but this button does not.🐛 Proposed fix
onclick = function() - if not app.map or app.selection.isEmpty then + local sel = app.selection + if not app.map or not sel or sel.isEmpty then app.alert("Select map area first!") return end app.transaction("Add Bushes", function() - for _, tile in ipairs(app.selection.tiles) do + for _, tile in ipairs(sel.tiles) do tile:addItem(2785, 1) end end) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/hello_world/hello_world.lua` around lines 382 - 394, The onclick handler currently checks app.selection.isEmpty without ensuring app.selection exists, which can error; update the onclick function to guard app.selection (same pattern as the Inspect Selection button) by checking if not app.selection or app.selection.isEmpty before calling app.alert and returning, then proceed to call app.transaction("Add Bushes", ...) that iterates app.selection.tiles and calls tile:addItem(2785, 1).source/rendering/ui/map_display.h (1)
51-51:⚠️ Potential issue | 🔴 CriticalThe weaker parent type still needs a checked
GetMapWindow()path.Changing the ctor to
wxWindow*removes the compile-time guarantee that the parent is aMapWindow, but this class still exposesGetMapWindow() const. Unless the implementation now does a runtime-checked cast and every caller handles failure, the new preview/dialog parents can still turn later uses into UB.#!/bin/bash fd map_display.cpp source --exec sed -n '1,260p' {} rg -nP --type=cpp --type=h -C2 '\bGetMapWindow\s*\(' source rg -nP --type=cpp --type=h -C2 '\bMapCanvas\s*\(' source🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/ui/map_display.h` at line 51, The MapCanvas constructor was weakened to take wxWindow* but GetMapWindow() still assumes a MapWindow parent; update the code to avoid UB by changing GetMapWindow() to perform a runtime-checked cast (e.g., use dynamic_cast<MapWindow*>(GetParent()) or wxDynamicCast(GetParent(), MapWindow)) and return nullptr on failure (or optionally assert/throw with a clear message), and then update any callers of GetMapWindow() to handle a null result safely (or revert the MapCanvas ctor to accept MapWindow* if you require a compile-time guarantee). Ensure the symbols to edit are MapCanvas::MapCanvas(...) and MapCanvas::GetMapWindow().source/lua/lua_api_map.cpp (1)
32-74:⚠️ Potential issue | 🟠 MajorThe iterator validity check is still only raw-pointer equality.
These closures snapshot positions, but both
next()implementations still consider the iterator valid wheneverg_gui.GetCurrentEditor()->getMap() == map. After a close/reopen, address reuse can make that pass for a differentMap, andmap->getTile(pos)will then walk stale iteration state. Guard this with a stable owner/generation check before dereferencing.Also applies to: 78-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_map.cpp` around lines 32 - 74, The iterator currently only checks raw pointer equality (LuaMapTileIterator::next uses currentEditor->getMap() == map) which can falsely succeed after map address reuse; fix by snapshotting a stable map identity in the constructor (e.g., store mapGeneration or mapId as a new member set from map->getGeneration() or map->getId() inside LuaMapTileIterator(Map*)), then in next() validate both that currentEditor->getMap() == map and that the stored generation/id still matches map->getGeneration()/getId() before dereferencing map or calling map->getTile(pos); if Map lacks a stable id/generation, add one (e.g., Map::getGeneration()/getId()) and bump it on reopen/replace so the iterator can reliably detect stale maps.scripts/terrain_generator_demo.lua (2)
515-515:⚠️ Potential issue | 🟠 MajorPick one source of truth for generator seeds.
These controls are initialized from
config.seed, but the Generate handlers persist intoconfig.island.seed/config.cave.seed/config.dungeon.seed, while preview doesn't persist its edited value at all. Reopening the dialog can still show one seed and run another.Also applies to: 542-543, 568-568, 585-585, 606-606, 622-622, 650-650, 671-671, 688-688, 703-703, 712-712, 736-736
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/terrain_generator_demo.lua` at line 515, The seed controls currently read from config.seed but the Generate handlers write to per-generator fields (config.island.seed, config.cave.seed, config.dungeon.seed) and the preview edits aren't persisted, causing mismatch; pick a single source of truth (either keep a single config.seed or switch all controls/handlers/previews to use the per-generator fields) and make them consistent: update each dlg:number (e.g., id="island_seed", "cave_seed", "dungeon_seed", etc.) to initialize from and write back to the chosen config field, change the Generate handlers (the functions that assign to config.island.seed / config.cave.seed / config.dungeon.seed) to use the same field the control reads/writes, and ensure preview edits persist by updating the same config field when the preview value changes; apply this same change to all listed occurrences so the dialog shows and runs the same seed.
179-180:⚠️ Potential issue | 🟠 MajorUse a local seeded PRNG instead of Lua's shared global RNG.
Tree placement and the Randomize button read from
math.random(), while river generation callsmath.randomseed(seed)and mutates that shared state. The same seed still won't reliably reproduce the same map, and running the river generator changes later randomness.Also applies to: 308-315, 729-729
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/terrain_generator_demo.lua` around lines 179 - 180, Replace uses of the global math.random() in the tree placement block (the "canPlaceTree and math.random() < cfg.treeDensity" check), in the Randomize button handler, and in other spots referenced (around the river generator and lines noted) with a local, seeded PRNG instance so river generation’s math.randomseed(seed) no longer mutates shared state; create a local RNG (e.g., rng) seeded from the map seed at the start of generation and use rng:random() (or the project's equivalent RNG call) for tree placement, river logic, and any UI Randomize logic so all randomness is reproducible and isolated from the global math.random state.source/lua/lua_api_app.cpp (1)
508-516:⚠️ Potential issue | 🟠 MajorCap
storage.load()before buffering the whole file.
app.storage()is config-oriented, but this path still streams any allowed file into memory with no size check. Pointing it at a large asset or data file can stall the UI or exhaust memory from script code.🛡️ Possible fix
storage["load"] = [path](sol::this_state ts2, sol::object) -> sol::object { sol::state_view lua(ts2); + std::error_code ec; + const auto size = std::filesystem::file_size(path, ec); + if (ec || size > 1024 * 1024) { + return sol::make_object(lua, sol::nil); + } std::ifstream file(path); if (!file.is_open()) { return sol::make_object(lua, sol::nil); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_app.cpp` around lines 508 - 516, The storage["load"] lambda currently reads the entire file into a stringstream which can OOM or block the UI; before buffering, check the file size (e.g., using std::filesystem::file_size or std::ifstream::seekg/tellg) against a hard limit or configurable MAX_ALLOWED_SIZE and return sol::nil (or an error) if it exceeds the cap, and when allowed only read up to that limit (or reserve the target string capacity) to avoid unbounded buffering; update the lambda (storage["load"]) to perform this size check and bounded read logic for the given path variable.source/lua/lua_api_http.cpp (2)
163-172:⚠️ Potential issue | 🟠 MajorKeep
hasDatainside the same locked snapshot.
httpStreamStatus()snapshots finished/error/status/headers, then askssession->hasChunks()separately. That reintroduces the mixed states the snapshot work was supposed to remove. IncludehasDatainSnapshotand populate it under the same session lock.Suggested fix
struct Snapshot { std::string data; bool finished; bool hasError; + bool hasData; std::string error; int status; cpr::Header headers; }; @@ Snapshot getStatus() { std::lock_guard<std::mutex> lock(mutex_); Snapshot s; s.finished = finished_; s.hasError = hasError_; + s.hasData = !chunks_.empty(); s.error = errorMessage_; s.status = statusCode_; s.headers = responseHeaders_; return s; } @@ - result["hasData"] = session->hasChunks(); + result["hasData"] = s.hasData;Also applies to: 654-660
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_http.cpp` around lines 163 - 172, Snapshot currently omits hasData and httpStreamStatus() calls session->hasChunks() outside the session lock, reintroducing race conditions; add a hasData field to the Snapshot struct and set it inside the same locked getStatus() method (alongside finished_, hasError_, errorMessage_, statusCode_, responseHeaders_) by calling session->hasChunks() (or equivalent) while holding mutex_, then update httpStreamStatus() and any other callers (e.g., the other snapshot site around getStatus usages) to read hasData from the Snapshot instead of calling session->hasChunks() separately.
303-314:⚠️ Potential issue | 🔴 CriticalPreserve the original scheme in synchronous HTTP calls.
These requests still rebuild the target as
"http://" + safeIp + path, sohttps://URLs are silently downgraded. The manual split also mishandles query-only URLs and never brackets IPv6 literals. ReuseparseUrlParts()/buildPinnedUrl()here, like the streaming path already does.Suggested fix
- size_t hostStart = url.find("://"); - if (hostStart == std::string::npos) hostStart = 0; else hostStart += 3; - size_t pathStart = url.find("/", hostStart); - std::string host = (pathStart == std::string::npos) ? url.substr(hostStart) : url.substr(hostStart, pathStart - hostStart); - std::string path = (pathStart == std::string::npos) ? "" : url.substr(pathStart); + UrlParts parts = parseUrlParts(url); @@ - cpr::Response response = cpr::Get( - cpr::Url { "http://" + safeIp + path }, - headers, - cpr::Header { { "Host", host } }, - cpr::Timeout { 10000 } - ); + cpr::Response response = cpr::Get( + buildPinnedUrl(safeIp, parts), + headers, + cpr::Header { { "Host", parts.authority } }, + cpr::Timeout { 10000 } + ); @@ - size_t hostStart = url.find("://"); - if (hostStart == std::string::npos) hostStart = 0; else hostStart += 3; - size_t pathStart = url.find("/", hostStart); - std::string host = (pathStart == std::string::npos) ? url.substr(hostStart) : url.substr(hostStart, pathStart - hostStart); - std::string path = (pathStart == std::string::npos) ? "" : url.substr(pathStart); + UrlParts parts = parseUrlParts(url); @@ - cpr::Response response = cpr::Post( - cpr::Url { "http://" + safeIp + path }, - cpr::Body { body }, - headers, - cpr::Header { { "Host", host } }, - cpr::Timeout { 10000 } - ); + cpr::Response response = cpr::Post( + buildPinnedUrl(safeIp, parts), + cpr::Body { body }, + headers, + cpr::Header { { "Host", parts.authority } }, + cpr::Timeout { 10000 } + );Also applies to: 352-364
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_http.cpp` around lines 303 - 314, The sync GET call is reconstructing the URL as "http://"+safeIp+path which downgrades https, mishandles query-only URLs and doesn't bracket IPv6; replace the manual parsing/host/path logic and the cpr::Get Url construction with the same helpers used by the streaming path: call parseUrlParts(...) to get scheme, host, port, path/query and then use buildPinnedUrl(...) (or the project's equivalent) to produce the pinned URL (which handles IPv6 brackets and preserves the original scheme), and pass that pinned URL into cpr::Get along with the existing headers/Host header and timeout; update both occurrences (the shown block and the similar block at lines ~352-364) to use parseUrlParts/buildPinnedUrl instead of manual string manipulation.source/lua/lua_script.cpp (1)
137-145:⚠️ Potential issue | 🟠 Major
manifest.luavalues still have to start on the same line as=.Both branches stop at
nextLine, so valid Lua likemain =\n "tool.lua"orautorun =\n trueis still parsed as missing and falls back to defaults. After finding=, keep scanning through whitespace/newlines before reading the value token.Also applies to: 174-186
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_script.cpp` around lines 137 - 145, The parser currently requires the value to start on the same line as '=' by comparing quotePos and nextLine (using eqPos and nextLine), so inputs like `main =\n "tool.lua"` are treated as missing; instead, after locating '=' (eqPos) advance a scan index past any whitespace and newlines to the first non-space character, then treat that position as the start of the value (check for quote or token) when computing quotePos/valStart/valEnd; apply the same change to the other parsing branch around the code referenced (lines using eqPos, nextLine, quotePos, valStart, valEnd) so values that begin on the next line are accepted.source/lua/lua_api_tile.cpp (2)
236-254:⚠️ Potential issue | 🟠 MajorReject non-ground and out-of-range values in
ground.This setter still accepts any positive integer or
Item*and stores it directly intile->ground. That can leave malformed tiles, and values aboveUINT16_MAXwill still narrow on the cast. Please validate the id range and the item type before mutating, and throw on unsupported values instead of silently accepting them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_tile.cpp` around lines 236 - 254, The ground setter currently accepts any positive int or Item* and narrows values, so update the groundObj handling to validate inputs and throw on bad values: for groundObj.is<int>(), check the int is >=1 and <= UINT16_MAX before calling Item::Create(static_cast<uint16_t>(...)); otherwise throw a Lua error; for groundObj.is<Item*>(), verify the Item* is non-null and is actually a ground item (e.g. using the codebase's ground-type check such as item->isGround() or item->getCategory() == ITEM_GROUND) before assigning tile->ground = item->deepCopy(), otherwise throw; keep the tile->ground.reset() branch only for sol::nil_t and call tile->modify() after successful mutation.
337-347:⚠️ Potential issue | 🟠 MajorReturn
Positionfrom tile coordinate accessors.
positionandgetPosition()still build plain Lua tables, so they won't bind to APIs expecting the registeredPositionusertype, e.g.Map:getTile(...)/getOrCreateTile(...). ReturnPositiondirectly here.Suggested fix
- "position", sol::property([](Tile* tile, sol::this_state ts) { - sol::state_view lua(ts); - sol::table t = lua.create_table(); - if (tile) { - Position p = tile->getPosition(); - t["x"] = p.x; - t["y"] = p.y; - t["z"] = p.z; - } - return t; - }), + "position", sol::property([](Tile* tile) -> Position { + return tile ? tile->getPosition() : Position(); + }), @@ - "getPosition", [](Tile* tile, sol::this_state ts) { - sol::state_view lua(ts); - sol::table t = lua.create_table(); - if (tile) { - Position p = tile->getPosition(); - t["x"] = p.x; - t["y"] = p.y; - t["z"] = p.z; - } - return t; }, + "getPosition", [](Tile* tile) -> Position { + return tile ? tile->getPosition() : Position(); + },Also applies to: 501-510
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_tile.cpp` around lines 337 - 347, The "position" sol::property currently constructs a plain Lua table from Tile::getPosition(), which prevents binding to the registered Position usertype; change the accessor to return the Position usertype instead of a sol::table by creating a Lua userdata/object for the Position (e.g. use sol::make_object(lua, tile->getPosition()) or equivalent sol userdata creation) and return that; apply the same change to the similar getPosition() binding (the other accessor around lines ~501-510) so callers receive the registered Position usertype rather than a raw table.
🧹 Nitpick comments (9)
source/game/creatures.h (1)
22-23: Reduce header coupling by forward-declaring wx types.Since this header only uses references to
wxFileName/wxString, you can forward-declare them and move<wx/filename.h>to the.cppto reduce transitive include cost.♻️ Suggested refactor
`#include` "game/outfit.h" `#include` "ext/pugixml.hpp" -#include <wx/filename.h> `#include` <string> `#include` <map> `#include` <vector> +class wxFileName; +class wxString; + class CreatureType; class CreatureBrush;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/game/creatures.h` around lines 22 - 23, Forward-declare the wx types used as references in this header instead of including <wx/filename.h>: add forward declarations for "class wxFileName;" and "class wxString;" in the header that currently includes those types (creatures.h) and remove the `#include` "<wx/filename.h>" from that header; then add the necessary includes (<wx/filename.h> and/or <wx/string.h>) to the corresponding .cpp where functions/methods that use wxFileName or wxString are defined so any uses of their members have the full definitions available (check functions/methods named in this header to ensure their implementations are updated to include the wx headers).source/ui/gui.cpp (1)
67-83: Static local state inemitBrushChangeIfNeededis not thread-safe.The function uses
staticlocal variables (lastBrush,lastBrushName) which are shared across all calls. While this is likely called only from the main UI thread, consider documenting this assumption or using thread-local storage if there's any chance of concurrent access.Additionally, the comparison
currentBrush != lastBrush || currentName != lastBrushNamecorrectly handles the case where a brush pointer remains the same but its name changes (e.g., brush reconfiguration).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/ui/gui.cpp` around lines 67 - 83, emitBrushChangeIfNeeded uses shared static locals (lastBrush, lastBrushName) which are not thread-safe; replace those function-scope statics with thread-local storage (e.g., thread_local const Brush* lastBrush and thread_local std::string lastBrushName) so each thread keeps its own state, or alternatively protect access with a mutex. Update emitBrushChangeIfNeeded (which calls GUI::GetCurrentBrush and g_luaScripts.emit("brushChange", ...)) to use thread_local variables (or a std::mutex) and add a brief comment documenting the assumption about main-thread UI calls if you choose the mutex/annotation route.source/map/tile.h (1)
67-69: Default-initializeownedLocation.This new owning pointer now depends on every constructor path remembering to initialize it before the destructor or
setLocation()logic touches it. Giving it an in-classnullptrinitializer makes the ownership state safe by default.♻️ Suggested change
TileLocation* location; - TileLocation* ownedLocation; + TileLocation* ownedLocation = nullptr;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/map/tile.h` around lines 67 - 69, The member ownedLocation is not default-initialized, so some constructor paths may leave it indeterminate and break the destructor or setLocation() logic; update the Tile class member declaration for ownedLocation in tile.h to give it an in-class nullptr initializer (e.g. "TileLocation* ownedLocation = nullptr;") so ownership state is safe by default and you don't have to update every constructor path that touches ownedLocation, ensuring existing code in setLocation() and the Tile destructor remains correct.source/lua/lua_api_selection.cpp (2)
148-175: Rawstart/finishmethods are still exposed alongsidetransaction.While the
transactionmethod provides a safe scoped alternative, the rawstartandfinishmethods remain accessible. Thefinishmethod has guards (isBusy()andisDeferred()checks), but Lua scripts could still misuse them. Consider documenting thattransactionis the preferred approach, or marking these as internal/advanced APIs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_selection.cpp` around lines 148 - 175, The raw Selection::start and Selection::finish bindings are still exposed alongside the safer transaction API; update the Lua bindings so start/finish are treated as internal-only (e.g., rename to _start/_finish or remove from the public table) and add a brief doc comment pointing scripts to use Selection::transaction instead; keep the existing guard logic (isBusy/isDeferred) only for internal/private usage and ensure the public API surface exposes transaction as the preferred/scoped method.
176-183: Consider usingsol::protected_functionfor safer callback invocation.The
transactionbinding calls the Lua callback directly viacallback(). If the Lua code throws an error, sol2 will propagate it as a C++ exception. While theScopedInternalSelectionSessionguard will still clean up correctly, usingsol::protected_functionwould allow you to handle Lua errors more gracefully and return error information to the caller.♻️ Proposed change for safer error handling
- "transaction", [](Selection* sel, sol::function callback) { + "transaction", [](Selection* sel, sol::protected_function callback) { if (!sel || !callback.valid()) { return; } const ScopedInternalSelectionSession guard(sel); - callback(); + auto result = callback(); + if (!result.valid()) { + sol::error err = result; + throw std::runtime_error(std::string("Selection transaction failed: ") + err.what()); + } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_selection.cpp` around lines 176 - 183, The transaction binding currently takes a sol::function named callback and calls it directly; change the parameter to sol::protected_function (e.g., replace sol::function callback with sol::protected_function callback in the "transaction" binding), validate callback.valid() as before, create the ScopedInternalSelectionSession guard(sel) and invoke callback via its protected call API (call(...) / operator()) and then check the result for errors; on failure extract the error/message from the protected result and return or propagate that error back to Lua instead of allowing a C++ exception to escape. Ensure you reference the "transaction" binding, the Selection* parameter, and ScopedInternalSelectionSession when updating the lambda.source/lua/lua_api_json.cpp (1)
163-181: Consider returning error info fromdecodeinstead of silentnil.The
encodefunctions throw exceptions on error (cycle, unsupported types), butdecodesilently returnsnilon parse failure. This asymmetry can make debugging harder for script authors. Consider returningnil, error_messageor documenting the difference clearly.♻️ Optional: Return error message from decode
- jsonTable.set_function("decode", [](std::string jsonStr, sol::this_state s) -> sol::object { + jsonTable.set_function("decode", [](std::string jsonStr, sol::this_state s) -> std::tuple<sol::object, sol::object> { try { nlohmann::json val = nlohmann::json::parse(jsonStr); sol::state_view lua(s); - return jsonToLua(val, lua); + return std::make_tuple(jsonToLua(val, lua), sol::nil); } catch (const nlohmann::json::exception& e) { - return sol::nil; + sol::state_view lua(s); + return std::make_tuple(sol::nil, sol::make_object(lua, std::string(e.what()))); } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_json.cpp` around lines 163 - 181, The decode lambda currently swallows parse errors and returns only sol::nil; change its behavior to mirror encode's error reporting by catching the nlohmann::json::exception as ex and returning two Lua values: on success return jsonToLua(val, lua) (single value), and on failure return sol::nil followed by ex.what() (a string) so script authors get an error message; update the jsonTable.set_function("decode", ...) signature/return handling accordingly and ensure callers handle the (value, error) tuple.source/lua/lua_api_geo.cpp (1)
143-149: Read ordered point lists by index, not generic table iteration.
bezierCurve*,polygon, andpointInPolygonall depend on control-point order, but these loops build their vectors with genericsol::tableiteration. For array-style Lua input, use1..table.size()/ipairssemantics here so sparse or mixed tables can't scramble geometry.Also applies to: 197-203, 669-675, 790-796
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_api_geo.cpp` around lines 143 - 149, The loop that builds points from a sol::table uses generic iteration (for (auto& kv : controlPoints)) which can reorder array-style Lua tables; change it to iterate by integer index from 1 to controlPoints.size() and fetch each entry with controlPoints.get_or(index, sol::nil) (or controlPoints[index]) to preserve Lua ipairs ordering, then read x/y via pt.get_or("x", pt.get_or(1, 0.0f)) and pt.get_or("y", pt.get_or(2, 0.0f)) as before; apply this same indexed iteration fix to the other occurrences used by bezierCurve*, polygon, and pointInPolygon so control-point order remains stable for geometry construction.source/lua/lua_script_manager.h (1)
250-251: Macro accessor is functional but consider a typed alternative.The
g_luaScriptsmacro works but doesn't provide type information to IDEs and can complicate debugging. An inline function would be slightly more robust.♻️ Optional: Use inline function for better type safety
-// Global accessor macro -#define g_luaScripts LuaScriptManager::getInstance() +// Global accessor +inline LuaScriptManager& g_luaScripts() { + return LuaScriptManager::getInstance(); +}Note: This would require updating all call sites from
g_luaScripts.method()tog_luaScripts().method().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_script_manager.h` around lines 250 - 251, The g_luaScripts macro hides type information and IDE support; replace the macro definition with a typed inline accessor function that returns LuaScriptManager& (wrapping LuaScriptManager::getInstance()), then update call sites to use g_luaScripts().method() instead of g_luaScripts.method(); keep the accessor name identical (e.g., g_luaScripts()) so changes are minimal and ensure it is declared in the same header alongside LuaScriptManager for linkage and inlining.source/lua/lua_script_manager.cpp (1)
630-634: Consider using wxFileName for cross-platform path handling.The platform-specific separator handling works but is slightly fragile. wxFileName handles path separators automatically and would be more robust.
♻️ Optional improvement using wxFileName
-#ifdef _WIN32 - const std::string sep = "\\"; -#else - const std::string sep = "/"; -#endif - wxString name; // First, check for manifest.lua in the CURRENT directory // If it exists, this directory IS a package, don't scan deeper - std::string manifestPath = directory + sep + "manifest.lua"; + wxFileName manifestFn(directory, "manifest.lua"); + std::string manifestPath = manifestFn.GetFullPath().ToStdString();Apply similar changes to other path concatenations in this function.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lua/lua_script_manager.cpp` around lines 630 - 634, Replace manual platform-specific separator logic (the sep constant) and string concatenation in the Lua script manager function with wxFileName-based path construction: create a wxFileName for the base directory and use wxFileName methods (e.g., AppendDir, SetFullName or CreateTempFileName/Assign) or the wxFileName(path1, path2) constructor to join parts, then call GetFullPath() when you need the combined string; also update any other path concatenations in the same function to use wxFileName to ensure robust cross-platform path handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f590631f-8c88-4092-bf54-2327eb3acfc6
📒 Files selected for processing (96)
CMakeLists.txtbuild_ninja.batdata/menubar.xmlscripts/README.mdscripts/fps_counter.luascripts/hello_world/hello_world.luascripts/hello_world/manifest.luascripts/linter.luascripts/terrain_generator_demo.luasource/CMakeLists.txtsource/app/application.cppsource/app/preferences/preferences_layout.cppsource/brushes/brush.hsource/editor/action.cppsource/editor/action.hsource/editor/action_queue.cppsource/editor/action_queue.hsource/editor/editor.hsource/editor/persistence/editor_persistence.cppsource/editor/selection.cppsource/editor/selection.hsource/ext/fast_noise_lite.hsource/game/creatures.hsource/game/items.hsource/io/filehandle.hsource/lua/lua_api.cppsource/lua/lua_api.hsource/lua/lua_api_algo.cppsource/lua/lua_api_algo.hsource/lua/lua_api_app.cppsource/lua/lua_api_app.hsource/lua/lua_api_brush.cppsource/lua/lua_api_brush.hsource/lua/lua_api_color.cppsource/lua/lua_api_color.hsource/lua/lua_api_creature.cppsource/lua/lua_api_creature.hsource/lua/lua_api_geo.cppsource/lua/lua_api_geo.hsource/lua/lua_api_http.cppsource/lua/lua_api_http.hsource/lua/lua_api_image.cppsource/lua/lua_api_image.hsource/lua/lua_api_item.cppsource/lua/lua_api_item.hsource/lua/lua_api_json.cppsource/lua/lua_api_json.hsource/lua/lua_api_map.cppsource/lua/lua_api_map.hsource/lua/lua_api_noise.cppsource/lua/lua_api_noise.hsource/lua/lua_api_position.cppsource/lua/lua_api_position.hsource/lua/lua_api_selection.cppsource/lua/lua_api_selection.hsource/lua/lua_api_tile.cppsource/lua/lua_api_tile.hsource/lua/lua_dialog.cppsource/lua/lua_dialog.hsource/lua/lua_engine.cppsource/lua/lua_engine.hsource/lua/lua_script.cppsource/lua/lua_script.hsource/lua/lua_script_manager.cppsource/lua/lua_script_manager.hsource/lua/lua_scripts_window.cppsource/lua/lua_scripts_window.hsource/lua/lua_sol_config.hsource/map/map_region.cppsource/map/map_region.hsource/map/tile.cppsource/map/tile.hsource/rendering/core/coordinate_mapper.cppsource/rendering/core/coordinate_mapper.hsource/rendering/core/map_overlay.hsource/rendering/drawers/overlays/lua_overlay_drawer.cppsource/rendering/drawers/overlays/lua_overlay_drawer.hsource/rendering/map_drawer.cppsource/rendering/map_drawer.hsource/rendering/ui/map_display.cppsource/rendering/ui/map_display.hsource/rendering/ui/map_menu_handler.cppsource/rendering/ui/map_menu_handler.hsource/ui/common_windows.hsource/ui/gui.cppsource/ui/gui_ids.hsource/ui/main_frame.cppsource/ui/main_menubar.cppsource/ui/main_menubar.hsource/ui/map_popup_menu.cppsource/ui/menubar/menubar_action_manager.cppsource/ui/menubar/script_menu_handler.cppsource/ui/menubar/script_menu_handler.hsource/ui/menubar_loader.cppsource/ui/menubar_loader.hvcpkg.json
💤 Files with no reviewable changes (1)
- build_ninja.bat
| return { | ||
| name = "Lua UI Showcase", | ||
| description = "A curated five-tab showcase for layout, controls, previews, environment tools, and system actions.", | ||
| version = "1.0.0", | ||
| author = "Michy", | ||
| main = "hello_world", | ||
| shortcut = "Ctrl+Shift+H", | ||
| autorun = false | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check what files exist in the hello_world script directory and how main is resolved
fd -t f . scripts/hello_world/
# Check how the script loader parses the main field
rg -n "main" source/lua/lua_script.cpp -C 3Repository: karolak6612/remeres-map-editor-redux
Length of output: 1561
Fix indentation inconsistency on line 8.
Line 8 uses a tab character for indentation while the rest of the table uses spaces. This should be corrected for consistency.
The main = "hello_world" field is correct—the script loader automatically appends the .lua extension to values without it, correctly resolving to hello_world.lua.
Suggested fix
return {
name = "Lua UI Showcase",
description = "A curated five-tab showcase for layout, controls, previews, environment tools, and system actions.",
version = "1.0.0",
author = "Michy",
main = "hello_world",
shortcut = "Ctrl+Shift+H",
- autorun = false
+ autorun = false
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return { | |
| name = "Lua UI Showcase", | |
| description = "A curated five-tab showcase for layout, controls, previews, environment tools, and system actions.", | |
| version = "1.0.0", | |
| author = "Michy", | |
| main = "hello_world", | |
| shortcut = "Ctrl+Shift+H", | |
| autorun = false | |
| } | |
| return { | |
| name = "Lua UI Showcase", | |
| description = "A curated five-tab showcase for layout, controls, previews, environment tools, and system actions.", | |
| version = "1.0.0", | |
| author = "Michy", | |
| main = "hello_world", | |
| shortcut = "Ctrl+Shift+H", | |
| autorun = false | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/hello_world/manifest.lua` around lines 1 - 9, The indentation on the
table entry for autorun is inconsistent (it uses a tab); update the line
containing the autorun = false entry so its leading whitespace matches the other
table entries (use the same number of spaces as lines like main =
"hello_world"), leaving the key name and value unchanged; locate the autorun
line in the returned table and replace the tab with spaces to maintain
consistent indentation.
| ### Location | ||
| All Lua scripts must be placed in the `scripts` directory of your RME installation. | ||
| The editor scans this directory on startup. Files must have the `.lua` extension. | ||
|
|
There was a problem hiding this comment.
Split “discovery” from “execution” in the startup docs.
This section says the loader only scans .lua files and that scripts execute on editor load, but the package section later documents directory-based scripts with optional autorun. As written, package scripts and non-autorun scripts are described incorrectly.
📝 Suggested wording
-All Lua scripts must be placed in the `scripts` directory of your RME installation.
-The editor scans this directory on startup. Files must have the `.lua` extension.
+The editor scans the `scripts` directory on startup for:
+- single-file scripts ending in `.lua`
+- package directories containing `manifest.lua`
@@
-Scripts are executed sequentially when the editor loads. You can define global functions or use the API immediately.
+Scripts are discovered when the editor loads. Only autorun scripts execute automatically; other scripts are exposed through the Scripts UI / shortcuts.Also applies to: 44-46
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/README.md` around lines 30 - 33, Split the current combined note into
two clear subsections: "Discovery" and "Execution". In "Discovery" state that
the loader scans the scripts directory for files with the .lua extension
(reference the scripts directory and .lua). In "Execution" explain that
discovery does not imply automatic running — only scripts marked autorun (or
those placed in package-defined autorun directories) are executed on editor
startup, while package scripts without autorun are discovered but not executed;
mention the optional autorun flag and package-based directory conventions so
readers understand the distinction between finding scripts and running them.
| * `show()`: Displays the dialog modally. The script waits here until the dialog is closed. | ||
| * `close()`: Closes the dialog programmatically. | ||
| * `show({wait=true, center="parent"|"screen", center=false, bounds=...})`: `center` defaults to parent when no explicit `x/y` or `bounds` are set. |
There was a problem hiding this comment.
Document show(...) as alternatives, not one invalid Lua literal.
The same options table lists center twice. In code form that reads as invalid, copy-pasteable Lua. Split the "parent", "screen", and false cases into separate examples or prose.
📝 Suggested wording
-* `show({wait=true, center="parent"|"screen", center=false, bounds=...})`: `center` defaults to parent when no explicit `x/y` or `bounds` are set.
+* `show({wait=true, center="parent", bounds=...})`
+* `show({wait=true, center="screen", bounds=...})`
+* `show({wait=true, center=false, bounds=...})`
+ `center` defaults to the parent when no explicit `x/y` or `bounds` are set.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/README.md` around lines 421 - 423, The README's description of the
show(...) options for show() is listing `center` twice and uses an invalid Lua
literal like `"parent"|"screen", center=false`; update the prose so `show`'s
options are expressed as alternatives rather than a single invalid expression:
document `center` as accepting one of the values (e.g., "parent", "screen", or
false) and/or provide separate example calls such as show({wait=true,
center="parent"}), show({wait=true, center="screen"}), and show({wait=true,
center=false, bounds=...}) or split into three short bullets explaining each
behavior; ensure references to the `show` function and its `center` option are
preserved and unambiguous.
| struct CacheKey { | ||
| int start_x = 0; | ||
| int start_y = 0; | ||
| int end_x = 0; | ||
| int end_y = 0; | ||
| int floor = 0; | ||
| double zoom = 0.0; | ||
| int view_scroll_x = 0; | ||
| int view_scroll_y = 0; | ||
| int tile_size = 0; | ||
| int screen_width = 0; | ||
| int screen_height = 0; | ||
| }; | ||
|
|
||
| MapDrawer* mapDrawer; | ||
| std::vector<MapOverlayCommand> cachedCommands; | ||
| CacheKey cachedKey {}; | ||
| bool cacheValid = false; | ||
|
|
||
| void refreshCache(const RenderView& view); | ||
| CacheKey makeCacheKey(const RenderView& view) const; | ||
| }; |
There was a problem hiding this comment.
Invalidate the overlay cache when Lua overlay state changes.
CacheKey only tracks view geometry. Any overlay that changes because Lua state, selection, or action history changed while the camera stayed still will keep reusing cachedCommands, so the user will not see the update until they pan or zoom.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/rendering/drawers/overlays/lua_overlay_drawer.h` around lines 24 - 45,
Cache invalidation only uses view geometry so overlays driven by
Lua/selection/history never refresh; extend CacheKey to include a short
overlay-state signature (e.g., lua_overlay_version or state_hash, selection_id,
and/or action_history_version) and populate it inside makeCacheKey(const
RenderView& view) using the same source used to compute overlays (Lua
state/version from the MapDrawer or RenderView, current selection/action history
id). Update refreshCache to compare the new CacheKey against cachedKey and set
cacheValid = false when the overlay-state fields differ so cachedCommands are
regenerated when Lua/selection/history changes.
| ////////////////////////////////////////////////////////////////////// | ||
| // This file is part of Remere's Map Editor | ||
| ////////////////////////////////////////////////////////////////////// | ||
| // Compatibility stub — legacy code included this header but nothing | ||
| // from it is actually used in the lua subsystem. | ||
| ////////////////////////////////////////////////////////////////////// | ||
|
|
||
| #ifndef RME_UI_COMMON_WINDOWS_H_ | ||
| #define RME_UI_COMMON_WINDOWS_H_ | ||
|
|
||
| // This header intentionally left minimal. | ||
| // If you need specific dialog classes, include them directly. | ||
|
|
||
| #endif // RME_UI_COMMON_WINDOWS_H_ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find all files that include common_windows.h to assess removal feasibility.
# Search for includes of this header
rg -n '#include.*common_windows\.h' --type=cpp --type=c
# Also check for quoted includes
rg -n '#include.*".*common_windows\.h"' --type=cpp --type=cRepository: karolak6612/remeres-map-editor-redux
Length of output: 199
🏁 Script executed:
# Check what lua_dialog.cpp includes around line 27 and what symbols it uses
sed -n '20,50p' source/lua/lua_dialog.cpp
# Also check if any symbol from common_windows.h is actually used in the file
rg -n 'common_windows' source/lua/lua_dialog.cppRepository: karolak6612/remeres-map-editor-redux
Length of output: 883
Remove the include of this stub header from lua_dialog.cpp:27 and delete this file.
This empty stub has no declarations and is never used. The include at source/lua/lua_dialog.cpp:27 can be safely removed, eliminating both the file and the dead include.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/ui/common_windows.h` around lines 1 - 14, Remove the dead include and
header: delete the empty stub header (common_windows.h) and remove its `#include`
from lua_dialog.cpp (the include at the top of lua_dialog.cpp around line 27) so
the build no longer depends on RME_UI_COMMON_WINDOWS_H_; ensure no other source
files include common_windows.h before committing the deletion.
…on tracking Updated Lua type annotations in linter.lua (Color fields r/g/b/a, MapView method signatures). Added map generation counter to detect stale iterators and invalidate overlays. Improved file I/O with binary mode and 4MB size limits. Enhanced color parsing with wxColour backend and hex support. Added selection change tracking for tile modifications. Fixed dock panel cleanup on shutdown.
|
🤖 Hi @karolak6612, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
Added 18 Lua test files covering all RME Redux API modules (Position, Selection, Creature, Brush, Color, JSON, HTTP, Noise, Algo, Geo, Items, Dialog) with ~450+ test cases. Includes test framework, three test runners (complete/bulk/original), and documentation.
|
🤖 Hi @karolak6612, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
1 participant
Thanks to https://github.com/michyaraque for the original implemetation in OTA RME
PR 1 - OTAcademy#37
PR 2 - OTAcademy#38
Summary by CodeRabbit
New Features
New Features (Demos & Tools)
Documentation
Chores