Skip to content

DM Placeables Tool#1958

Open
LazyTrain082 wants to merge 16 commits intozunath:masterfrom
LazyTrain082:DM-Tools
Open

DM Placeables Tool#1958
LazyTrain082 wants to merge 16 commits intozunath:masterfrom
LazyTrain082:DM-Tools

Conversation

@LazyTrain082
Copy link
Copy Markdown
Contributor

@LazyTrain082 LazyTrain082 commented Mar 29, 2026

New command: /plctools

DMs now have a dedicated placeable tool NUI panel that should help with map setup.

The tool does the following:
Allows you to see all DM-spawned placeables in the current map in one list.
Click to highlight/select for editing like the current player housing system
Nudge positions on XYZ and rotate facing
Quick search/filter to find a specific placeable
Save and load setups as named layout presets
Missing placeables from a saved layout get respawned automatically
Shared presets let the whole DM team reuse setups in shared-event scenarios
Layouts capped at 30 with a short 5 sec cooldown to prevent spam/mistakes

Summary by CodeRabbit

  • New Features
    • Added /plctools (alias /placeabletools) DM/Admin-only command to open a resizable DM Placeable Tools window.
    • Browse DM-spawned placeables with search, pagination, refresh, selection highlighting, and contextual instructions/validation.
    • Live edit selected placeable: adjust X/Y/Z, reset Z to ground, rotate/set facing, and delete.
    • Save, load, and delete per-area layout presets (limits and cooldowns apply); applying presets moves or spawns placeables and reports results.
    • DM-spawned placeables are now tracked so tools reliably identify and manage them.

/dmtools now opens a placeable menu built off the existing player housing menu.

Comes with a save/load feature.
Added necessary lines back into the module.ifo.json to stop it from packing corrupted .mod files.
Trying to patch anything that might cause issues
30 layouts per area (shared across al DMs) and a 5 second cooldown to avoid spamming the server with requests.
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Mar 29, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds a DM Placeable Tools feature: new entity types for persisting area placeable layouts, a DM-only chat command to open a GUI, DM-spawn metadata tagging, a GUI window definition, and a server-side view-model implementing discovery, live editing, pagination/search, and layout save/load/delete workflows.

Changes

Cohort / File(s) Summary
Data Model
SWLOR.Game.Server/Entity/DMAreaPlaceableLayout.cs
Adds DMAreaPlaceableLayout and DMAreaPlaceableLayoutEntry to persist per-area placeable layout entries (tag, resref, match index, position, facing).
Chat Command
SWLOR.Game.Server/Feature/ChatCommandDefinition/DMChatCommand.cs
Registers plctools (alias placeabletools) DM-only chat command to toggle the DM Tools GUI; restricted to DM/Admin.
DM Spawn Handling
SWLOR.Game.Server/Feature/DMActions.cs
Parses OBJECT_TYPE from DM spawn payload, validates it, and when placeables are spawned marks them with local metadata (DM_SPAWNED_PLACEABLE, spawner UUID/name) using correct DM reference resolution.
GUI Definition
SWLOR.Game.Server/Feature/GuiDefinition/DMToolsDefinition.cs
Adds DMToolsDefinition implementing IGuiWindowDefinition, building a resizable DM Tools window with instructions, paginated placeable list, search, placeable actions, position/facing controls, and layout preset controls.
ViewModel / Business Logic
SWLOR.Game.Server/Feature/GuiDefinition/ViewModel/DMToolsViewModel.cs
Adds DMToolsViewModel that enumerates DM-spawned placeables, supports search/pagination, selection with edit-ownership and stale-timeout, live move/rotate/delete actions, and implements layout save/load/delete with grouping by (Tag,Resref)/match-index, cooldowns, and spawn/move application logic.

Sequence Diagram(s)

sequenceDiagram
    participant DM as DM Player
    participant Chat as ChatCommand
    participant GUI as DMTools Window
    participant VM as DMToolsViewModel
    participant Area as Game Area
    participant DB as Database

    DM->>Chat: /plctools
    Chat->>GUI: Toggle open
    GUI->>VM: Initialize
    VM->>Area: Scan for DM_SPAWNED_PLACEABLE objects
    VM->>DB: Load layouts for area
    DB-->>VM: Return layouts
    VM-->>GUI: Populate placeable list & layout toggles

    DM->>GUI: Search / Page / Select / Edit actions
    GUI->>VM: Action callbacks
    VM->>Area: Highlight / Move / Rotate / Delete / Spawn placeables
    VM->>DB: Save / Load / Delete layout records
    VM-->>GUI: Update UI (counts, instructions)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Poem

🐇 I hop through maps with a twitch and a cheer,
I tag tiny placeables that DMs hold dear.
Toggle the tools, nudge X, Y, Z with delight,
Save a layout at dusk, load it by moonlight. ✨🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The pull request title 'DM Placeables Tool' directly and accurately describes the main change: a new DM tools system for editing and managing placeables with a dedicated NUI panel.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@LazyTrain082
Copy link
Copy Markdown
Contributor Author

image

Preview of the UI. Preview video is too big to put on GitHub

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@SWLOR.Game.Server/Feature/GuiDefinition/ViewModel/DMToolsViewModel.cs`:
- Around line 497-548: The code stamps DM_SPAWNED_BY* from Player directly which
attributes spawns to a possessed NPC; change the dmId/dmName assignment to
resolve the real DM first by checking GetIsDMPossessed(Player) and, if true,
calling GetMaster(Player) and using that master (if valid) for
GetObjectUUID/GetName; otherwise fall back to Player—update references around
dmId/dmName initialization (where GetObjectUUID(Player) and GetName(Player) are
used) to implement this resolution.
- Around line 420-455: Trim LayoutName at the start and use the trimmed value
for validation, comparisons, searches and when creating/saving a new
DMAreaPlaceableLayout to avoid near-duplicates; specifically, replace uses of
LayoutName with a trimmed variable (e.g., trimmedName = LayoutName?.Trim()),
check string.IsNullOrWhiteSpace(trimmedName), use trimmedName when matching
existing.Name (StringComparison.OrdinalIgnoreCase) and when assigning Name on a
new DMAreaPlaceableLayout, and ensure the area lookup (areaQuery) and
MaxLayoutsPerArea check operate against trimmedName to prevent saving names that
differ only by trailing/leading whitespace.
- Around line 588-597: Capture the selected layout values into locals before
calling ShowModal so the confirmation text and deletion use the same snapshot;
e.g., read LayoutName into a local (e.g., selectedName) and
_layoutIds[_selectedLayoutIndex] into a local (e.g., selectedId) immediately
before invoking ShowModal, then use those locals inside the modal confirmation
callback and the message instead of referencing LayoutName or
_selectedLayoutIndex/_layoutIds at callback time (affects ShowModal call,
LayoutName usage, _layoutIds/_selectedLayoutIndex lookup, and
DB.Delete<DMAreaPlaceableLayout> invocation).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 9ca9483f-af72-4930-a26a-d109bbf89067

📥 Commits

Reviewing files that changed from the base of the PR and between d7d760b and bbe74e5.

📒 Files selected for processing (5)
  • SWLOR.Game.Server/Entity/DMAreaPlaceableLayout.cs
  • SWLOR.Game.Server/Feature/ChatCommandDefinition/DMChatCommand.cs
  • SWLOR.Game.Server/Feature/DMActions.cs
  • SWLOR.Game.Server/Feature/GuiDefinition/DMToolsDefinition.cs
  • SWLOR.Game.Server/Feature/GuiDefinition/ViewModel/DMToolsViewModel.cs

Comment thread SWLOR.Game.Server/Feature/GuiDefinition/ViewModel/DMToolsViewModel.cs Outdated
Comment thread SWLOR.Game.Server/Feature/GuiDefinition/ViewModel/DMToolsViewModel.cs Outdated
Comment thread SWLOR.Game.Server/Feature/GuiDefinition/ViewModel/DMToolsViewModel.cs Outdated
I forgot the delete button. Surgically delete placeables rather than relying on the DMFI tools. (Can only delete spawned placeables)
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
SWLOR.Game.Server/Feature/GuiDefinition/ViewModel/DMToolsViewModel.cs (2)

734-745: "Save Changes" button name may be misleading.

The button updates the instruction text with current coordinates but doesn't persist anything—position/facing changes are already applied live by the individual nudge buttons. Consider renaming to "Show Position" or "Confirm Changes" to clarify that layout-level persistence requires "Save Layout."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@SWLOR.Game.Server/Feature/GuiDefinition/ViewModel/DMToolsViewModel.cs` around
lines 734 - 745, The "Save Changes" handler name is misleading because it only
updates the on-screen position/facing text rather than persisting layout; rename
the method OnSaveChanges to a clearer name (e.g., OnShowPosition or
OnConfirmChanges) and update all references/bindings (the UI button that
triggers this action) to use the new name, while keeping the existing logic that
reads GetSelectedPlaceable(), GetPosition(placeable), GetFacing(placeable),
SelectedPlaceableIndex and _placeableDisplayNames and sets
Instructions/InstructionColor; also update the button label in the view to "Show
Position" or "Confirm Changes" to match the new method name.

477-487: Cooldown uses int truncation for Unix timestamp.

The cooldown stores Unix timestamps as int via SetLocalInt/GetLocalInt. This works correctly today but will overflow after 2038 (Y2038 problem). This is likely acceptable given NWN's local variable system only supports 32-bit integers.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@SWLOR.Game.Server/Feature/GuiDefinition/ViewModel/DMToolsViewModel.cs` around
lines 477 - 487, The code currently casts the 64-bit Unix timestamp to int which
risks Y2038 overflow; read the stored value with GetLocalInt into a long (e.g.
long nextAllowedLoad = (long)GetLocalInt(Player,
NextLayoutLoadTimestampVariable)), compare using long now =
DateTimeOffset.UtcNow.ToUnixTimeSeconds(), and when setting use a clamp before
calling SetLocalInt: compute long target = now + LayoutLoadCooldownSeconds and
if target > int.MaxValue set the stored value to int.MaxValue else cast to
(int)target; keep references to NextLayoutLoadTimestampVariable,
LayoutLoadCooldownSeconds, GetLocalInt, and SetLocalInt, and preserve the
Instruction/InstructionColor logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@SWLOR.Game.Server/Feature/GuiDefinition/ViewModel/DMToolsViewModel.cs`:
- Around line 734-745: The "Save Changes" handler name is misleading because it
only updates the on-screen position/facing text rather than persisting layout;
rename the method OnSaveChanges to a clearer name (e.g., OnShowPosition or
OnConfirmChanges) and update all references/bindings (the UI button that
triggers this action) to use the new name, while keeping the existing logic that
reads GetSelectedPlaceable(), GetPosition(placeable), GetFacing(placeable),
SelectedPlaceableIndex and _placeableDisplayNames and sets
Instructions/InstructionColor; also update the button label in the view to "Show
Position" or "Confirm Changes" to match the new method name.
- Around line 477-487: The code currently casts the 64-bit Unix timestamp to int
which risks Y2038 overflow; read the stored value with GetLocalInt into a long
(e.g. long nextAllowedLoad = (long)GetLocalInt(Player,
NextLayoutLoadTimestampVariable)), compare using long now =
DateTimeOffset.UtcNow.ToUnixTimeSeconds(), and when setting use a clamp before
calling SetLocalInt: compute long target = now + LayoutLoadCooldownSeconds and
if target > int.MaxValue set the stored value to int.MaxValue else cast to
(int)target; keep references to NextLayoutLoadTimestampVariable,
LayoutLoadCooldownSeconds, GetLocalInt, and SetLocalInt, and preserve the
Instruction/InstructionColor logic.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f5df4294-c154-489d-9465-b663cf7572f7

📥 Commits

Reviewing files that changed from the base of the PR and between bbe74e5 and 806146d.

📒 Files selected for processing (2)
  • SWLOR.Game.Server/Feature/GuiDefinition/DMToolsDefinition.cs
  • SWLOR.Game.Server/Feature/GuiDefinition/ViewModel/DMToolsViewModel.cs


private void DMTools()
{
_builder.Create("dmtools", "plctools")
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"dmtools" is too generic. This is specifically for placeable setup & storage. Suggest you use something less generic or keep it as plctools

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Swapped to /plctools specifically

_builder.Create("dmtools", "plctools")
.Description("Toggles the DM Placeable Tools window.")
.Permissions(AuthorizationLevel.DM, AuthorizationLevel.Admin)
.AvailableToAllOnTestEnvironment()
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want this available to everyone on the test environment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed test environment for everyone

As requested:

Added area lock vars
Changed Dmtools just to plctools
Area-scoped cooldown
@LazyTrain082
Copy link
Copy Markdown
Contributor Author

@coderabbitai re-review with current requested changes implemented.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 4, 2026

@LazyTrain082 Sure! I'll kick off a fresh review of the PR with the current changes.

✅ Actions performed

Full review triggered.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@SWLOR.Game.Server/Feature/GuiDefinition/ViewModel/DMToolsViewModel.cs`:
- Around line 308-312: Cached _layoutIds may be stale when the DM changes areas;
before performing load or delete actions (e.g., in the lambda returned by
OnRefreshPlaceables and in OnDeleteLayout), refresh/validate layouts from
RebuildPlaceables/Search and verify the selected layout's layout.AreaResref
equals the current GetAreaResref() value; only proceed to set the shared load
cooldown or open the delete modal after this validation passes. Locate uses of
_layoutIds and the cooldown assignment in the load handler and the modal-trigger
in OnDeleteLayout and add the guard that re-fetches the placeable list (call
RebuildPlaceables()/Search()) and compares layout.AreaResref to GetAreaResref()
before applying the cooldown or opening the confirmation.
- Around line 284-300: GetSelectedPlaceable currently only verifies the object
handle validity; additionally check that the cached placeable is in the current
player area by comparing GetArea(placeable) (or GetArea(placeable) equivalent)
to GetArea(Player) after GetIsObjectValid(placeable) returns true, and if the
areas differ set Instructions/InstructionColor, call ClearHighlight(),
RebuildPlaceables(), Search() and return OBJECT_INVALID; update references:
GetSelectedPlaceable(), SelectedPlaceableIndex, _placeables,
GetIsObjectValid(placeable), GetArea(Player), OBJECT_INVALID, Instructions,
InstructionColor, ClearHighlight(), RebuildPlaceables(), Search().
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0acd74aa-52c7-49d2-82be-917293c82739

📥 Commits

Reviewing files that changed from the base of the PR and between 806146d and eaa316f.

📒 Files selected for processing (2)
  • SWLOR.Game.Server/Feature/ChatCommandDefinition/DMChatCommand.cs
  • SWLOR.Game.Server/Feature/GuiDefinition/ViewModel/DMToolsViewModel.cs

Added lightweight per-placeable edit claims in DMToolsViewModel

Added stale timeout: EditingClaimStaleSeconds = 120

Enforced ownership checks before:
AdjustPosition
AdjustFacing
OnZAxisReset
OnDeletePlaceable

When selected placeable is invalid (e.g., another DM deleted it), message now says:
"Selected placeable no longer exists (possibly removed by another DM). List refreshed.

Hardened OnDeleteLayout modal callback
@LazyTrain082
Copy link
Copy Markdown
Contributor Author

@coderabbitai re-re-review for all the marbles, CodeVole.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 4, 2026

@LazyTrain082 All the marbles it is! 🎱 Kicking off a full re-review right now — let's see how this holds up!


🧠 Learnings used
Learnt from: zunath
Repo: zunath/SWLOR_NWN PR: 1958
File: SWLOR.Game.Server/Feature/GuiDefinition/ViewModel/DMToolsViewModel.cs:770-770
Timestamp: 2026-04-04T17:02:24.442Z
Learning: Preference: For DM/admin tools in SWLOR.Game.Server, favor code-level concurrency guards over relying on manual DM coordination. In DMToolsViewModel, use placeable-local edit claims (DMTOOLS_EDITING_DM_ID/NAME/TS) with stale timeout and release on window close to prevent multi-DM overwrite.

Learnt from: LazyTrain082
Repo: zunath/SWLOR_NWN PR: 1958
File: SWLOR.Game.Server/Feature/GuiDefinition/ViewModel/DMToolsViewModel.cs:308-312
Timestamp: 2026-04-04T16:54:53.747Z
Learning: In SWLOR.Game.Server (C#), NUI windows (including DMToolsViewModel / GuiWindowType.DMTools) automatically close when a DM jumps to a different area. This means cached per-area state (e.g., `_layoutIds`, `_allPlaceables`) is always rebuilt fresh via `Initialize()` when the window is reopened in the new area. Do not flag stale-cache concerns for area-scoped data in this view-model, as the window lifecycle guarantees a clean re-initialization on every area transition.
✅ Actions performed

Full review triggered.

@LazyTrain082
Copy link
Copy Markdown
Contributor Author

@coderabbitai Okay actual re-review now that we're not rate limited

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 4, 2026

@LazyTrain082 No problem! Now that the rate limit has lifted, I'll kick off a proper full review of the PR right away. 🐇


🧠 Learnings used
Learnt from: zunath
Repo: zunath/SWLOR_NWN PR: 1958
File: SWLOR.Game.Server/Feature/GuiDefinition/ViewModel/DMToolsViewModel.cs:770-770
Timestamp: 2026-04-04T17:02:24.442Z
Learning: Preference: For DM/admin tools in SWLOR.Game.Server, favor code-level concurrency guards over relying on manual DM coordination. In DMToolsViewModel, use placeable-local edit claims (DMTOOLS_EDITING_DM_ID/NAME/TS) with stale timeout and release on window close to prevent multi-DM overwrite.

Learnt from: LazyTrain082
Repo: zunath/SWLOR_NWN PR: 1958
File: SWLOR.Game.Server/Feature/GuiDefinition/ViewModel/DMToolsViewModel.cs:308-312
Timestamp: 2026-04-04T16:54:53.747Z
Learning: In SWLOR.Game.Server (C#), NUI windows (including DMToolsViewModel / GuiWindowType.DMTools) automatically close when a DM jumps to a different area. This means cached per-area state (e.g., `_layoutIds`, `_allPlaceables`) is always rebuilt fresh via `Initialize()` when the window is reopened in the new area. Do not flag stale-cache concerns for area-scoped data in this view-model, as the window lifecycle guarantees a clean re-initialization on every area transition.
✅ Actions performed

Full review triggered.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
SWLOR.Game.Server/Feature/GuiDefinition/ViewModel/DMToolsViewModel.cs (2)

747-758: Consider logging caught exceptions before discarding.

The catch-all silently swallows exceptions, which could hide real issues. Per coding guidelines, Serilog should be used for logging.

♻️ Proposed improvement
             try
             {
                 DB.Delete<DMAreaPlaceableLayout>(layoutId);
             }
             catch
+            catch (Exception ex)
             {
+                Log.Warning(ex, "Failed to delete layout {LayoutId}", layoutId);
                 LayoutName = string.Empty;
                 LoadLayouts();
                 Instructions = "Unable to delete layout. It may have already been removed.";
                 InstructionColor = GuiColor.Red;
                 return;
             }

Add using Serilog; at the top of the file.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@SWLOR.Game.Server/Feature/GuiDefinition/ViewModel/DMToolsViewModel.cs` around
lines 747 - 758, The catch-all around DB.Delete<DMAreaPlaceableLayout>(layoutId)
is currently swallowing exceptions; add logging via Serilog instead of
discarding the exception: add "using Serilog;" to the file, catch the exception
as a variable (e.g., catch(Exception ex)) and call Log.Error/existing logger
with context (e.g., Log.Error(ex, "Failed to delete DMAreaPlaceableLayout
{LayoutId}"), then keep the existing recovery steps: clear LayoutName, call
LoadLayouts(), set Instructions and InstructionColor to GuiColor.Red and return.
Ensure the logged message includes layoutId and the exception.

855-873: Redundant GetSelectedPlaceable() calls in rotation methods.

Both OnRotateClockwise and OnRotateCounterClockwise call GetSelectedPlaceable() to get the facing, then AdjustFacing calls it again internally. This is minor but could be streamlined.

♻️ Proposed refactor
-        private void AdjustFacing(float facing)
+        private void AdjustFacing(float facing, uint placeable = OBJECT_INVALID)
         {
-            var placeable = GetSelectedPlaceable();
-            if (!GetIsObjectValid(placeable))
-                return;
+            if (!GetIsObjectValid(placeable))
+            {
+                placeable = GetSelectedPlaceable();
+                if (!GetIsObjectValid(placeable))
+                    return;
+            }
             if (!EnsureEditingOwnership(placeable))
                 return;

             facing = NormalizeFacing(facing);
             AssignCommand(placeable, () => SetFacing(facing));
         }

         public Action OnRotateClockwise() => () =>
         {
             var placeable = GetSelectedPlaceable();
             if (!GetIsObjectValid(placeable))
                 return;

             var facing = GetFacing(placeable) + 20f;
-            AdjustFacing(facing);
+            AdjustFacing(facing, placeable);
         };

         public Action OnRotateCounterClockwise() => () =>
         {
             var placeable = GetSelectedPlaceable();
             if (!GetIsObjectValid(placeable))
                 return;

             var facing = GetFacing(placeable) - 20f;
-            AdjustFacing(facing);
+            AdjustFacing(facing, placeable);
         };
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@SWLOR.Game.Server/Feature/GuiDefinition/ViewModel/DMToolsViewModel.cs` around
lines 855 - 873, The rotation handlers call GetSelectedPlaceable() twice (once
in the handler and again inside AdjustFacing); to remove the redundant call,
capture the selected placeable once in OnRotateClockwise and
OnRotateCounterClockwise, validate it with GetIsObjectValid, compute the new
facing with GetFacing(placeable) ±20f, and then call a single AdjustFacing
variant that uses that same placeable; implement this by adding an
AdjustFacing(NwPlaceable placeable, float facing) overload (or changing
AdjustFacing to accept an optional placeable parameter) and update both rotation
methods to call that overload, referencing OnRotateClockwise,
OnRotateCounterClockwise, GetSelectedPlaceable, GetIsObjectValid, GetFacing, and
AdjustFacing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@SWLOR.Game.Server/Feature/DMActions.cs`:
- Around line 18-22: The current early return when
EventsPlugin.GetEventData("OBJECT_TYPE") fails prevents the subsequent creature
droppable-flag logic from running; instead, parse OBJECT_TYPE into rawObjectType
but do not return on failure—either set a sentinel (e.g., rawObjectType = -1) or
only cast to objectType when TryParse succeeds, and allow the code that uses
GetObjectType and the creature inventory/droppable flag logic (the block after
the OBJECT_TYPE handling) to run regardless; update the code around
EventsPlugin.GetEventData("OBJECT_TYPE"), rawObjectType, and objectType so
failure to parse does not cause an immediate return.

---

Nitpick comments:
In `@SWLOR.Game.Server/Feature/GuiDefinition/ViewModel/DMToolsViewModel.cs`:
- Around line 747-758: The catch-all around
DB.Delete<DMAreaPlaceableLayout>(layoutId) is currently swallowing exceptions;
add logging via Serilog instead of discarding the exception: add "using
Serilog;" to the file, catch the exception as a variable (e.g., catch(Exception
ex)) and call Log.Error/existing logger with context (e.g., Log.Error(ex,
"Failed to delete DMAreaPlaceableLayout {LayoutId}"), then keep the existing
recovery steps: clear LayoutName, call LoadLayouts(), set Instructions and
InstructionColor to GuiColor.Red and return. Ensure the logged message includes
layoutId and the exception.
- Around line 855-873: The rotation handlers call GetSelectedPlaceable() twice
(once in the handler and again inside AdjustFacing); to remove the redundant
call, capture the selected placeable once in OnRotateClockwise and
OnRotateCounterClockwise, validate it with GetIsObjectValid, compute the new
facing with GetFacing(placeable) ±20f, and then call a single AdjustFacing
variant that uses that same placeable; implement this by adding an
AdjustFacing(NwPlaceable placeable, float facing) overload (or changing
AdjustFacing to accept an optional placeable parameter) and update both rotation
methods to call that overload, referencing OnRotateClockwise,
OnRotateCounterClockwise, GetSelectedPlaceable, GetIsObjectValid, GetFacing, and
AdjustFacing.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 037168d0-0ebe-4bbf-adc7-2a4720afccfc

📥 Commits

Reviewing files that changed from the base of the PR and between fe44143 and 6c6d12b.

📒 Files selected for processing (5)
  • SWLOR.Game.Server/Entity/DMAreaPlaceableLayout.cs
  • SWLOR.Game.Server/Feature/ChatCommandDefinition/DMChatCommand.cs
  • SWLOR.Game.Server/Feature/DMActions.cs
  • SWLOR.Game.Server/Feature/GuiDefinition/DMToolsDefinition.cs
  • SWLOR.Game.Server/Feature/GuiDefinition/ViewModel/DMToolsViewModel.cs

Comment thread SWLOR.Game.Server/Feature/DMActions.cs Outdated
@LazyTrain082
Copy link
Copy Markdown
Contributor Author

@zunath Merged and ready to party

@zunath
Copy link
Copy Markdown
Owner

zunath commented Apr 4, 2026

@coderabbitai - Are previous discussions resolved?

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 4, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get the current state of the PR
gh pr view 1958 --repo zunath/SWLOR_NWN --json state,title,mergedAt,reviewDecision

# Get all review threads and their resolved status using GraphQL
gh api graphql -f query='
{
  repository(owner: "zunath", name: "SWLOR_NWN") {
    pullRequest(number: 1958) {
      state
      reviewThreads(first: 50) {
        nodes {
          id
          isResolved
          isOutdated
          comments(first: 1) {
            nodes {
              author {
                login
              }
              body
              createdAt
              path
              line
            }
          }
        }
      }
    }
  }
}'

Length of output: 20375


@zunath Here's a full status breakdown of all review threads on this PR:


✅ Resolved (4 threads)

Thread Issue
Layout name trimming before save/match Addressed in commits 0846b81–6c6d12b
DM-possessed actor resolution for DM_SPAWNED_BY* Addressed in commits e63cfb9–806146d
Capture selected layout snapshot before delete modal Addressed in commits e63cfb9–806146d
Early return in DMActions.cs skipping creature droppable-flag logic Addressed in commit 0d1a409

⚠️ Unresolved (5 threads)

Outdated (code at those lines changed, but threads were never explicitly closed):

  1. /dmtools command name too generic — You suggested using plctools or something less generic. The code changed in that area but the thread was not resolved.
  2. Command available to all on test environment — You noted you don't want this available to everyone on the test environment. Same situation — code changed but thread is open.

Still active (code unchanged at flagged lines):

  1. Multi-DM concurrent area modification (your own question to me) — You asked about shortcomings when multiple DMs modify the same area at the same time. The existing concurrency guards (placeable-local edit claims with stale timeout, per-area DB save lock) help but there are still race windows, particularly around layout load/save and placeable spawning.

  2. GetSelectedPlaceable() — no area validation on cached placeable — The nudge/facing/delete actions don't verify the cached placeable is still in GetArea(Player). Note: Per the recorded learning, the NUI window closes when a DM changes areas, which mitigates this significantly — but the code path itself has no explicit guard if the placeable is somehow from the wrong area.

  3. _layoutIds stale-area revalidation before load/delete — The cached layout list isn't refreshed or cross-checked against GetAreaResref() before load/delete operations. Same mitigation applies (window closes on area change), but the code has no explicit guard.


In short: The 3 CodeRabbit/bot issues were all addressed. The 2 outdated zunath threads need explicit resolution (confirm in code or close manually). The 3 active threads—including your own multi-DM question—still have open items. Would you like me to do a deeper analysis on the multi-DM concurrency gaps, or help resolve any of these threads?


🧠 Learnings used
Learnt from: zunath
Repo: zunath/SWLOR_NWN PR: 1958
File: SWLOR.Game.Server/Feature/GuiDefinition/ViewModel/DMToolsViewModel.cs:770-770
Timestamp: 2026-04-04T17:02:24.442Z
Learning: Preference: For DM/admin tools in SWLOR.Game.Server, favor code-level concurrency guards over relying on manual DM coordination. In DMToolsViewModel, use placeable-local edit claims (DMTOOLS_EDITING_DM_ID/NAME/TS) with stale timeout and release on window close to prevent multi-DM overwrite.

Learnt from: LazyTrain082
Repo: zunath/SWLOR_NWN PR: 1958
File: SWLOR.Game.Server/Feature/GuiDefinition/ViewModel/DMToolsViewModel.cs:308-312
Timestamp: 2026-04-04T16:54:53.747Z
Learning: In SWLOR.Game.Server (C#), NUI windows (including DMToolsViewModel / GuiWindowType.DMTools) automatically close when a DM jumps to a different area. This means cached per-area state (e.g., `_layoutIds`, `_allPlaceables`) is always rebuilt fresh via `Initialize()` when the window is reopened in the new area. Do not flag stale-cache concerns for area-scoped data in this view-model, as the window lifecycle guarantees a clean re-initialization on every area transition.

@LazyTrain082
Copy link
Copy Markdown
Contributor Author

Tip

For best results, initiate chat on the files or code changes.
🧩 Analysis chain

@zunath Here's a full status breakdown of all review threads on this PR:

✅ Resolved (4 threads)

Thread Issue
Layout name trimming before save/match Addressed in commits 0846b81–6c6d12b
DM-possessed actor resolution for DM_SPAWNED_BY* Addressed in commits e63cfb9–806146d
Capture selected layout snapshot before delete modal Addressed in commits e63cfb9–806146d
Early return in DMActions.cs skipping creature droppable-flag logic Addressed in commit 0d1a409

⚠️ Unresolved (5 threads)

Outdated (code at those lines changed, but threads were never explicitly closed):

1. **`/dmtools` command name too generic** — You suggested using `plctools` or something less generic. The code changed in that area but the thread was not resolved.

2. **Command available to all on test environment** — You noted you don't want this available to everyone on the test environment. Same situation — code changed but thread is open.

Still active (code unchanged at flagged lines):

3. **Multi-DM concurrent area modification** _(your own question to me)_ — You asked about shortcomings when multiple DMs modify the same area at the same time. The existing concurrency guards (placeable-local edit claims with stale timeout, per-area DB save lock) help but there are still race windows, particularly around layout load/save and placeable spawning.

4. **`GetSelectedPlaceable()` — no area validation on cached placeable** — The nudge/facing/delete actions don't verify the cached placeable is still in `GetArea(Player)`. _Note: Per the recorded learning, the NUI window closes when a DM changes areas, which mitigates this significantly — but the code path itself has no explicit guard if the placeable is somehow from the wrong area._

5. **`_layoutIds` stale-area revalidation before load/delete** — The cached layout list isn't refreshed or cross-checked against `GetAreaResref()` before load/delete operations. Same mitigation applies (window closes on area change), but the code has no explicit guard.

In short: The 3 CodeRabbit/bot issues were all addressed. The 2 outdated zunath threads need explicit resolution (confirm in code or close manually). The 3 active threads—including your own multi-DM question—still have open items. Would you like me to do a deeper analysis on the multi-DM concurrency gaps, or help resolve any of these threads?
🧠 Learnings used

@coderabbitai

eaa316f

and

fc96659

Should be the fixes to Zunath's original question. I just forgot to resolve the threads. Double check those cover the issues.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 4, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check what files/changes are in commit eaa316f
gh api repos/zunath/SWLOR_NWN/commits/eaa316fcb32acf22bcaa95c7fa7dd22d91848963 --jq '.files[] | {filename, status, additions, deletions, patch}'

Length of output: 6885


🏁 Script executed:

#!/bin/bash
# Check what files/changes are in commit fc96659
gh api repos/zunath/SWLOR_NWN/commits/fc966595e897f2b94205c02ab9ee8c111557d7d2 --jq '.files[] | {filename, status, additions, deletions, patch}'

Length of output: 9411


@LazyTrain082 I've reviewed both commits. Here's the breakdown:


✅ Commit eaa316f — Addresses threads 1 & 2 + strengthens layout concurrency

Unresolved Thread Status
1. Command name too generic Fully fixed — primary command is now plctools, alias placeabletools
2. Available to all on test env Fully fixed.AvailableToAllOnTestEnvironment() removed
3. Multi-DM concurrent saves Improved — save lock moved from Player-local → area-object-local with try/finally guarantee; load cooldown also moved to area-local so it throttles all DMs on that area, not just the one who loaded

✅ Commit fc96659 — Addresses thread 3 (edit claims) + hardens delete

Unresolved Thread Status
3. Multi-DM concurrent edits Fully covered — full per-placeable edit claim system added (DMTOOLS_EDITING_DM_ID/NAME/LAST_TS, 120s stale timeout). Every mutating action (AdjustPosition, OnZAxisReset, AdjustFacing, OnDeletePlaceable) now calls EnsureEditingOwnership(). Claim is acquired on OnSelectPlaceable() and released in ClearHighlight() / on selection switch.
4. GetSelectedPlaceable() area validation Effectively mitigatedEnsureEditingOwnership() gates all mutations. The remaining no-area-check in GetSelectedPlaceable() itself is acceptable given the window-close-on-area-change lifecycle guarantee.
5. _layoutIds stale revalidation Effectively mitigated — explicit area validity check added before load/save, and the window lifecycle still guarantees a clean re-Initialize() on every area transition.

Summary: All five unresolved threads are covered by these two commits. Threads 1 & 2 are fully and cleanly fixed. Thread 3 (multi-DM concurrency) is now comprehensively handled end-to-end between the two commits. Threads 4 & 5 are sufficiently mitigated via the new ownership checks plus the existing window lifecycle behaviour. You're good to go! 🐇✨


🧠 Learnings used
Learnt from: zunath
Repo: zunath/SWLOR_NWN PR: 1958
File: SWLOR.Game.Server/Feature/GuiDefinition/ViewModel/DMToolsViewModel.cs:770-770
Timestamp: 2026-04-04T17:02:24.442Z
Learning: Preference: For DM/admin tools in SWLOR.Game.Server, favor code-level concurrency guards over relying on manual DM coordination. In DMToolsViewModel, use placeable-local edit claims (DMTOOLS_EDITING_DM_ID/NAME/TS) with stale timeout and release on window close to prevent multi-DM overwrite.

Learnt from: LazyTrain082
Repo: zunath/SWLOR_NWN PR: 1958
File: SWLOR.Game.Server/Feature/GuiDefinition/ViewModel/DMToolsViewModel.cs:308-312
Timestamp: 2026-04-04T16:54:53.747Z
Learning: In SWLOR.Game.Server (C#), NUI windows (including DMToolsViewModel / GuiWindowType.DMTools) automatically close when a DM jumps to a different area. This means cached per-area state (e.g., `_layoutIds`, `_allPlaceables`) is always rebuilt fresh via `Initialize()` when the window is reopened in the new area. Do not flag stale-cache concerns for area-scoped data in this view-model, as the window lifecycle guarantees a clean re-initialization on every area transition.

Learnt from: zunath
Repo: zunath/SWLOR_NWN PR: 1957
File: Module/ifo/module.ifo.json:2899-2899
Timestamp: 2026-03-29T13:23:17.045Z
Learning: In the SWLOR_NWN repository, `Module/ifo/module.ifo.json` has been historically corrupted by PRs that introduced truncated or missing lines (e.g., incomplete entries in `Mod_Area_list`). On every PR touching this file, always: (1) verify the file is valid, well-formed JSON; (2) check every entry in `Mod_Area_list` has a complete `__struct_id` and `Area_Name` block with both `type` and `value` fields; (3) flag any truncated, incomplete, or empty entries; (4) flag any removals from `Mod_Area_list` unless explicitly confirmed as intentional by the PR author.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants