feat(Building): interior systems, NPC navigation, terrain prep, and post-build customization - #6
Merged
Merged
Conversation
…dges in TerrainClearer
…penStringer variations
…dd IsLivingEntity utility
|
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 building part registry, richer wall/opening APIs, interior-wall and interior-navigation systems (A* grid + navigator), roof/stair/decor builders, terrain clearing/flattening with retry, deferred networked-prefab linking, dual-material walls, and many builder API extensions and material-registration hooks. Changes
Sequence Diagram(s)sequenceDiagram
actor Builder as BuildingBuilder
participant Terrain as TerrainFlattener
participant NavBuilder as NavigationBuilder
participant Grid as InteriorPathGrid
participant NavCore as InteriorNavigatorCore
participant NPC as NPCMovement
Builder->>Terrain: FlattenUnder(targetY,...)
Terrain->>Terrain: locate Terrain, modify heights, flush
Builder->>NavBuilder: CreateNavigationBuilder()
NavBuilder->>Grid: Construct grid from room & doorways
Grid->>Grid: Generate walkability (walls, openings, physics probes)
NavBuilder->>NavCore: Build core + attach navigator
NavCore->>NavCore: install patches (intercept NPC.SetDestination)
NPC->>NavCore: SetDestination(interiorTarget)
NavCore->>Grid: FindPath(exterior->doorway)
Grid-->>NavCore: waypoints
NavCore->>NPC: approach doorway, lerp through (handle stairs)
NavCore->>Grid: FindPath(doorway->interiorTarget)
Grid-->>NavCore: interior waypoints
NavCore->>NPC: traverse interior waypoints, trigger arrival
sequenceDiagram
actor Client as FishNetClient
participant Placer as PrefabPlacer
participant Queue as NetworkedPrefabLinkerQueue
participant Scene as SceneRoot
participant Parent as BuildingParent
Client->>Placer: Place(prefab, pos, networked=true, onReady)
Placer->>Placer: InstantiateNetworkedInactive(...)
Placer-->>Client: null (client cannot spawn)
Placer->>Queue: Enqueue(prefabName, expectedPos, parent, localPose, onLinked)
Queue->>Queue: Ensure behaviour, schedule polls
Note over Scene: FishNet replicates instance later
Queue->>Scene: Tick(): scan scene roots by prefab-name & pos
Scene-->>Queue: Match found (pos/name)
Queue->>Parent: Reparent instance, set local transform
Queue->>Client: Invoke onLinked/onReady
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (6)
ProceduralMesh/PrimitiveBuilder.cs-273-275 (1)
273-275:⚠️ Potential issue | 🟡 MinorBehavioral change: shadow default changed from
SofttoNoneaffects all callers.All three existing calls to
CreatePointLightinLightingBuilder.cs(ceiling lights at line 70, point light at line 106, ambient lights at line 146) will now default toLightShadows.Noneinstead of the previous hardcodedLightShadows.Soft. None of these callers pass the newshadowsparameter, so they will silently adopt the new behavior.For ceiling and ambient lights,
Noneis appropriate and aligns with URP conventions. However, this is still a behavioral change that could affect visual output. Consider documenting this shift in migration notes or preserving the previous default (LightShadows.Soft) for backward compatibility, withNoneused explicitly where needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ProceduralMesh/PrimitiveBuilder.cs` around lines 273 - 275, The default for the shadows parameter on CreatePointLight changed from LightShadows.Soft to LightShadows.None, which silently alters behavior for all callers that don't pass shadows (e.g., the ceiling/point/ambient lights in LightingBuilder); either restore the original default to LightShadows.Soft in the CreatePointLight signature to preserve backward compatibility, or explicitly update each caller (the three CreatePointLight usages in LightingBuilder) to pass the intended LightShadows value (None for ceiling/ambient, Soft where previously used), and add a brief migration note documenting the change if you keep the new default.Core/PrefabRef.cs-150-153 (1)
150-153:⚠️ Potential issue | 🟡 MinorXML doc contradicts actual parent timing.
Line 150 says parent is set before spawn, but the implementation parents after spawn (Lines 209-215). Please align the docs with current behavior.
Also applies to: 209-215
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/PrefabRef.cs` around lines 150 - 153, The XML doc for the PrefabRef method that accepts (Transform parent, Vector3 localPosition, Quaternion localRotation) incorrectly states the parent is set before spawn; update the summary/param comment to reflect that the parent is assigned after the object is spawned (matching the implementation where parent is applied post-spawn in the PrefabRef method body around the block that handles parenting on lines 209-215). Ensure the param description for "parent" and any return remarks explicitly say the transform is parented after instantiation/network-spawn so docs match the code.Building/Config/BuildingPalette.cs-26-32 (1)
26-32:⚠️ Potential issue | 🟡 MinorThese XML docs contradict each other.
Lines 26-28 say
InteriorWallMaterial == nullfalls back to a single-material wall, but Lines 30-32 sayInteriorWallColoris used in exactly that case. Generated docs will advertise both behaviors at once.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Building/Config/BuildingPalette.cs` around lines 26 - 32, The XML comments for InteriorWallMaterial and InteriorWallColor conflict; reconcile them so they describe the same fallback behavior. Update the comment on InteriorWallMaterial and/or InteriorWallColor so they state a single consistent rule (e.g., "If InteriorWallMaterial is null, exterior walls use InteriorWallColor; when InteriorWallColor is null it defaults to WallColor" or the alternative chosen behavior), referencing the properties InteriorWallMaterial, InteriorWallColor and WallColor so both summaries match.docs/examples.md-229-247 (1)
229-247:⚠️ Potential issue | 🟡 MinorCall out the server/host requirement before the
networked: trueplacements.This example is easy to copy into client-side startup code, but the networking overview later treats networked prefab spawning as server-authoritative. Add that precondition here so readers do not reproduce the multiplayer failure mode from the example itself.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/examples.md` around lines 229 - 247, The example calls PrefabPlacer.Place with networked: true (e.g., PrefabPlacer.Place and the Prefabs.MetalGlassDoor / Prefabs.ClassicalWoodenDoor usages) but doesn’t state the server/host requirement; update the example text to explicitly require running these networked placements only on the server/host (or when isServer/isHost is true) and add a short guard comment above the two Place calls indicating they must be executed on the authoritative server/host to ensure FishNet server-authoritative spawning.Building/Components/NetworkedPrefabLinker.cs-176-197 (1)
176-197:⚠️ Potential issue | 🟡 MinorSubstring matching may link unintended objects.
The
obj.name.Contains(req.PrefabName)check on Line 179 could match unrelated objects. For example, a request for"Door"would also match"DoorFrame","SlidingDoors", or"BackDoor".Consider using a more precise match (e.g., exact name or starts-with) and/or verifying the object has a
NetworkObjectcomponent:🛡️ Suggested stricter matching
- if (!obj.name.Contains(req.PrefabName)) continue; + // Match by exact name or name(Clone) pattern from Instantiate + if (!obj.name.Equals(req.PrefabName) && + !obj.name.StartsWith(req.PrefabName + "(Clone)") && + !obj.name.StartsWith(req.PrefabName + " (")) + continue;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Building/Components/NetworkedPrefabLinker.cs` around lines 176 - 197, The current substring match using obj.name.Contains(req.PrefabName) can link unintended objects; update the matching in the loop to require a stricter check (e.g., obj.name == req.PrefabName or obj.name.StartsWith(req.PrefabName, StringComparison.Ordinal) depending on desired behavior) and additionally verify the candidate has a NetworkObject component (e.g., TryGetComponent<NetworkObject>(out _)) before considering it for PositionTolerance / bestDist logic so only valid networked prefabs are selected; keep the existing distance/tolerance logic (PositionTolerance, bestMatch, bestDist) otherwise.Building/Structural/RoofBuilder.cs-178-186 (1)
178-186:⚠️ Potential issue | 🟡 MinorApply the resolved slab material.
roofSlabMatis selected here but never assigned to the slab, so textured/shader overrides collapse to a plain colored box.Possible fix
- PrimitiveBuilder.CreateBox("ParapetRoofSlab", + GameObject roofSlab = PrimitiveBuilder.CreateBox("ParapetRoofSlab", new Vector3(_roomSize.x / 2f, _roomSize.y + roofSlabThickness / 2f, _roomSize.z / 2f), new Vector3(_roomSize.x, roofSlabThickness, _roomSize.z), roofSlabMat.color, container.transform); + ApplyMaterial(roofSlab, roofSlabMat);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Building/Structural/RoofBuilder.cs` around lines 178 - 186, The code computes roofSlabMat but only passes roofSlabMat.color into PrimitiveBuilder.CreateBox, losing material/texture/shader info; update the call that creates "ParapetRoofSlab" to apply the full Material (roofSlabMat) instead of just its color—either use the CreateBox overload that accepts a Material or capture the returned GameObject from PrimitiveBuilder.CreateBox and assign roofSlabMat to its Renderer.material (referencing roofSlabMat and the "ParapetRoofSlab" CreateBox invocation).
🧹 Nitpick comments (10)
Building/Components/LightingBuilder.cs (1)
61-61: Consider extracting the ceiling offset toConstants.cs.The change from 0.2f to 0.3f is reasonable and well-documented. Since the PR introduces centralized tuning constants in
Constants.cs, this ceiling light offset (and potentially the 0.5f used inAddAmbientLighting) could be moved there for easier future adjustments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Building/Components/LightingBuilder.cs` at line 61, Extract the magic ceiling offset values into the centralized Constants.cs: replace the hardcoded 0.3f used when computing yPos in LightingBuilder (the line setting float yPos = _roomSize.y - 0.3f) with a descriptive constant (e.g., CeilingLightOffset) and also move the 0.5f used in AddAmbientLighting into Constants.cs (e.g., AmbientLightHeightOffset); update references in LightingBuilder and AddAmbientLighting to use those new constants and ensure names clearly reflect purpose for easy tuning.Building/Config/BuildingPart.cs (1)
7-28: Reserve0for an unspecified enum value.
default(BuildingPart)currently resolves toExteriorWalls. Any uninitialized field, omitted serialized value, or defaulted array element will silently target a real category and can mis-register or repaint geometry. AddNone = 0and start the concrete parts at1.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Building/Config/BuildingPart.cs` around lines 7 - 28, The BuildingPart enum currently has no explicit zero value so default(BuildingPart) equals ExteriorWalls; add an explicit None = 0 member and renumber the concrete parts to start at 1 (e.g., set ExteriorWalls = 1 and increment the rest) so uninitialized/omitted values map to None instead of a real part; update any switch/default handling or serialization assumptions that rely on implicit ordering if present (refer to the BuildingPart enum declaration and any consumers that switch on BuildingPart).ProceduralMesh/ProceduralMeshBuilder.cs (1)
220-221: Don't hard-code every generated mesh into all rendering layers.This forces every procedural mesh to match every layer-masked light/decal, so callers lose rendering-layer isolation. Prefer a builder option for the mask, or keep Unity's default unless the caller explicitly opts into all layers. The same concern applies to
CustomMeshBuilder.Build().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ProceduralMesh/ProceduralMeshBuilder.cs` around lines 220 - 221, The code currently forces every generated mesh into all rendering layers by setting MeshRenderer.renderingLayerMask = uint.MaxValue in ProceduralMeshBuilder (and similarly in CustomMeshBuilder.Build()); change this to leave the renderer's default mask unless the builder explicitly receives an option to override it (e.g., expose a renderingLayerMask property/parameter on ProceduralMeshBuilder and CustomMeshBuilder.Build), and only assign renderer.renderingLayerMask when that option is provided by the caller so generated meshes retain Unity's default layer isolation unless explicitly requested.Building/Structural/TerrainRetryBehaviour.cs (1)
36-51: Closure captures may reference destroyed objects.When
TerrainFlattener.FlattenUnderenqueues a retry, the lambda capturesbuildingRoot. If the buildingGameObjectis destroyed before the retry executes, the captured reference becomes invalid (Unity returnsnullfor destroyed objects, but the closure still holds it).Consider adding a null-check on
targetat the start ofTickor within the queued operation itself to gracefully skip retries for destroyed buildings.🛡️ Suggested defensive check in Tick
internal static bool Tick(int id, string buildingName) { if (!States.TryGetValue(id, out var state) || state.Ops.Count == 0) { States.Remove(id); return false; } + + // Early exit if the building was destroyed + // (buildingName will be null or empty for destroyed objects) + if (string.IsNullOrEmpty(buildingName)) + { + DebugLog.Info("[TerrainRetry] Building destroyed — clearing pending ops."); + States.Remove(id); + return false; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Building/Structural/TerrainRetryBehaviour.cs` around lines 36 - 51, The queued lambda can hold a reference to a destroyed GameObject (target/buildingRoot), so update TerrainRetryBehaviour to skip retries when the target has been destroyed: in the Tick method (or just before invoking each queued operation stored in RetryState.Ops) check the associated GameObject reference (the same target passed into Enqueue / found via TerrainRetryBehaviour.EnsureOn) using UnityEngine.Object null semantics (target == null) and remove/skip any operations for destroyed targets instead of invoking them; ensure you adjust RetryState handling so Ops count and States cleanup reflect skipped/dropped operations.Building/NavigationBuilder.cs (1)
306-333: Consider disabling carving when building is deactivated.The
NavMeshObstaclewithcarving=truewill continue carving the NavMesh even if the building GameObject is deactivated (only destroyed objects stop carving). If buildings can be toggled inactive, this could leave phantom "holes" in the NavMesh.If building deactivation is a supported use case, consider adding an
OnDisable/OnEnablehook inInteriorNavigatorto toggleNavMeshObstacle.carving.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Building/NavigationBuilder.cs` around lines 306 - 333, The NavMeshObstacle created in PlaceFootprintCarvingObstacle uses carving=true and will keep carving when its GameObject is deactivated; add logic to toggle obstacle.carving on enable/disable so deactivated buildings don't leave NavMesh holes. Specifically, update InteriorNavigator (or the component that manages _carvingObstacles created by PlaceFootprintCarvingObstacle) to implement OnDisable and OnEnable handlers that iterate _carvingObstacles (the GameObjects returned from PlaceFootprintCarvingObstacle), get their NavMeshObstacle components, and set obstacle.carving = false in OnDisable and obstacle.carving = true in OnEnable (guarding for nulls), ensuring carving is restored only for active buildings.Building/Components/NetworkedPrefabLinker.cs (1)
199-228: Consider checking if the matched object is already parented.If a matched object is already parented to another transform (e.g., a different building instance), reparenting it could cause unexpected behavior. A simple guard would prevent stealing objects from other hierarchies:
🛡️ Suggested parent check
if (bestMatch != null) { + // Don't steal objects already parented elsewhere + if (bestMatch.transform.parent != null) + { + DebugLog.Warning($"[PrefabLinker] '{bestMatch.name}' already has parent " + + $"'{bestMatch.transform.parent.name}' — skipping."); + return false; + } + if (bestDist > PositionTolerance)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Building/Components/NetworkedPrefabLinker.cs` around lines 199 - 228, bestMatch may already be parented to a different Transform, so add a guard before reparenting: check bestMatch.transform.parent (or bestMatch.transform.IsChildOf(req.Parent)) and if it is non-null and not the same as req.Parent, skip linking (log an Info/Warning and return false or continue searching) to avoid stealing objects from other hierarchies; place this check just before bestMatch.transform.SetParent(req.Parent) in the matching block that uses bestMatch, req.Parent, PositionTolerance, attempts and still invoke req.OnLinked only when you actually perform the reparenting.Building/Components/PrefabPlacer.cs (1)
64-80: Consider simplifying callback composition.The current if-else chain for combining
enableComponentsandonReadyis verbose. A more concise approach:♻️ Simplified callback composition
- Action<GameObject>? combined = null; - if (enableComponents && onReady != null) - { - combined = (go) => { go.EnableAllComponents(recursive: true); onReady(go); }; - } - else if (enableComponents) - { - combined = (go) => go.EnableAllComponents(recursive: true); - } - else - { - combined = onReady; - } + Action<GameObject>? combined = enableComponents + ? (go) => { go.EnableAllComponents(recursive: true); onReady?.Invoke(go); } + : onReady;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Building/Components/PrefabPlacer.cs` around lines 64 - 80, The Place method builds a combined callback with verbose if/else; simplify by creating a single lambda assigned to combined that, when invoked, calls go.EnableAllComponents(recursive: true) if enableComponents is true and then calls onReady(go) if onReady is not null, otherwise set combined = onReady when enableComponents is false, then pass combined into PlaceInternal; update references to Place, PlaceInternal and EnableAllComponents accordingly so behavior is identical but code is more concise.Building/InteriorPathGrid.cs (2)
440-447:DuplicateKeyComparertrick is clever but subtle.Returning
1for equal keys allows duplicate f-scores but breaks theIComparercontract (transitivity). This works forSortedListbut could cause issues if the comparer is reused elsewhere.Consider adding a comment explaining this intentional contract violation:
📝 Suggested clarifying comment
/// <summary> /// Comparer that allows duplicate keys in SortedList. + /// WARNING: Violates IComparer contract by never returning 0. + /// Only safe for use with SortedList as a pseudo priority queue. /// </summary> private sealed class DuplicateKeyComparer : IComparer<float>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Building/InteriorPathGrid.cs` around lines 440 - 447, DuplicateKeyComparer currently returns 1 for equal floats to allow duplicate f-scores but intentionally violates IComparer transitivity; update the Compare method (DuplicateKeyComparer.Compare) with a clear comment explaining that returning 1 is a deliberate workaround to allow duplicate keys in the SortedList used by InteriorPathGrid and that this comparer must not be reused elsewhere (or consider replacing with a different data structure if broader use is needed). Keep the class sealed/private scope note and mention the specific behavior (treat equal as greater) and the risk (breaks IComparer contract/transitivity) so future maintainers understand the intentional tradeoff.
366-414: SortedList as priority queue has O(n) removal cost.
SortedList.RemoveAt(0)is O(n) because it shifts all remaining elements. For pathfinding on larger grids, this could become a bottleneck. The comment on Line 366 acknowledges this is "fine for few hundred cells."If grids grow larger, consider switching to a proper min-heap (e.g.,
PriorityQueue<TElement, TPriority>in .NET 6+, or a custom binary heap for earlier targets).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Building/InteriorPathGrid.cs` around lines 366 - 414, The current A* uses a SortedList named open with DuplicateKeyComparer and calls open.RemoveAt(0) which is O(n); replace this with a proper min-heap (preferably System.Collections.Generic.PriorityQueue<int, float> when targeting .NET 6+) or a small custom binary heap class: create a PriorityQueue<int,float> open, use open.Enqueue(startIdx, fScore[startIdx]) instead of open.Add(...), use open.TryDequeue(out int currentIdx, out float _ ) (or Dequeue) to pop the lowest-f node, keep the existing closed[] check immediately after dequeuing, and when updating a neighbor call open.Enqueue(neighborIdx, fScore[neighborIdx]) (or your heap.Push) instead of open.Add; remove the DuplicateKeyComparer and any logic dependent on SortedList ordering. Ensure all references to open.RemoveAt(0) and open.Add(...) are replaced and that the algorithm still checks closed[neighborIdx] before processing.docs/building.md (1)
176-183: Consider adding error handling guidance for NavigationBuilder.The example shows
navBuilder.Build()but doesn't mention what happens if the building isn't positioned yet or ifBuild()is called multiple times. TheNavigationBuildercode logs a warning for double-builds. Consider adding a brief note:> **Note:** Call `Build()` only once after the building is positioned. > Use `Rebuild()` if interior objects change later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/building.md` around lines 176 - 183, Add a short note in the docs near the NavigationBuilder example clarifying lifecycle and error handling: explain that NavigationBuilder is obtained via buildingBuilder.CreateNavigationBuilder(), that navBuilder.Build() should be called once after the building is positioned (calling Build() multiple times triggers a logged warning), and that callers should use navBuilder.Rebuild() when interior objects change later instead of repeatedly calling Build().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Building/BuildingBuilder.cs`:
- Around line 150-152: GetDecorBuilder(...) returns a memoized DecorBuilder
whose constructor captures the palette, so calling AddFloor(...) or
AddCeiling(...) with a per-call palette override produces stale renders; change
BuildingBuilder to only apply the palette override on the first decor build and
ignore/remove per-call palette arguments for subsequent calls: locate
GetDecorBuilder, the calls to AddFloor(_config.FloorThickness) and
AddCeiling(...) and the DecorBuilder constructor usage, ensure the palette is
set once when the DecorBuilder is created and remove or bypass any palette
cloning/override logic on later AddFloor/AddCeiling invocations so subsequent
builds use the already-initialized DecorBuilder's palette.
- Around line 360-384: CreateNavigationBuilder can produce a doorway list with
zero exterior entries which later breaks NavigationBuilder; before constructing
NavigationBuilder, check the doorways list for at least one exterior doorway
(i.e., an entry where isInterior is false—inspect NavDoorwayInfo's
property/field that represents interior vs exterior or track the isInterior
constructor arg), and if none exist, fail early (throw a clear
InvalidOperationException or return an error) instead of constructing
NavigationBuilder; update CreateNavigationBuilder to perform this guard and
include a descriptive message referencing CreateNavigationBuilder and
NavigationBuilder.
- Around line 564-579: AddStairs currently only stores (wall, foundationHeight,
width, lateralOffset) so ComputeStairBasePosition reconstructs stair run with
defaults and ignores stepDepth, maxStepHeight, gap, flushWithFloor and style;
change the stored stair record (_stairs) to include the full stair specification
used in GetDecorBuilder().AddStairs (e.g., stepDepth, maxStepHeight,
color/material identifiers, style, flushWithFloor, gap) or create a
StairSpec/DTO and push that into _stairs, then update ComputeStairBasePosition
to read the real stair parameters from that StairSpec and compute the base/run
accordingly so navigation/doorway placement matches actual stair geometry.
Ensure any registry registration (_registry.Register(BuildingPart.Stairs,
stairs)) still happens but does not discard the full spec.
In `@Building/BuildingPartRegistry.cs`:
- Around line 205-208: The code currently checks only that submeshIndex <
mats.Length before assigning mats[submeshIndex] = material and updating
renderers[i].materials; add a lower-bound check so you only index when
submeshIndex >= 0 && submeshIndex < mats.Length (e.g. skip the assignment/update
when out of range) to prevent negative-index exceptions; locate the logic around
submeshIndex, mats, material and renderers[i].materials in BuildingPartRegistry
and apply this guard.
In `@Building/BuildingUtilities.cs`:
- Around line 143-149: ComputeGridCellSize currently returns a cell size that
may not evenly divide roomX or roomZ for non-square rooms (e.g., 10x7),
violating the implied contract; change the API to return both the computed cell
size and the integer cell counts so callers can reconstruct exact tiling. Update
the ComputeGridCellSize method signature to return a tuple or a small struct
(e.g., (float cellSize, int nx, int nz) or GridCellResult) or add out parameters
for nx and nz, compute nx = Mathf.Max(1, Mathf.CeilToInt(roomX / target)) and nz
= Mathf.Max(1, Mathf.CeilToInt(roomZ / target)), compute cellSize as
Mathf.Min(roomX / nx, roomZ / nz), and ensure callers of ComputeGridCellSize
(and any related doc/comments) are updated to use the returned counts instead of
recomputing them.
- Around line 158-170: The IsLivingEntity method currently treats any root with
an Animator as "living," which is too broad; modify IsLivingEntity to use a
stricter character test (e.g., check for game-specific character components such
as NPCMovement, CharacterController, PlayerCharacter, or other explicit
character marker components) instead of relying on Animator, and add an optional
override hook so callers can supply a predicate (or use an existing
ClearingOptions.Filter) to decide "living" status; update references to
livingRoots and staticRoots logic to only add rootId to livingRoots when the
stricter component check or caller predicate returns true, otherwise add to
staticRoots.
In `@Building/InteriorNavigator.cs`:
- Around line 20-29: Wrap calls to _core.Update() in Update() and
_core.Cleanup() in OnDestroy() with try/catch blocks that catch exceptions, log
the exception (including context) and then clear/null the _core reference and
disable further processing so a single failure doesn't repeat every frame;
specifically modify Update() to try { _core.Update(); } catch (Exception ex) {
processLogger?.Error/Debug with context; _core = null; } and modify OnDestroy()
to try { _core.Cleanup(); } catch (Exception ex) { log the error; } finally {
_core = null; } so teardown always completes and no repeated exceptions occur.
In `@Building/InteriorNavigatorCore.cs`:
- Around line 672-692: ReleaseAllNPCs currently omits restoring the
TrackedNPC.HasDestination flag (which BeginDoorwayEntry clears and ReleaseNPC
restores), causing NPC movement loops to remain disabled after bulk release;
update ReleaseAllNPCs (in the loop over _tracked) to set
kvp.Value.HasDestination = true (or use the same restore logic as ReleaseNPC)
for NPCs being released (e.g., before calling EnableAgent and re-enabling
colliders), ensuring NPCs removed in Cleanup regain their destination state.
- Around line 374-395: The new branch unconditionally registers the TrackedNPC
(from CreateTrackedNPC) into nav._tracked and _globallyManaged and returns false
even when tracked.Agent is null/disabled/off-navmesh, which deadlocks
UpdateApproaching; change the logic in the block that sets
tracked.TargetDoorway/State/etc. so you only add to nav._tracked and
_globallyManaged and return false when tracked.Agent != null &&
tracked.Agent.enabled && tracked.Agent.isOnNavMesh (i.e. the same condition used
for SetDestination); otherwise skip adding the tracked NPC (or let the original
move proceed) so the original movement is not suppressed when the agent is
unavailable.
- Around line 1113-1117: The double null check around data.ChaseTarget is
ineffective because both checks use Unity's overloaded null operator (so a
destroyed Transform makes the outer != null false and the inner == null never
runs); modify the logic in the method containing data.ChaseTarget to first cache
the raw reference (e.g., var chase = data.ChaseTarget) and then use
System.Object.ReferenceEquals(chase, null) for the existence test or explicitly
check for destroyed via chase == null vs ReferenceEquals to distinguish
destroyed instances, and call BeginExit(npc, data) when the cached reference is
destroyed or becomes null; update any related conditionals so BeginExit is
invoked correctly when the target was destroyed.
In `@Building/Structural/DecorBuilder.cs`:
- Around line 681-683: The calculation for tread vertical position uses "+
treadThickness / 2f" which places treads one full thickness too high; update the
computation of the variable treadY (and the same pattern at the other
occurrences around the open-stringer/closed-riser code) to subtract half the
tread thickness from the intended top surface instead (i.e., use "-
treadThickness / 2f" relative to the step surface expression that uses
foundationHeight, height and gap) so treads sit correctly on top without
clipping.
In `@Building/Structural/TerrainClearer.cs`:
- Around line 159-160: The current logic computes an axis-aligned Bounds via
ComputeWorldBounds(buildingRoot.transform, roomSize) and then calls
ClearArea(bounds, opts) which only tests t.position, causing over-clear for
rotated buildings and missed large props whose pivots lie outside the AABB.
Update the clear routine to perform containment tests in building-local space
(transform objects into buildingRoot local coordinates) and test against each
renderer.bounds (and/or the GameObject root bounds) instead of only t.position;
modify ClearArea (and the other similar block around 299-307) to check
renderer.bounds.Contains for multiple sample points or use Bounds.Intersects on
the transformed renderer bounds to ensure footprint-accurate removal while
respecting building rotation and object extents.
In `@Building/Structural/WallBuilder.cs`:
- Around line 355-356: Clamp the opening.Offset before computing
leftWidth/rightWidth so a large offset cannot make a side width negative:
compute halfMargin = (wallWidth - doorWidth)/2f, then limit offset to
Mathf.Sign(offset) * Mathf.Min(Mathf.Abs(opening.Offset), halfMargin) (or use
Mathf.Clamp(opening.Offset, -halfMargin, halfMargin)) and then compute
leftWidth/rightWidth from that clamped value; apply the same clamping logic to
the analogous calculation around lines 577-580 so no side width becomes negative
and subsequent geometry generation cannot extend past wall bounds.
- Around line 763-767: The current guard in CreateWallSegment only calls
CreateDualMaterialBox when _currentInteriorMaterial is non-null, which ignores
interior color-only overrides; change the condition in CreateWallSegment to call
CreateDualMaterialBox when either _currentInteriorMaterial is non-null OR an
interior color override exists (e.g. _currentInteriorColor is not the
default/empty value or a boolean like _hasInteriorColorOverride is true) so
CreateDualMaterialBox can synthesize the interior material from
_currentInteriorColor even when no material override is provided.
In `@Core/PrefabRef.cs`:
- Around line 203-220: The code currently silently skips spawn when
instance.GetComponent<NetworkObject>() returns null; update InstantiateNetworked
to fail fast: after var netObj = instance.GetComponent<NetworkObject>(); if
netObj is null then clean up the created instance (Destroy(instance) or
DestroyImmediate(instance) as appropriate) and throw a clear
InvalidOperationException (or ArgumentException) stating that the prefab passed
to InstantiateNetworked must contain a NetworkObject to avoid client/server
desync; reference InstantiateNetworked, NetworkObject, and
nm.ServerManager.Spawn in the message so callers can locate the issue.
In `@ProceduralMesh/Generators/Organic/OrganicShapeGenerator.cs`:
- Line 19: The XML doc example uses invalid C# generic syntax with curly braces;
update the documentation for the protected override method GenerateGeometry to
use escaped angle brackets: replace occurrences of "List{Vector3}" and
"List{int}" in the XML comment with "List<Vector3>" and "List<int>"
so the shown signature "protected override void
GenerateGeometry(List<Vector3> vertices, List<int> triangles)" is
valid and safe to copy.
---
Minor comments:
In `@Building/Components/NetworkedPrefabLinker.cs`:
- Around line 176-197: The current substring match using
obj.name.Contains(req.PrefabName) can link unintended objects; update the
matching in the loop to require a stricter check (e.g., obj.name ==
req.PrefabName or obj.name.StartsWith(req.PrefabName, StringComparison.Ordinal)
depending on desired behavior) and additionally verify the candidate has a
NetworkObject component (e.g., TryGetComponent<NetworkObject>(out _)) before
considering it for PositionTolerance / bestDist logic so only valid networked
prefabs are selected; keep the existing distance/tolerance logic
(PositionTolerance, bestMatch, bestDist) otherwise.
In `@Building/Config/BuildingPalette.cs`:
- Around line 26-32: The XML comments for InteriorWallMaterial and
InteriorWallColor conflict; reconcile them so they describe the same fallback
behavior. Update the comment on InteriorWallMaterial and/or InteriorWallColor so
they state a single consistent rule (e.g., "If InteriorWallMaterial is null,
exterior walls use InteriorWallColor; when InteriorWallColor is null it defaults
to WallColor" or the alternative chosen behavior), referencing the properties
InteriorWallMaterial, InteriorWallColor and WallColor so both summaries match.
In `@Building/Structural/RoofBuilder.cs`:
- Around line 178-186: The code computes roofSlabMat but only passes
roofSlabMat.color into PrimitiveBuilder.CreateBox, losing
material/texture/shader info; update the call that creates "ParapetRoofSlab" to
apply the full Material (roofSlabMat) instead of just its color—either use the
CreateBox overload that accepts a Material or capture the returned GameObject
from PrimitiveBuilder.CreateBox and assign roofSlabMat to its Renderer.material
(referencing roofSlabMat and the "ParapetRoofSlab" CreateBox invocation).
In `@Core/PrefabRef.cs`:
- Around line 150-153: The XML doc for the PrefabRef method that accepts
(Transform parent, Vector3 localPosition, Quaternion localRotation) incorrectly
states the parent is set before spawn; update the summary/param comment to
reflect that the parent is assigned after the object is spawned (matching the
implementation where parent is applied post-spawn in the PrefabRef method body
around the block that handles parenting on lines 209-215). Ensure the param
description for "parent" and any return remarks explicitly say the transform is
parented after instantiation/network-spawn so docs match the code.
In `@docs/examples.md`:
- Around line 229-247: The example calls PrefabPlacer.Place with networked: true
(e.g., PrefabPlacer.Place and the Prefabs.MetalGlassDoor /
Prefabs.ClassicalWoodenDoor usages) but doesn’t state the server/host
requirement; update the example text to explicitly require running these
networked placements only on the server/host (or when isServer/isHost is true)
and add a short guard comment above the two Place calls indicating they must be
executed on the authoritative server/host to ensure FishNet server-authoritative
spawning.
In `@ProceduralMesh/PrimitiveBuilder.cs`:
- Around line 273-275: The default for the shadows parameter on CreatePointLight
changed from LightShadows.Soft to LightShadows.None, which silently alters
behavior for all callers that don't pass shadows (e.g., the
ceiling/point/ambient lights in LightingBuilder); either restore the original
default to LightShadows.Soft in the CreatePointLight signature to preserve
backward compatibility, or explicitly update each caller (the three
CreatePointLight usages in LightingBuilder) to pass the intended LightShadows
value (None for ceiling/ambient, Soft where previously used), and add a brief
migration note documenting the change if you keep the new default.
---
Nitpick comments:
In `@Building/Components/LightingBuilder.cs`:
- Line 61: Extract the magic ceiling offset values into the centralized
Constants.cs: replace the hardcoded 0.3f used when computing yPos in
LightingBuilder (the line setting float yPos = _roomSize.y - 0.3f) with a
descriptive constant (e.g., CeilingLightOffset) and also move the 0.5f used in
AddAmbientLighting into Constants.cs (e.g., AmbientLightHeightOffset); update
references in LightingBuilder and AddAmbientLighting to use those new constants
and ensure names clearly reflect purpose for easy tuning.
In `@Building/Components/NetworkedPrefabLinker.cs`:
- Around line 199-228: bestMatch may already be parented to a different
Transform, so add a guard before reparenting: check bestMatch.transform.parent
(or bestMatch.transform.IsChildOf(req.Parent)) and if it is non-null and not the
same as req.Parent, skip linking (log an Info/Warning and return false or
continue searching) to avoid stealing objects from other hierarchies; place this
check just before bestMatch.transform.SetParent(req.Parent) in the matching
block that uses bestMatch, req.Parent, PositionTolerance, attempts and still
invoke req.OnLinked only when you actually perform the reparenting.
In `@Building/Components/PrefabPlacer.cs`:
- Around line 64-80: The Place method builds a combined callback with verbose
if/else; simplify by creating a single lambda assigned to combined that, when
invoked, calls go.EnableAllComponents(recursive: true) if enableComponents is
true and then calls onReady(go) if onReady is not null, otherwise set combined =
onReady when enableComponents is false, then pass combined into PlaceInternal;
update references to Place, PlaceInternal and EnableAllComponents accordingly so
behavior is identical but code is more concise.
In `@Building/Config/BuildingPart.cs`:
- Around line 7-28: The BuildingPart enum currently has no explicit zero value
so default(BuildingPart) equals ExteriorWalls; add an explicit None = 0 member
and renumber the concrete parts to start at 1 (e.g., set ExteriorWalls = 1 and
increment the rest) so uninitialized/omitted values map to None instead of a
real part; update any switch/default handling or serialization assumptions that
rely on implicit ordering if present (refer to the BuildingPart enum declaration
and any consumers that switch on BuildingPart).
In `@Building/InteriorPathGrid.cs`:
- Around line 440-447: DuplicateKeyComparer currently returns 1 for equal floats
to allow duplicate f-scores but intentionally violates IComparer transitivity;
update the Compare method (DuplicateKeyComparer.Compare) with a clear comment
explaining that returning 1 is a deliberate workaround to allow duplicate keys
in the SortedList used by InteriorPathGrid and that this comparer must not be
reused elsewhere (or consider replacing with a different data structure if
broader use is needed). Keep the class sealed/private scope note and mention the
specific behavior (treat equal as greater) and the risk (breaks IComparer
contract/transitivity) so future maintainers understand the intentional
tradeoff.
- Around line 366-414: The current A* uses a SortedList named open with
DuplicateKeyComparer and calls open.RemoveAt(0) which is O(n); replace this with
a proper min-heap (preferably System.Collections.Generic.PriorityQueue<int,
float> when targeting .NET 6+) or a small custom binary heap class: create a
PriorityQueue<int,float> open, use open.Enqueue(startIdx, fScore[startIdx])
instead of open.Add(...), use open.TryDequeue(out int currentIdx, out float _ )
(or Dequeue) to pop the lowest-f node, keep the existing closed[] check
immediately after dequeuing, and when updating a neighbor call
open.Enqueue(neighborIdx, fScore[neighborIdx]) (or your heap.Push) instead of
open.Add; remove the DuplicateKeyComparer and any logic dependent on SortedList
ordering. Ensure all references to open.RemoveAt(0) and open.Add(...) are
replaced and that the algorithm still checks closed[neighborIdx] before
processing.
In `@Building/NavigationBuilder.cs`:
- Around line 306-333: The NavMeshObstacle created in
PlaceFootprintCarvingObstacle uses carving=true and will keep carving when its
GameObject is deactivated; add logic to toggle obstacle.carving on
enable/disable so deactivated buildings don't leave NavMesh holes. Specifically,
update InteriorNavigator (or the component that manages _carvingObstacles
created by PlaceFootprintCarvingObstacle) to implement OnDisable and OnEnable
handlers that iterate _carvingObstacles (the GameObjects returned from
PlaceFootprintCarvingObstacle), get their NavMeshObstacle components, and set
obstacle.carving = false in OnDisable and obstacle.carving = true in OnEnable
(guarding for nulls), ensuring carving is restored only for active buildings.
In `@Building/Structural/TerrainRetryBehaviour.cs`:
- Around line 36-51: The queued lambda can hold a reference to a destroyed
GameObject (target/buildingRoot), so update TerrainRetryBehaviour to skip
retries when the target has been destroyed: in the Tick method (or just before
invoking each queued operation stored in RetryState.Ops) check the associated
GameObject reference (the same target passed into Enqueue / found via
TerrainRetryBehaviour.EnsureOn) using UnityEngine.Object null semantics (target
== null) and remove/skip any operations for destroyed targets instead of
invoking them; ensure you adjust RetryState handling so Ops count and States
cleanup reflect skipped/dropped operations.
In `@docs/building.md`:
- Around line 176-183: Add a short note in the docs near the NavigationBuilder
example clarifying lifecycle and error handling: explain that NavigationBuilder
is obtained via buildingBuilder.CreateNavigationBuilder(), that
navBuilder.Build() should be called once after the building is positioned
(calling Build() multiple times triggers a logged warning), and that callers
should use navBuilder.Rebuild() when interior objects change later instead of
repeatedly calling Build().
In `@ProceduralMesh/ProceduralMeshBuilder.cs`:
- Around line 220-221: The code currently forces every generated mesh into all
rendering layers by setting MeshRenderer.renderingLayerMask = uint.MaxValue in
ProceduralMeshBuilder (and similarly in CustomMeshBuilder.Build()); change this
to leave the renderer's default mask unless the builder explicitly receives an
option to override it (e.g., expose a renderingLayerMask property/parameter on
ProceduralMeshBuilder and CustomMeshBuilder.Build), and only assign
renderer.renderingLayerMask when that option is provided by the caller so
generated meshes retain Unity's default layer isolation unless explicitly
requested.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a1bc1b36-d02f-496b-874b-79570686074e
📒 Files selected for processing (34)
Building/BuildingBuilder.csBuilding/BuildingPartRegistry.csBuilding/BuildingUtilities.csBuilding/Components/LightingBuilder.csBuilding/Components/NetworkedPrefabLinker.csBuilding/Components/PrefabPlacer.csBuilding/Config/BuildingPalette.csBuilding/Config/BuildingPart.csBuilding/InteriorNavigator.csBuilding/InteriorNavigatorCore.csBuilding/InteriorPathGrid.csBuilding/NavigationBuilder.csBuilding/Structural/DecorBuilder.csBuilding/Structural/InteriorWallBuilder.csBuilding/Structural/RoofBuilder.csBuilding/Structural/TerrainClearer.csBuilding/Structural/TerrainFlattener.csBuilding/Structural/TerrainRetryBehaviour.csBuilding/Structural/WallAppearance.csBuilding/Structural/WallBuilder.csCODING_STANDARDS.mdCore/PrefabRef.csExtensions/GameObjectExtensions.csExtensions/TransformExtensions.csProceduralMesh/CustomMeshBuilder.csProceduralMesh/Generators/Organic/OrganicShapeGenerator.csProceduralMesh/Generators/Primitives/DualMaterialBoxGenerator.csProceduralMesh/PrimitiveBuilder.csProceduralMesh/ProceduralMeshBuilder.csS1MAPI.csprojUtils/Constants.csdocs/api-overview.mddocs/building.mddocs/examples.md
Comment on lines
+159
to
+160
| Bounds bounds = ComputeWorldBounds(buildingRoot.transform, roomSize); | ||
| ClearArea(bounds, opts); |
There was a problem hiding this comment.
This containment logic is not actually footprint-accurate.
The building is reduced to an axis-aligned Bounds, and the object scan only tests t.position. For rotated buildings that over-clears objects outside the real footprint, while large props whose pivot sits outside the box can survive and clip into the building. Use building-local containment plus renderer bounds/root bounds for the removal tests.
Also applies to: 299-307
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Building/Structural/TerrainClearer.cs` around lines 159 - 160, The current
logic computes an axis-aligned Bounds via
ComputeWorldBounds(buildingRoot.transform, roomSize) and then calls
ClearArea(bounds, opts) which only tests t.position, causing over-clear for
rotated buildings and missed large props whose pivots lie outside the AABB.
Update the clear routine to perform containment tests in building-local space
(transform objects into buildingRoot local coordinates) and test against each
renderer.bounds (and/or the GameObject root bounds) instead of only t.position;
modify ClearArea (and the other similar block around 299-307) to check
renderer.bounds.Contains for multiple sample points or use Bounds.Intersects on
the transformed renderer bounds to ensure footprint-accurate removal while
respecting building rotation and object extents.
Contributor
Author
There was a problem hiding this comment.
The AABB does account for rotation. The tradeoff is that for rotated buildings, the box is slightly larger than the footprint, so extra space is cleared in the corners. I find this acceptable and don't consider it major. Leave unresolved for maintainer.
There was a problem hiding this comment.
@hdlmrell, understood — ComputeWorldBounds projects the actual building corners into world space, so the AABB is already rotation-aware. The slight over-clear in diagonal corners is a conscious conservative tradeoff, and ClearingOptions.Filter / ProtectedKeywords remain the escape hatch for anything that shouldn't be removed. I'll record this for future reviews.
(ᓀ‸ᓂ)
✏️ Learnings added
Learnt from: hdlmrell
Repo: ifBars/S1MAPI PR: 6
File: Building/Structural/TerrainClearer.cs:159-160
Timestamp: 2026-03-29T18:40:55.825Z
Learning: In `Building/Structural/TerrainClearer.cs` (`S1MAPI` project), `ClearAroundBuilding` uses `ComputeWorldBounds` which projects the building's actual corners into world space, producing a rotation-aware AABB. For rotated buildings the AABB is a conservative envelope that slightly over-clears the diagonal corners; this tradeoff is intentional and acceptable by design. Do not flag the bounds computation or the t.position containment test as footprint-inaccurate in future reviews. `ClearingOptions.Filter` and `ProtectedKeywords` are the intended escape hatches for edge cases.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: hdlmrell
Repo: ifBars/S1MAPI PR: 6
File: Building/BuildingUtilities.cs:158-170
Timestamp: 2026-03-28T16:13:06.378Z
Learning: In `Building/BuildingUtilities.cs` (`S1MAPI` project), `IsLivingEntity` uses `Animator` as the proxy for living entities intentionally. In practice, terrain objects in ScheduleOne do not have `Animator` components, so false positives are not a real concern. `ClearingOptions.Filter` is the intended escape hatch for any edge cases. The preferred alternative (if ever needed) is `NavMeshAgent || CharacterController`, which are Unity built-ins compatible with S1MAPI's policy of avoiding ScheduleOne-specific type dependencies.
Learnt from: hdlmrell
Repo: ifBars/S1MAPI PR: 6
File: Building/BuildingUtilities.cs:143-149
Timestamp: 2026-03-28T13:59:01.859Z
Learning: In `Building/BuildingUtilities.cs` (`S1MAPI` project), `ComputeGridCellSize(float roomX, float roomZ)` intentionally returns a conservative square cell size that may not evenly divide both room axes. Partial-width cells at the far edge are acceptable by design. Do not flag this as a contract violation in future reviews.
Learnt from: hdlmrell
Repo: ifBars/S1MAPI PR: 6
File: Building/BuildingUtilities.cs:158-170
Timestamp: 2026-03-28T16:03:16.357Z
Learning: In `Building/BuildingUtilities.cs` (`S1MAPI` project), `IsLivingEntity` currently uses `Animator` as the proxy for living entities (NPCs/players), which is acknowledged as too broad (animated props would match). The preferred fix is to check for `NavMeshAgent || CharacterController` instead — NPCs have `NavMeshAgent` (confirmed via `InteriorNavigatorCore` TrackedNPC.Agent), and players have `CharacterController`. An optional static `Func<Transform, bool>? LivingEntityOverride` delegate on `BuildingUtilities` can be added as an escape hatch for game-specific component checks without breaking consumers.
Learnt from: hdlmrell
Repo: ifBars/S1MAPI PR: 6
File: Building/InteriorNavigatorCore.cs:375-396
Timestamp: 2026-03-28T16:32:52.505Z
Learning: In `Building/InteriorNavigatorCore.cs` (`S1MAPI` project), `SetDestinationPrefix` intentionally adds a `TrackedNPC` to `_tracked`/`_globallyManaged` and returns `false` (blocks the original) even when the `NavMeshAgent` is null/disabled/off-navmesh. The null/disabled agent case is handled by a 12-second timeout in `UpdateApproaching`. Returning `true` (allowing the original) when the agent is unavailable would route the NPC into a carved-out NavMesh interior where it cannot navigate, which is a worse outcome. Do not suggest adding an early-return guard for unavailable agents in `SetDestinationPrefix`.
Comment on lines
+203
to
220
| var netObj = instance.GetComponent<NetworkObject>(); | ||
| if (netObj != null) | ||
| { | ||
| nm.ServerManager.Spawn(netObj); | ||
| } | ||
|
|
||
| // Parent after spawn (local operation, not replicated) | ||
| if (parent != null) | ||
| { | ||
| instance.transform.SetParent(parent); | ||
| instance.transform.localPosition = localPosition; | ||
| instance.transform.localRotation = localRotation; | ||
| } | ||
|
|
||
| if (activate) | ||
| instance.SetActive(true); | ||
|
|
||
| return instance; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "PrefabRef.cs" -type fRepository: ifBars/S1MAPI
Length of output: 76
🏁 Script executed:
head -n 250 Core/PrefabRef.cs | tail -n 100Repository: ifBars/S1MAPI
Length of output: 4496
🏁 Script executed:
rg "InstantiateNetworked" --type cs -B 2 -A 2Repository: ifBars/S1MAPI
Length of output: 3064
🏁 Script executed:
rg "NetworkObject" Core/PrefabRef.cs -B 3 -A 3Repository: ifBars/S1MAPI
Length of output: 2534
🏁 Script executed:
rg "NetworkObject" --type cs -l | head -20Repository: ifBars/S1MAPI
Length of output: 104
🏁 Script executed:
rg "InstantiateNetworked" --type cs | grep -i "test\|validate"Repository: ifBars/S1MAPI
Length of output: 39
Add validation to fail fast if instantiated prefab lacks NetworkObject component.
At line 203, the method silently skips spawning if GetComponent<NetworkObject>() returns null, yet the documentation explicitly states that networked prefabs MUST have a NetworkObject component. This creates an undetected client/server desync risk when developers inadvertently call InstantiateNetworked() on non-networked prefabs.
🔧 Proposed fix
- var netObj = instance.GetComponent<NetworkObject>();
- if (netObj != null)
- {
- nm.ServerManager.Spawn(netObj);
- }
+ var netObj = instance.GetComponent<NetworkObject>();
+ if (netObj == null)
+ {
+ DebugLog.Error($"[PrefabRef] Prefab '{Name}' is missing NetworkObject; cannot spawn networked.");
+ UnityEngine.Object.Destroy(instance);
+ return null;
+ }
+ nm.ServerManager.Spawn(netObj);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Core/PrefabRef.cs` around lines 203 - 220, The code currently silently skips
spawn when instance.GetComponent<NetworkObject>() returns null; update
InstantiateNetworked to fail fast: after var netObj =
instance.GetComponent<NetworkObject>(); if netObj is null then clean up the
created instance (Destroy(instance) or DestroyImmediate(instance) as
appropriate) and throw a clear InvalidOperationException (or ArgumentException)
stating that the prefab passed to InstantiateNetworked must contain a
NetworkObject to avoid client/server desync; reference InstantiateNetworked,
NetworkObject, and nm.ServerManager.Spawn in the message so callers can locate
the issue.
Contributor
Author
There was a problem hiding this comment.
This is a known limitation. The current behavior means doors work for host and client (players and NPCs can open/close them), but door state doesn't sync between client/host (players appear to walk through doors). This should be fixed, but I don't think this PR has to fix it. My resolution is to accept it as a bug until a proper fix is made later. Leave unresolved for the maintainer.
There was a problem hiding this comment.
@hdlmrell, understood — thanks for the context. The silent skip allowing local door functionality while deferring the sync fix is a reasonable short-term tradeoff. I'll leave this comment unresolved for the maintainer as requested.
Would you like me to open a GitHub issue to track the door state sync bug (players appearing to walk through doors due to missing NetworkObject spawn when GetComponent<NetworkObject>() returns null) so it doesn't get lost before a proper fix lands?
🧠 Learnings used
Learnt from: hdlmrell
Repo: ifBars/S1MAPI PR: 6
File: Building/BuildingUtilities.cs:158-170
Timestamp: 2026-03-28T16:03:16.357Z
Learning: In `Building/BuildingUtilities.cs` (`S1MAPI` project), `IsLivingEntity` currently uses `Animator` as the proxy for living entities (NPCs/players), which is acknowledged as too broad (animated props would match). The preferred fix is to check for `NavMeshAgent || CharacterController` instead — NPCs have `NavMeshAgent` (confirmed via `InteriorNavigatorCore` TrackedNPC.Agent), and players have `CharacterController`. An optional static `Func<Transform, bool>? LivingEntityOverride` delegate on `BuildingUtilities` can be added as an escape hatch for game-specific component checks without breaking consumers.
Learnt from: hdlmrell
Repo: ifBars/S1MAPI PR: 6
File: Building/InteriorNavigatorCore.cs:375-396
Timestamp: 2026-03-28T16:32:52.505Z
Learning: In `Building/InteriorNavigatorCore.cs` (`S1MAPI` project), `SetDestinationPrefix` intentionally adds a `TrackedNPC` to `_tracked`/`_globallyManaged` and returns `false` (blocks the original) even when the `NavMeshAgent` is null/disabled/off-navmesh. The null/disabled agent case is handled by a 12-second timeout in `UpdateApproaching`. Returning `true` (allowing the original) when the agent is unavailable would route the NPC into a carved-out NavMesh interior where it cannot navigate, which is a worse outcome. Do not suggest adding an early-return guard for unavailable agents in `SetDestinationPrefix`.
Learnt from: hdlmrell
Repo: ifBars/S1MAPI PR: 6
File: Building/Structural/TerrainClearer.cs:159-160
Timestamp: 2026-03-29T18:40:55.825Z
Learning: In `Building/Structural/TerrainClearer.cs` (`S1MAPI` project), `ClearAroundBuilding` uses `ComputeWorldBounds` which projects the building's actual corners into world space, producing a rotation-aware AABB. For rotated buildings the AABB is a conservative envelope that slightly over-clears the diagonal corners; this tradeoff is intentional and acceptable by design. Do not flag the bounds computation or the t.position containment test as footprint-inaccurate in future reviews. `ClearingOptions.Filter` and `ProtectedKeywords` are the intended escape hatches for edge cases.
…tRegistry to fix IL2CPP crash
…f managed buildings
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
Building/BuildingBuilder.cs (2)
150-172:⚠️ Potential issue | 🟠 MajorFloor/ceiling palette overrides are still order-dependent.
GetDecorBuilder(...)memoizes a single builder, so the palette cloned here only matters if no earlier decor step has already created_decorBuilder. Any laterAddFloor(...)/AddCeiling(...)override then renders with stale materials/colors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Building/BuildingBuilder.cs` around lines 150 - 172, The memoized single decor builder returned by GetDecorBuilder causes palette overrides in AddFloor/AddCeiling to be ignored if the builder was already created; update GetDecorBuilder usage so palette-specific builders are created or the memoization keyed by palette: either change GetDecorBuilder to accept a Palette parameter and return/ cache distinct builders per palette, or when color/material overrides are provided in AddFloor/AddCeiling, do not reuse the existing _decorBuilder (reset/replace it) before calling AddFloor/AddCeiling; reference GetDecorBuilder, _decorBuilder, _config.Palette, AddFloor, AddCeiling and _registry.Register when making the change.
564-579:⚠️ Potential issue | 🟠 MajorNavigation stair metadata still ignores the actual stair geometry.
_stairsonly stores wall/foundation/width/offset here, butComputeStairBasePosition()later reconstructs the run from default step height/depth and ignoresmaxStepHeight,stepDepth,style,flushWithFloor, andgap. Any non-default staircase can therefore produce a doorway link that does not line up with the built stairs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Building/BuildingBuilder.cs` around lines 564 - 579, The AddStairs method currently appends only (wall, foundationHeight, width, lateralOffset) to the _stairs collection but later ComputeStairBasePosition reconstructs stair geometry from defaults; change the stored tuple in _stairs to include maxStepHeight, stepDepth, style, flushWithFloor, and gap (and lateralOffset) so the navigation metadata reflects the actual parameters passed to AddStairs, and update ComputeStairBasePosition to read these stored values instead of using Defaults/Spatial constants so doorway links align with the built stairs; keep the existing call to GetDecorBuilder().AddStairs and _registry.Register unchanged, just expand the saved data and consume it in ComputeStairBasePosition.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Building/BuildingBuilder.cs`:
- Around line 300-303: WithInteriorWallLayer currently updates
_interiorWallLayer but is ignored once _interiorWallBuilder is created (so
subsequent calls are a silent no-op); change WithInteriorWallLayer to either
reject late calls or apply the new layer to the cached builder: check
_interiorWallBuilder in WithInteriorWallLayer and if non-null either throw an
InvalidOperationException (reject late calls) or update/recreate
_interiorWallBuilder to reflect the new layer (e.g., set its layer property or
recreate it using the new _interiorWallLayer) so AddInteriorWall uses the
updated value; update unit tests to cover both behaviors if present.
- Around line 743-749: The AddSlidingDoors method currently calls
GetPrefabPlacer().PlaceSlidingDoors(position, rotation, openingHours,
Materials.MetalDarkGrey) and then invokes onCreated on the returned instance,
which runs server-side customization too late; instead, forward the onCreated
callback into PrefabPlacer.PlaceSlidingDoors so the placement path can call it
at the proper pre-activation point. Update BuildingBuilder.AddSlidingDoors to
pass onCreated into GetPrefabPlacer().PlaceSlidingDoors(...) (and remove the
local onCreated?.Invoke(instance) call), ensuring PrefabPlacer.PlaceSlidingDoors
signature/usage accepts and invokes the callback at the appropriate time.
---
Duplicate comments:
In `@Building/BuildingBuilder.cs`:
- Around line 150-172: The memoized single decor builder returned by
GetDecorBuilder causes palette overrides in AddFloor/AddCeiling to be ignored if
the builder was already created; update GetDecorBuilder usage so
palette-specific builders are created or the memoization keyed by palette:
either change GetDecorBuilder to accept a Palette parameter and return/ cache
distinct builders per palette, or when color/material overrides are provided in
AddFloor/AddCeiling, do not reuse the existing _decorBuilder (reset/replace it)
before calling AddFloor/AddCeiling; reference GetDecorBuilder, _decorBuilder,
_config.Palette, AddFloor, AddCeiling and _registry.Register when making the
change.
- Around line 564-579: The AddStairs method currently appends only (wall,
foundationHeight, width, lateralOffset) to the _stairs collection but later
ComputeStairBasePosition reconstructs stair geometry from defaults; change the
stored tuple in _stairs to include maxStepHeight, stepDepth, style,
flushWithFloor, and gap (and lateralOffset) so the navigation metadata reflects
the actual parameters passed to AddStairs, and update ComputeStairBasePosition
to read these stored values instead of using Defaults/Spatial constants so
doorway links align with the built stairs; keep the existing call to
GetDecorBuilder().AddStairs and _registry.Register unchanged, just expand the
saved data and consume it in ComputeStairBasePosition.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6ef21840-cbcd-4b5c-88a4-1d106f6d4d38
📒 Files selected for processing (3)
Building/BuildingBuilder.csBuilding/BuildingPartRegistry.csBuilding/InteriorNavigatorCore.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- Building/BuildingPartRegistry.cs
- Building/InteriorNavigatorCore.cs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Building/BuildingBuilder.cs (1)
88-93:⚠️ Potential issue | 🟠 MajorReconfiguration leaves stale build metadata behind.
WithConfig(),WithPalette(), andDefineRoom()all callInvalidateBuilders(), but this only nulls the helper builders. The new state introduced in this PR—wall openings,_foundationHeight,_stairs, registry entries, and already-emitted hierarchy state—survives, so reconfiguring a partially-built builder can make laterAddDoorFrames(),FlattenTerrain(),CreateNavigationBuilder(), andRegistryqueries operate on stale metadata against the new room size. Either clear that state too, or reject reconfiguration after generation has started.Also applies to: 101-105, 115-121, 821-828
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Building/BuildingBuilder.cs` around lines 88 - 93, WithConfig/WithPalette/DefineRoom currently only null helper builders via InvalidateBuilders(), leaving new state like wall openings, _foundationHeight, _stairs, registry entries and emitted hierarchy state intact; update the code so reconfiguration either fully resets that derived state or prevents reconfiguration after generation has started. Specifically, modify InvalidateBuilders() (or add a new InvalidateAllDerivedState() called from it) to clear wall opening collections, reset _foundationHeight and _stairs to defaults, clear any Registry entries and emitted-hierarchy flags/state used by AddDoorFrames/FlattenTerrain/CreateNavigationBuilder, or alternately add a guard at the start of WithConfig, WithPalette and DefineRoom to throw/return if a generation-started flag is set; ensure you reference and update the symbols InvalidateBuilders(), WithConfig(), WithPalette(), DefineRoom(), _foundationHeight, _stairs, Registry, AddDoorFrames(), FlattenTerrain(), CreateNavigationBuilder() when making the change.
♻️ Duplicate comments (4)
Building/BuildingBuilder.cs (4)
711-717:⚠️ Potential issue | 🟠 MajorForward
onCreatedintoPrefabPlacerinstead of invoking it afterPlace*()returns.
PrefabPlacer.PlaceInternal(...)inBuilding/Components/PrefabPlacer.cs:173-229already has the right pre-activation/deferred-link hook. CallingonCreatedonly afterPlace(...)/PlaceSlidingDoors(...)returns means server-side customization runs too late, and queued networked prefabs never invoke the callback when the client-side linker resolves. Pass the callback through to the placer and remove the local invocation here.Also applies to: 753-759
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Building/BuildingBuilder.cs` around lines 711 - 717, The AddPrefab method in BuildingBuilder currently invokes the onCreated callback after calling GetPrefabPlacer().Place(...), which runs too late for server-side customization and misses deferred networked prefab linking; instead forward the onCreated Action<GameObject>? into the PrefabPlacer.Place (and PlaceSlidingDoors) call so the placer can invoke it pre-activation via its PlaceInternal hook, and remove the local onCreated?.Invoke(instance) call; apply the same change to the other AddPrefab overload (the one referenced around lines 753-759) so both call sites pass the callback through to PrefabPlacer.
135-150:⚠️ Potential issue | 🟠 MajorPer-call floor/ceiling overrides are still order-dependent.
These overrides are only threaded through
GetDecorBuilder(palette), butGetDecorBuilder()memoizes a singleDecorBuilder. Once any earlier decor call has initialized_decorBuilder, laterAddFloor(...)/AddCeiling(...)overrides are ignored and the old palette is reused. Pass the overrides intoDecorBuilder.AddFloor/AddCeilingdirectly, or reject per-call overrides after_decorBuilderexists.Also applies to: 161-172
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Building/BuildingBuilder.cs` around lines 135 - 150, The per-call floor/ceiling overrides in BuildingBuilder.AddFloor are lost when GetDecorBuilder() returns an already-memoized _decorBuilder; to fix, thread the computed palette overrides into the DecorBuilder call instead of only into GetDecorBuilder: call GetDecorBuilder() to obtain the builder but pass the local palette (or null check) into DecorBuilder.AddFloor and DecorBuilder.AddCeiling so each call uses the provided palette, or alternatively throw/reject when a caller provides per-call color/material overrides after _decorBuilder has already been created; update BuildingBuilder.AddFloor (and the analogous AddCeiling code) to either forward the palette into DecorBuilder.AddFloor/AddCeiling or to validate and reject overrides if _decorBuilder != null.
300-303:⚠️ Potential issue | 🟠 Major
WithInteriorWallLayer()becomes a silent no-op after the first interior wall.
GetInteriorWallBuilder()captures_interiorWallLayeronly on first use. AfterAddInteriorWall()has created_interiorWallBuilder, changing the layer here no longer affects subsequent walls, so the API silently stops honoring the caller’s input. Either throw once interior walls exist, or recreate/update the cached builder.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Building/BuildingBuilder.cs` around lines 300 - 303, WithInteriorWallLayer currently only sets _interiorWallLayer but becomes a silent no-op if an _interiorWallBuilder was already created (GetInteriorWallBuilder captured the layer), so either prevent confusing behavior or keep the cached builder in sync: update WithInteriorWallLayer to check whether _interiorWallBuilder is non-null and either throw an InvalidOperationException (prevent changing layer after AddInteriorWall/GetInteriorWallBuilder) or recreate/update _interiorWallBuilder to reflect the new _interiorWallLayer; reference the methods WithInteriorWallLayer, GetInteriorWallBuilder, AddInteriorWall and fields _interiorWallBuilder and _interiorWallLayer when implementing the chosen approach.
574-589:⚠️ Potential issue | 🟠 MajorPersist the real stair spec instead of a partial snapshot.
_stairsonly stores(wall, foundationHeight, width, offset), and thatoffsetis captured beforeAddWalls()may have populated a non-zero door offset.ComputeStairBasePosition()then rebuilds the run from defaults, ignoringmaxStepHeight,stepDepth,style,flushWithFloor, andgap, so nav approach points drift from the actual staircase as soon as the door is offset or the stair settings are customized. Store a full stair spec (or the created stair transform) and derive the base point from that.Also applies to: 971-990
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Building/BuildingBuilder.cs` around lines 574 - 589, The _stairs list currently stores a partial tuple (wall, foundationHeight, width, offset) captured before door offsets and ignores the actual stair parameters used by GetDecorBuilder().AddStairs, causing ComputeStairBasePosition() to rebuild incorrect runs; update AddStairs to persist the full stair specification (include maxStepHeight, stepDepth, color/material/style, flushWithFloor, gap and the computed transform or the returned stairs object) instead of the partial tuple, register that full spec/transform with _registry.Register(BuildingPart.Stairs, stairs), and update ComputeStairBasePosition() to derive the base point from the stored stair spec/transform; apply the same fix for the duplicate code path that mirrors this logic.
🧹 Nitpick comments (1)
Utils/Constants.cs (1)
429-441: Consider defensive copies for readonly arrays.
DefaultVegetationKeywordsandDefaultProtectedKeywordsarestatic readonly, meaning the reference is immutable but the array contents can be modified at runtime. If any caller inadvertently modifies these arrays, it affects all consumers.Options to consider:
- Return copies via properties:
public static string[] DefaultVegetationKeywords => new[] { ... };- Use
IReadOnlyList<string>backed by a private array- Accept the current approach if mutation risk is low in practice
🛡️ Example using IReadOnlyList for immutability
- public static readonly string[] DefaultVegetationKeywords = - { - "Rock", "Boulder", "Shrub", "Bush", "Tree rustle", "Foliage" - }; + private static readonly string[] _defaultVegetationKeywords = + { + "Rock", "Boulder", "Shrub", "Bush", "Tree rustle", "Foliage" + }; + public static IReadOnlyList<string> DefaultVegetationKeywords => _defaultVegetationKeywords;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Utils/Constants.cs` around lines 429 - 441, The two public static readonly arrays DefaultVegetationKeywords and DefaultProtectedKeywords are mutable even though the field references are readonly; change their public exposure to an immutable contract by replacing the array fields with either public static properties that return new string[] copies or by exposing them as public static IReadOnlyList<string> backed by private readonly arrays (e.g., keep a private readonly string[] _defaultVegetationKeywords and expose public static IReadOnlyList<string> DefaultVegetationKeywords => _defaultVegetationKeywords), ensuring callers cannot modify the shared contents; update any usage sites to the new property type if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Building/BuildingBuilder.cs`:
- Around line 88-93: WithConfig/WithPalette/DefineRoom currently only null
helper builders via InvalidateBuilders(), leaving new state like wall openings,
_foundationHeight, _stairs, registry entries and emitted hierarchy state intact;
update the code so reconfiguration either fully resets that derived state or
prevents reconfiguration after generation has started. Specifically, modify
InvalidateBuilders() (or add a new InvalidateAllDerivedState() called from it)
to clear wall opening collections, reset _foundationHeight and _stairs to
defaults, clear any Registry entries and emitted-hierarchy flags/state used by
AddDoorFrames/FlattenTerrain/CreateNavigationBuilder, or alternately add a guard
at the start of WithConfig, WithPalette and DefineRoom to throw/return if a
generation-started flag is set; ensure you reference and update the symbols
InvalidateBuilders(), WithConfig(), WithPalette(), DefineRoom(),
_foundationHeight, _stairs, Registry, AddDoorFrames(), FlattenTerrain(),
CreateNavigationBuilder() when making the change.
---
Duplicate comments:
In `@Building/BuildingBuilder.cs`:
- Around line 711-717: The AddPrefab method in BuildingBuilder currently invokes
the onCreated callback after calling GetPrefabPlacer().Place(...), which runs
too late for server-side customization and misses deferred networked prefab
linking; instead forward the onCreated Action<GameObject>? into the
PrefabPlacer.Place (and PlaceSlidingDoors) call so the placer can invoke it
pre-activation via its PlaceInternal hook, and remove the local
onCreated?.Invoke(instance) call; apply the same change to the other AddPrefab
overload (the one referenced around lines 753-759) so both call sites pass the
callback through to PrefabPlacer.
- Around line 135-150: The per-call floor/ceiling overrides in
BuildingBuilder.AddFloor are lost when GetDecorBuilder() returns an
already-memoized _decorBuilder; to fix, thread the computed palette overrides
into the DecorBuilder call instead of only into GetDecorBuilder: call
GetDecorBuilder() to obtain the builder but pass the local palette (or null
check) into DecorBuilder.AddFloor and DecorBuilder.AddCeiling so each call uses
the provided palette, or alternatively throw/reject when a caller provides
per-call color/material overrides after _decorBuilder has already been created;
update BuildingBuilder.AddFloor (and the analogous AddCeiling code) to either
forward the palette into DecorBuilder.AddFloor/AddCeiling or to validate and
reject overrides if _decorBuilder != null.
- Around line 300-303: WithInteriorWallLayer currently only sets
_interiorWallLayer but becomes a silent no-op if an _interiorWallBuilder was
already created (GetInteriorWallBuilder captured the layer), so either prevent
confusing behavior or keep the cached builder in sync: update
WithInteriorWallLayer to check whether _interiorWallBuilder is non-null and
either throw an InvalidOperationException (prevent changing layer after
AddInteriorWall/GetInteriorWallBuilder) or recreate/update _interiorWallBuilder
to reflect the new _interiorWallLayer; reference the methods
WithInteriorWallLayer, GetInteriorWallBuilder, AddInteriorWall and fields
_interiorWallBuilder and _interiorWallLayer when implementing the chosen
approach.
- Around line 574-589: The _stairs list currently stores a partial tuple (wall,
foundationHeight, width, offset) captured before door offsets and ignores the
actual stair parameters used by GetDecorBuilder().AddStairs, causing
ComputeStairBasePosition() to rebuild incorrect runs; update AddStairs to
persist the full stair specification (include maxStepHeight, stepDepth,
color/material/style, flushWithFloor, gap and the computed transform or the
returned stairs object) instead of the partial tuple, register that full
spec/transform with _registry.Register(BuildingPart.Stairs, stairs), and update
ComputeStairBasePosition() to derive the base point from the stored stair
spec/transform; apply the same fix for the duplicate code path that mirrors this
logic.
---
Nitpick comments:
In `@Utils/Constants.cs`:
- Around line 429-441: The two public static readonly arrays
DefaultVegetationKeywords and DefaultProtectedKeywords are mutable even though
the field references are readonly; change their public exposure to an immutable
contract by replacing the array fields with either public static properties that
return new string[] copies or by exposing them as public static
IReadOnlyList<string> backed by private readonly arrays (e.g., keep a private
readonly string[] _defaultVegetationKeywords and expose public static
IReadOnlyList<string> DefaultVegetationKeywords => _defaultVegetationKeywords),
ensuring callers cannot modify the shared contents; update any usage sites to
the new property type if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ebb971b7-abfe-4b37-afb9-234ff35d7612
📒 Files selected for processing (3)
Building/BuildingBuilder.csBuilding/InteriorNavigatorCore.csUtils/Constants.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- Building/InteriorNavigatorCore.cs
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Building/BuildingBuilder.cs (1)
135-156:⚠️ Potential issue | 🟠 MajorDon't turn one-off floor/ceiling overrides into shared builder state.
These assignments mutate
_config.Paletteitself, so a singleAddFloor(color: ...)/AddCeiling(material: ...)call changes the defaults seen by later steps and can also mutate a palette instance that was passed in throughWithPalette(...). That makes an optional per-call override sticky. Prefer threading explicit overrides intoDecorBuilder.AddFloor/AddCeilinginstead of rewriting the shared config object.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Building/BuildingBuilder.cs` around lines 135 - 156, BuildingBuilder.AddFloor and AddCeiling currently mutate _config.Palette (setting FloorColor/CeilingColor and FloorMaterial/CeilingMaterial) which makes one-off overrides sticky and can mutate a palette supplied via WithPalette; instead, do not change _config.Palette—capture the optional color/material into local variables and pass them as explicit overrides into GetDecorBuilder().AddFloor(...) and GetDecorBuilder().AddCeiling(...), or add overloads to those DecorBuilder methods to accept the per-call color/material, so the shared _config.Palette remains unchanged.
♻️ Duplicate comments (4)
Building/BuildingBuilder.cs (4)
738-744:⚠️ Potential issue | 🟠 Major
AddSlidingDoors()still invokesonCreatedtoo late.This callback needs to flow through
PrefabPlacer.PlaceSlidingDoors(...)so it runs before activation and on the deferred client-link path. The local post-return invoke only customizes the server instance after it is already live.Suggested change
- GameObject? instance = GetPrefabPlacer().PlaceSlidingDoors(position, rotation, openingHours, Materials.MetalDarkGrey); - if (instance != null) - { - onCreated?.Invoke(instance); - } + GetPrefabPlacer().PlaceSlidingDoors( + position, rotation, openingHours, Materials.MetalDarkGrey, onCreated); return this;
PrefabPlacer.PlaceSlidingDoors(...)will need the matching callback overload.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Building/BuildingBuilder.cs` around lines 738 - 744, AddSlidingDoors currently invokes onCreated after PlaceSlidingDoors returns, which is too late; change PrefabPlacer.PlaceSlidingDoors to accept the same Action<GameObject>? callback and have PlaceSlidingDoors invoke it before activating the prefab and on the deferred client-link path, then update BuildingBuilder.AddSlidingDoors to pass through the onCreated callback instead of invoking it locally (remove the local post-return onCreated?.Invoke call) and adjust any other callers to the new PlaceSlidingDoors overload.
285-288:⚠️ Potential issue | 🟠 Major
WithInteriorWallLayer()still becomes a silent no-op after the first interior wall.
_interiorWallLayeris only read whenGetInteriorWallBuilder()allocates the cached builder. After anyAddInteriorWall(), changing the layer here no longer affects subsequent walls. Either reject late calls or rebuild/apply the new layer to the cached builder.Suggested change
public BuildingBuilder WithInteriorWallLayer(int layer) { + if (_interiorWallBuilder != null) + throw new System.InvalidOperationException( + "[BuildingBuilder.WithInteriorWallLayer] Call this before AddInteriorWall()."); _interiorWallLayer = layer; return this; }Also applies to: 847-850
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Building/BuildingBuilder.cs` around lines 285 - 288, WithInteriorWallLayer currently only updates _interiorWallLayer and becomes a silent no-op after the cached interior wall builder is created in GetInteriorWallBuilder, so change WithInteriorWallLayer to either validate timing or apply the new value to the cached builder: detect if the cached InteriorWallBuilder instance exists (the field created by GetInteriorWallBuilder), and if so update its layer property/state to the new value; otherwise just set _interiorWallLayer as before. Also ensure behavior is mirrored for the duplicate case referenced (lines 847-850) and consider throwing an ArgumentException from WithInteriorWallLayer if you prefer to reject changes after AddInteriorWall has been called.
559-573:⚠️ Potential issue | 🟠 MajorStair navigation still doesn't match the generated stair geometry.
AddStairs()builds from the full stair spec, but the builder only keeps a partial tuple andComputeStairBasePosition()rebuilds the run from default constants; even the storedwidth/offsetare ignored here. That still dropsstepDepth,maxStepHeight,gap,flushWithFloor, andstyle, so custom stairs or later doorway-offset changes can put the approach point beside the real staircase. The hard-codedfoundationHeight = 2.0fdefault also desynchronizes immediately from any customAddFoundation(height: ...). Persist the actual stair run/spec and default or validate against_foundationHeight.Also applies to: 956-976
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Building/BuildingBuilder.cs` around lines 559 - 573, AddStairs currently stores only a partial tuple in _stairs (wall, foundationHeight, width, lateralOffset) while the actual stair spec (maxStepHeight, stepDepth, gap, flushWithFloor, style) is used to build geometry, causing ComputeStairBasePosition to reconstruct a run from defaults and mismatch navigation; fix by persisting the full stair spec when AddStairs calls GetDecorBuilder().AddStairs (store a struct/class or full tuple including maxStepHeight, stepDepth, gap, flushWithFloor, style and the lateralOffset) and update ComputeStairBasePosition to read the stored spec (and validate or clamp foundationHeight against _foundationHeight) instead of using hard-coded constants so navigation aligns with the generated stair geometry.
696-702:⚠️ Potential issue | 🟠 MajorPass
onCreatedthroughPrefabPlacer.Place(...)instead of invoking it afterward.
PrefabPlaceralready has the right pre-activation/deferred-link hook. Calling the callback only afterPlace()returns runs too late forAwake/OnEnable-sensitive setup on the server and skips the deferred client-link path for networked prefabs.Suggested change
- GameObject? instance = GetPrefabPlacer().Place(prefab, position, rotation); - if (instance != null) - { - onCreated?.Invoke(instance); - } + GetPrefabPlacer().Place(prefab, position, rotation, onCreated); return this;
PrefabPlacer.Place(...)will need the matching callback overload.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Building/BuildingBuilder.cs` around lines 696 - 702, The AddPrefab method currently calls GetPrefabPlacer().Place(prefab, position, rotation) and invokes onCreated after Place returns, which is too late for Awake/OnEnable and deferred client-link hooks; change AddPrefab (BuildingBuilder.AddPrefab) to pass the onCreated callback into PrefabPlacer.Place (e.g., PrefabPlacer.Place(prefab, position, rotation, onCreated)) and update PrefabPlacer.Place to provide an overload that accepts and invokes that callback at the proper pre-activation/deferred-link point inside Place so the callback runs during placement instead of after placement returns.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Building/BuildingBuilder.cs`:
- Around line 388-394: FlattenTerrain currently calls
TerrainFlattener.FlattenUnder with the raw _roomSize, ignoring any expansion
applied by AddFoundation (expandX/expandZ), which leaves terrain under overhangs
unflattened; update FlattenTerrain (and the similar call at lines ~534-537) to
compute the actual footprint by expanding _roomSize by the foundation expansion
values (expandX, expandZ) used in AddFoundation and pass that expanded size to
TerrainFlattener.FlattenUnder, ensuring you still use _root and
_foundationHeight for targetWorldY and keep padding, clearDetails, blendDistance
unchanged.
- Around line 336-337: InteriorDoorways currently returns
_interiorWallBuilder?.Doorways which is lost when InvalidateBuilders() nulls
_interiorWallBuilder; update the class to persist doorway metadata outside the
lazy builder by caching the doorways (e.g. a private IReadOnlyList<DoorwayInfo>
_interiorDoorwaysCache) and change InteriorDoorways to return the cache fallback
when _interiorWallBuilder is null, and in InvalidateBuilders() copy
_interiorWallBuilder.Doorways into the cache before clearing the builder
(alternatively, reconstruct the interior wall builder and repopulate its
Doorways before nulling it). Ensure CreateNavigationBuilder() and
AddInteriorDoorFrames() read from InteriorDoorways so door openings survive
subsequent WithConfig/WithPalette/DefineRoom calls.
---
Outside diff comments:
In `@Building/BuildingBuilder.cs`:
- Around line 135-156: BuildingBuilder.AddFloor and AddCeiling currently mutate
_config.Palette (setting FloorColor/CeilingColor and
FloorMaterial/CeilingMaterial) which makes one-off overrides sticky and can
mutate a palette supplied via WithPalette; instead, do not change
_config.Palette—capture the optional color/material into local variables and
pass them as explicit overrides into GetDecorBuilder().AddFloor(...) and
GetDecorBuilder().AddCeiling(...), or add overloads to those DecorBuilder
methods to accept the per-call color/material, so the shared _config.Palette
remains unchanged.
---
Duplicate comments:
In `@Building/BuildingBuilder.cs`:
- Around line 738-744: AddSlidingDoors currently invokes onCreated after
PlaceSlidingDoors returns, which is too late; change
PrefabPlacer.PlaceSlidingDoors to accept the same Action<GameObject>? callback
and have PlaceSlidingDoors invoke it before activating the prefab and on the
deferred client-link path, then update BuildingBuilder.AddSlidingDoors to pass
through the onCreated callback instead of invoking it locally (remove the local
post-return onCreated?.Invoke call) and adjust any other callers to the new
PlaceSlidingDoors overload.
- Around line 285-288: WithInteriorWallLayer currently only updates
_interiorWallLayer and becomes a silent no-op after the cached interior wall
builder is created in GetInteriorWallBuilder, so change WithInteriorWallLayer to
either validate timing or apply the new value to the cached builder: detect if
the cached InteriorWallBuilder instance exists (the field created by
GetInteriorWallBuilder), and if so update its layer property/state to the new
value; otherwise just set _interiorWallLayer as before. Also ensure behavior is
mirrored for the duplicate case referenced (lines 847-850) and consider throwing
an ArgumentException from WithInteriorWallLayer if you prefer to reject changes
after AddInteriorWall has been called.
- Around line 559-573: AddStairs currently stores only a partial tuple in
_stairs (wall, foundationHeight, width, lateralOffset) while the actual stair
spec (maxStepHeight, stepDepth, gap, flushWithFloor, style) is used to build
geometry, causing ComputeStairBasePosition to reconstruct a run from defaults
and mismatch navigation; fix by persisting the full stair spec when AddStairs
calls GetDecorBuilder().AddStairs (store a struct/class or full tuple including
maxStepHeight, stepDepth, gap, flushWithFloor, style and the lateralOffset) and
update ComputeStairBasePosition to read the stored spec (and validate or clamp
foundationHeight against _foundationHeight) instead of using hard-coded
constants so navigation aligns with the generated stair geometry.
- Around line 696-702: The AddPrefab method currently calls
GetPrefabPlacer().Place(prefab, position, rotation) and invokes onCreated after
Place returns, which is too late for Awake/OnEnable and deferred client-link
hooks; change AddPrefab (BuildingBuilder.AddPrefab) to pass the onCreated
callback into PrefabPlacer.Place (e.g., PrefabPlacer.Place(prefab, position,
rotation, onCreated)) and update PrefabPlacer.Place to provide an overload that
accepts and invokes that callback at the proper pre-activation/deferred-link
point inside Place so the callback runs during placement instead of after
placement returns.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e199fa33-f60d-4501-8cad-35f1146e5ef0
📒 Files selected for processing (1)
Building/BuildingBuilder.cs
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Building/BuildingPartRegistry.cs (1)
51-63: Consider pre-sizing the renderers list to reduce allocations.For large buildings with many parts,
GetComponentsInChildren<Renderer>()can return substantial arrays. The current implementation adds them one-by-one viaAddRange. A minor optimization would be to estimate capacity, though this is not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Building/BuildingPartRegistry.cs` around lines 51 - 63, GetRenderers currently builds renderers by repeatedly calling AddRange which causes multiple resizes; fix by first estimating total capacity then allocating the List once. Inside GetRenderers (use the existing _parts.TryGetValue and local list variable), do a first pass over list to sum the lengths of list[i].GetComponentsInChildren<Renderer>() (skip nulls), create renderers = new List<Renderer>(totalCount), then do a second pass to AddRange each non-null array and finally return renderers.ToArray(); this reduces intermediate allocations.Building/BuildingBuilder.cs (1)
166-174: Direct palette mutation may have unintended side effects.Lines 168-169 directly mutate
_config.Palette.FloorColorand_config.Palette.FloorMaterial. If the palette is shared (e.g., from a preset or passed viaWithPalette), this modifies the original instance rather than creating an override.Consider cloning the palette before mutation (similar to
AddWallsat lines 223-228) or documenting that overrides permanently modify the config.Proposed fix
public BuildingBuilder AddFloor(Color? color = null, Material? material = null) { - if (color.HasValue) _config.Palette.FloorColor = color.Value; - if (material != null) _config.Palette.FloorMaterial = material; + if (color.HasValue || material != null) + { + _config.Palette = _config.Palette.Clone(); + if (color.HasValue) _config.Palette.FloorColor = color.Value; + if (material != null) _config.Palette.FloorMaterial = material; + } var floor = GetDecorBuilder().AddFloor(_config.FloorThickness);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Building/BuildingBuilder.cs` around lines 166 - 174, AddFloor currently mutates _config.Palette directly (setting _config.Palette.FloorColor and FloorMaterial), which can unintentionally change a shared palette; update AddFloor to ensure the palette is cloned before applying overrides (follow the pattern used in AddWalls: create a new Palette instance or call the existing clone/Copy method on _config.Palette, assign it back to _config.Palette, then set FloorColor/FloorMaterial), so overrides only affect this builder's config and not any shared palette instances.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Building/BuildingBuilder.cs`:
- Around line 166-174: AddFloor currently mutates _config.Palette directly
(setting _config.Palette.FloorColor and FloorMaterial), which can
unintentionally change a shared palette; update AddFloor to ensure the palette
is cloned before applying overrides (follow the pattern used in AddWalls: create
a new Palette instance or call the existing clone/Copy method on
_config.Palette, assign it back to _config.Palette, then set
FloorColor/FloorMaterial), so overrides only affect this builder's config and
not any shared palette instances.
In `@Building/BuildingPartRegistry.cs`:
- Around line 51-63: GetRenderers currently builds renderers by repeatedly
calling AddRange which causes multiple resizes; fix by first estimating total
capacity then allocating the List once. Inside GetRenderers (use the existing
_parts.TryGetValue and local list variable), do a first pass over list to sum
the lengths of list[i].GetComponentsInChildren<Renderer>() (skip nulls), create
renderers = new List<Renderer>(totalCount), then do a second pass to AddRange
each non-null array and finally return renderers.ToArray(); this reduces
intermediate allocations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 046f3262-b11f-4b9e-81e3-911c7dc33a74
📒 Files selected for processing (5)
Building/BuildingBuilder.csBuilding/BuildingPartRegistry.csBuilding/BuildingUtilities.csBuilding/Structural/DecorBuilder.csUtils/Constants.cs
…ect destroyed objects
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Disclaimer: The summary of this PR was written by Claude and audited/edited by me.
This PR adds the building systems needed to make S1MAPI buildings feel like real game locations: interior walls, roofs, terrain preparation, NPC navigation, and post-build customization. It also hardens existing wall/decor geometry and fixes several IL2CPP compatibility issues.
34 files changed — 14 new files, 20 modified. ~8,200 lines added across 45 commits.
What's new
InteriorWallBuilder)RoofBuilder)TerrainFlattener) and tree/vegetation/object clearing (TerrainClearer), both with IL2CPP ICall support and auto-retry for late-loaded terrainSetDestinationinterception, multi-phase doorway entry/exit with stair lerping, combat-gated chase detection, directed NPC routing API (NavigationBuilder,InteriorNavigatorCore,InteriorPathGrid)BuildingPartRegistry)WallAppearance,DualMaterialBoxGenerator)NetworkedPrefabLinker)StairStyleenum)Constants.cs)What changed in existing code
BuildingBuilder— new fluent methods:AddInteriorWall,AddParapetRoof,AddHipRoof,AddCornerTrim,AddStairs,AddDoorFrames,AddInteriorDoorFrames,FlattenTerrain,CreateNavigationBuilder,WithInteriorWallLayer. ExposesRegistryandGridCellSizeproperties.WallBuilder— per-wallWallAppearanceoverrides, multiple windows per wall,WallOpening.Offsetfor off-center placement,DoorWithWindowsfactory, corner gap fix (N/S walls extend to cover E/W corners)DecorBuilder— stairs, corner trim, door frames (exterior + interior), base molding gaps around doors, molding protrusion fixPrefabPlacer—onCreatedcallback, networked prefab queue integrationPrefabRef—InstantiateNetworked()for FishNet-spawned prefabsLightingBuilder— URP-correct light defaults for Schedule I's pipelineProceduralMeshBuilder— all rendering layers enabled for decal projection supportforeach (Transform t in ...)with index-basedGetChild()loops across extensionsBreaking changes (binary)
Adding default parameters to existing public methods is a binary breaking change — consumer mods compiled against the old signature must recompile. Source compatibility is preserved (no code changes needed, just rebuild).
Affected methods:
BuildingBuilder.AddFoundationColor? color,Material? materialBuildingBuilder.AddBaseMoldingbool skipWallsBuildingBuilder.AddSlidingDoorsAction<GameObject>? onCreatedBuildingBuilder.AddPrefabAction<GameObject>? onCreatedDecorBuilder.AddBaseMoldingskipWalls+ door gap logicWallOpening.Doorfloat offsetWallOpening.WindowCount,DividerWidth,GlassMaterial,FrameColor,FrameMaterialAreas likely to be scrutinized
Harmony patch on
NPCMovement.SetDestinationThe interior navigator installs a prefix patch that intercepts every NPC
SetDestinationcall game-wide. If the destination resolves inside a managed building, the original call is blocked and custom A* routing takes over. This is the most invasive change in the PR.NavigationBuilderexists. Non-building destinations pass through unmodified. Combat detection uses reflection (no compile-time ScheduleOne dependency) with graceful fallback. The patch is removable viaNavigationBuilder.Remove().NPCMovementorSetDestination, the patch silently fails (no crash, NPCs just won't enter buildings). The reflection chain for combat detection (NPCMovement.npc → NPC.Behaviour → activeBehaviour) could break on game updates.Reflection-heavy NPC code
InteriorNavigatorCoreuses reflection for: NPC speed, NavMeshAgent access, destination clearing, combat behavior detection. All accessed viaMemberAccessorwithIsValidchecks and try/catch fallbacks. This is by design per coding standards (no ScheduleOne compile-time deps), but it's a lot of reflection surface area that could break on game updates. It might be worth having a conversation about.TerrainFlattener IL2CPP ICalls
TerrainFlatteneruses raw Unity ICalls (TerrainData::GetHeights_Internal,SetHeightsDelayLOD_Injected) on IL2CPP because the managedTerrainDatawrappers crash. This is fragile — Unity version changes could alter ICall signatures. The Mono path uses normal managed APIs. To my knowledge, this is the only way.Terrain clearing heuristics
TerrainCleareridentifies vegetation by name keywords (bush,tree,grass, etc.) and protects roads/fences by keyword. Misclassification is possible if a mod adds objects with unexpected names. TheFiltercallback is the escape hatch.Magic number density
Navigation thresholds, phase transition distances, and stuck detection timers are now in
Constants.InteriorNav, but the overall system has many tuning values that were calibrated for specific building sizes. Edge cases with very large or very small buildings may need threshold adjustments.AddWallssignature expansionThe
AddWallsmethod andWallOpeningfactories gained many optional parameters. While source-compatible, the parameter count is getting high. Future work might benefit from an options object pattern.Release Notes
Summary: 34 files changed, ~8,235 lines added and ~304 lines removed across the PR.