Fix storyloc structure spawning

[tehtelev]

Summary

Issue:

• No check kept a story structure's Y-coordinates inside the world bounds. A structure could spawn below bedrock (Y < 0) or poke above the max world height (Y2 > MapSizeY), which crashes the chunk generator; and when a previously generated structure was re-accessed (WorldgenHeight >= 0), startPos.Y was recomputed from raw terrain height instead of the saved coordinates, pushing it out of bounds again.
• With reduced world heights (e.g. worldHeight = 128), story or mod-added structures taller than the usable vertical space could never be placed, leaving their locations impossible to complete.

Changes:

• Added ClampStructureY, which constrains a structure's Y range to the valid world interval [1, MapSizeY - 1]. Why 1 and not 0: at Y = 0 lies the mantle/bedrock layer, so reserving it keeps structures from replacing bedrock (and the player can otherwise fall into that layer). The clamp runs for every placement type (UseWorldgenHeight, SurfaceRuin, Surface and underground), recomputing the lower edge first and then enforcing the upper bound by pulling the base down if needed, so the top can never exceed the world. It is now a plain static method (no ref) that only transforms its inputs.
• Structures taller than the usable height are no longer shifted silently: they are rejected up front in InitWorldGen. The guard was changed from sizeY > MapSizeY to sizeY > MapSizeY - 1 (usable range [1, MapSizeY)), which rejects a structure exactly as tall as the world and closes the underground path that never reached the clamp before. Such structures are skipped once per session with an error log (stratumTooTallStructures).
• Structures shifted to fit now emit one warning per session (stratumReportedShifted) instead of logging silently.
• Fixed re-loading: startPos.Y is always derived from the saved strucloc.Y1, never recomputed from raw terrain when WorldgenHeight >= 0.
• Removed logic duplication across placement branches; replaced .First(...) with .FirstOrDefault(...) plus a null check to avoid exceptions.
• Extracted the height guard into a standalone IsStructureTooTall static method and the reserved lower boundary into a stratumMinValidY field, so InitWorldGen and the test suite share a single source of truth for the boundary formula.

Tests:
Added StoryStructureBoundaryScenarios to the StratumScenarios suite. The scenario boots a patched server through Atlas and exercises both ClampStructureY and IsStructureTooTall via reflection on realistic world heights (128 from the bug report, 256 vanilla default, 448 tall-world option), covering:

• The arithmetic edge case where a structure exactly as tall as the world starts at the mantle (lower clamp lifts Y1 to 1, top lands exactly on maxY).
• The underground placement path that previously skipped the clamp entirely (CenterPos.Y pinned to 1, so a structure as tall as the world wrote its last block at Y = sizeY).
• The upper branch pulling the base down when the top would exceed maxValidY.
• Mod-added structures that motivated the PR: the 150-tall university from BetterRuins rejected on a 128-world and accepted on a 256-world.
• Shipped vanilla content: the 101-tall underground structure accepted on every sane world height.

Result:
Structures of any height generate at any world height without crashes or top clipping. Structures taller than the usable vertical space are skipped (logged once) instead of overflowing the world. Both boundary methods are pinned by a regression test, so the formula cannot drift without breaking the suite.

Type

• Bug fix
• Performance
• New feature
• Refactor or cleanup
• Docs or build

Checklist

• .\scripts\extract-patches.ps1 ran clean.
• dotnet build VintageStory.slnx -c Release is green.
• Every vanilla edit has a // Stratum marker.
• No vanilla source committed.
• Tested on a real server start, not just compilation.

Performance numbers

No changes

Related issues

World setting - default, but world height - 128.
Seed - 1347010773

Used mods:
BetterRuinsv0.6.3

Before
Structures break or get cut off at the top (depending on luck).

/tpstoryloc devastationarea

/tpstoryloc devastationarea

/tpstoryloc sunrift

After
All story and mod-added locations generated successfully, without crashes or distortions. The University structure failed to generate entirely, as it did not fit within the world's vertical height limits.

/tpstoryloc devastationarea

/tpstoryloc devastationarea

Sunrift structure will generate but may protrude slightly from the ground.
/tpstoryloc sunrift

The same structure with a world height of 256 or greater.

If the lower threshold for sunrift structure spawning is set to int minValidY = 0;, we see that the structure replaces the mantle.

[tehtelev]

The only location that didn't generate is the "university" from the Better Ruins mod. It might be taller than the world height limit of 128. I need to check.

[tehtelev]

Yes. The university structure has a height of 150. With a world height of 128, it simply failed to generate.
I added a check for the structure's height before checking the boundaries. Now, an error notification is also logged stating that the structure cannot be generated.

[Zaldaryon]

Changes requested\n\nThe current head still moves a structure whose lower edge is exactly .\n\nIn , sets and clamps when . That still shifts to , contrary to the boundary rule described in the patch. Please clamp only values below zero, or document and test why block is invalid.\n\nThe patch also spells the closing marker as . Please correct it to so the marker remains recognizable.\n\n## Verification\n\n- Reviewed the complete current head .\n- Confirmed the PR targets .\n- The PR has no reported CI checks.\n- The existing bootstrap, build, and smoke evidence does not cover the valid boundary case.

[Zaldaryon]

Changes requested

The current head still moves a structure whose lower edge is exactly Y=0.

In patches/VSSurvivalMod/Systems/WorldGen/Standard/ChunkGen/6.GenStructures/Story/GenStoryStructures.cs.patch, ClampStructureY sets minValidY = 1 and clamps when blockMinY < minValidY. That still shifts Y=0 to Y=1, contrary to the boundary rule described in the patch. Please clamp only values below zero, or document and test why block Y=0 is invalid.

The patch also spells the closing marker as // Startum end. Please correct it to // Stratum end so the marker remains recognizable.

Verification

• Reviewed the complete current head 087e1fc3.
• Confirmed the PR targets indev.
• The PR has no reported CI checks.
• The existing bootstrap, build, and smoke evidence does not cover the valid Y=0 boundary case.

Removed duplicate review after correcting its formatting in the follow-up review.

[tehtelev]

Yes. I’ve added a clarification to the description explaining exactly why Y must be 1 rather than 0. This is because structures can replace any block; the mantle/bedrock layer is at coordinate 0, and during testing, a structure could easily overwrite it, which poses a potential risk to players.

I updated the comments and ran the build and smoke test; both passed successfully.

[Zaldaryon]

Changes requested

The latest head fixes the two earlier points: it changes the closing marker to // Stratum end and documents why the patch reserves Y=0.

One boundary case remains in patches/VSSurvivalMod/Systems/WorldGen/Standard/ChunkGen/6.GenStructures/Story/GenStoryStructures.cs.patch. ClampStructureY reserves minValidY = 1, but the guard rejects only sizeY > maxY. With maxY = 128, sizeY = 128, and an initial lower edge of Y=0, the lower clamp moves blockMinY to 1. blockMaxY was calculated as 127 before that move, so the upper-bound branch does not run, and the final strucloc.Y2 becomes 129. The structure can still extend above the valid world range.

Please reject structures larger than the available range [minValidY, maxY), or recompute and enforce the upper bound after the lower clamp. Add a test for a structure exactly as tall as the world with the reserved lower boundary.

Verification

• Reviewed the complete current head a34c059d.
• Confirmed the PR targets indev.
• Confirmed the corrected marker and the new Y=0 explanation.
• The PR reports no CI checks.
• Existing build and smoke evidence does not cover the exact-height boundary case.

[Pixnop]

I went through the current head and I confirm Zaldaryon's open point: with a world height of 128 and a structure 128 blocks tall whose lower edge starts at 0, the lower clamp moves blockMinY to 1 while blockMaxY was already computed as 127, so the upper branch never runs and strucloc.Y2 ends at 129.

There is a second way into the same hole, and it matters more, because one line closes both.

Underground placement never reaches ClampStructureY. The three branches test UseWorldgenHeight, then SurfaceRuin, then Surface. Anything else falls through with isDirty false, so the only thing that happens is startPos.Y = strucloc.Y1, exactly as before the PR. That is not a hypothetical branch: in the game's own storystructures.json, resonancearchive is declared placement: "underground", and no story structure at all sets useWorldgenHeight, so that first branch is dead for vanilla content. The Y of those structures comes from DetermineStoryStructures, which rebuilds the cuboid from CenterPos.Y, and CenterPos.Y is pinned to 1 for every placement that is not Surface or SurfaceRuin. A structure sitting at Y=1 writes its last block at sizeY, so it lands outside the world exactly when sizeY equals the world height. The guard misses that by one, since 128 > 128 is false, and the clamp that would have caught it is never called on this path.

To be fair about the size of it: the shipped underground structure is 101 blocks tall, so nothing in vanilla overflows today. This bites a mod-added story structure as tall as the world, which is the same family of content that motivated the PR in the first place.

Changing the guard to reject anything that cannot fit in the usable range, sizeY > maxY - minValidY, closes the underground path and Zaldaryon's arithmetic case in one test, without touching the branches. With minValidY reserving the mantle, the usable range really is 127 blocks in a 128 world, so a structure of exactly 128 has nowhere to go and rejecting it with the existing log line is the honest outcome. Worth noting while you are in there: blockMaxY = maxValidY in the upper branch is a dead store, nothing reads it afterwards, and that is precisely why the lower clamp can push the top back out.

Smaller things:

• The third hunk carries no // Stratum marker and no fix. Its only two edits are if(structure.GenerateGrass) becoming if (structure.GenerateGrass) and an opening brace moved onto its own line. That is 22 of the 205 lines in the patch, and it will come back as conflict noise on the next vanilla bump. Three more cosmetic edits sit inside the marked region: the Intersects guard split across two lines, and the two else if( spacing fixes. CONTRIBUTING asks to match the surrounding style rather than reformat, so reverting those five lines in the working tree and re-running extract-patches shrinks the patch for free.
• The comment above stratumReportedTooTall uses an em dash, which the style section rules out. A colon does the job.
• The branch is 50 commits behind indev and carries a commit named "comment edits". None of those 50 commits touch this patch or its worldgen path, so the rebase should be uneventful, but 17 of them build the tests/StratumScenarios suite, which is where a boundary test would live. A rebase plus a squash into one commit with a descriptive subject would leave a clean history.

Questions rather than requests:

• The clamp translates a structure instead of refusing it, silently. A SurfaceRuin 60 blocks tall on terrain at 50 ends up floating, and a Surface building 100 blocks tall in a 128 world ends up buried below sea level, with no log line either way. Is a shifted structure preferable to a skipped one, and should it say so in the log?
• The config lookup and the height guard moved ahead of the Intersects test, so every generated column now pays a linear scan per story structure instead of only the columns the structure actually touches. Six structures make that cheap, but the section says "No changes" for performance. Computing the too-tall set once in InitWorldGen would remove both the per-column cost and the need for the session HashSet.
• ClampStructureY takes ref Cuboidi and ref BlockPos, both reference types, and does not use instance state. Dropping the ref keywords and making it static would say what it does more plainly.

On the test Zaldaryon asked for: a full Atlas scenario boots a server per test class, which is heavy for a world height of 128. If you would rather not go that far, making the clamp static and testable on its own would still pin the boundary case, and that is the part that keeps regressing.

[tehtelev]

Reply to @Zaldaryon

one boundary case remains … ClampStructureY reserves minValidY = 1, but the guard rejects only sizeY > maxY … the final strucloc.Y2 becomes 129. Please reject structures larger than [minValidY, maxY), or recompute and enforce the upper bound after the lower clamp. Add a test for a structure exactly as tall as the world with the reserved lower boundary.

Fixed on both fronts:

• Guard now rejects the whole family. Changed from sizeY > MapSizeY to sizeY > MapSizeY - 1, i.e. anything that cannot fit in [1, MapSizeY) is rejected up front in InitWorldGen. A structure exactly as tall as the world (sizeY = 128 in a 128 world) never reaches the clamp, so it can no longer overflow to Y2 = 129.
• Upper bound is enforced after the lower clamp. ClampStructureY now computes maxAllowedMinY = maxValidY - sizeY + 1 and pulls the base down when exceeded — there is no separate dead blockMaxY store anymore, so the top can never leave the range.
• Local test done for the exact-height case (structure as tall as the world with minValidY = 1). Verified against the current code:

case	result
sizeY=128, y1=0	rejected by guard (128 > usable 127)
sizeY=127, y1=0	shifted to minY=1, topEdge(Y2)=128, last block at 127 — fits exactly

Reply to @Pixnop

change the guard to reject anything that cannot fit in the usable range, sizeY > maxY - minValidY, closes the underground path and Zaldaryon's arithmetic case in one test

Done exactly like this. The same guard now rejects oversized structures on the underground path too: a structure sitting at Y = 1 whose sizeY == MapSizeY is rejected before any placement branch, so it can't land at sizeY. The shipped 101-block underground structure still generates (topEdge=102, fits).

blockMaxY = maxValidY in the upper branch is a dead store … that is precisely why the lower clamp can push the top back out.

Removed — replaced with the maxAllowedMinY recompute described above.

The third hunk carries no // Stratum marker … only two edits are if(structure.GenerateGrass) → if (structure.GenerateGrass) and a moved brace … revert those five lines in the working tree and re-run extract-patches

Reverted the formatting-only changes so the patch stays minimal.

The comment above stratumReportedTooTall uses an em dash … A colon does the job.

Rephrased with a colon; the field is now named stratumReportedShifted.

Rebase plus a squash into one commit with a descriptive subject would leave a clean history.

Planned for the git step (not done yet). The branch rebase + single squashed commit is part of the pending work.

On your questions rather than requests:

• Shift vs refuse, and should it say so in the log? Oversized structures are now refused outright with an error (stratumTooTallStructures, once per session); structures shifted to fit emit one warning per session (stratumReportedShifted). Neither path is silent anymore.
• Per-column cost from moving the guard ahead of Intersects. The too-tall set is computed once in InitWorldGen; the only thing before Intersects now is a HashSet.Contains, and the config lookup (FirstOrDefault) + size check stay after Intersects, so columns that don't intersect still pay only one cheap membership test.
• ref on reference types. Dropped — ClampStructureY is now a plain static bool ClampStructureY(Cuboidi strucloc, BlockPos startPos, int sizeY, int maxY).
• Test approach. Kept it as a focused unit test on the static clamp + guard boundary rather than a full Atlas scenario.

The build completes successfully, and the smoke test proceeds to the GameReady stage without errors.

[Zaldaryon]

Changes requested

1. Ignore my whitespaces messages.

Sorry for them.

2. Commit the exact-height boundary test

The current arithmetic covers the requested case, but the PR diff contains only the patch file. The latest author evidence describes a local check for sizeY = 128 and sizeY = 127 with the reserved lower boundary, but no test is committed that would fail if the guard or ClampStructureY regresses. Add the focused test requested in the prior review, or add it to the existing scenario suite if that is the supported test location.

3. Mark the new vanilla fields

The additions at the top of the patch introduce stratumTooTallStructures and stratumReportedShifted, but their comments begin with // key structure code and do not carry a // Stratum marker. Please mark this vanilla-only block as required by CONTRIBUTING.md.

Verification

• Reviewed the complete current head 4f39a0c3df0b9ae37e4aadc5e6471c52fc499f08.
• Confirmed the PR targets indev.
• Confirmed the underground path now reaches ClampStructureY and the guard uses the usable range.
• git diff --check origin/indev...HEAD reports the added patch whitespace, including the Y2 line above.
• The current PR diff contains no test file and GitHub reports no CI checks.

[tehtelev]

Changes made:

• Height check unification: Extracted the validation logic into a separate static method, IsStructureTooTall, and moved the mantle layer reservation to a constant, stratumMinValidY. Both InitWorldGen and the tests now rely on a single source of truth, preventing formula desynchronization during future edits.
• Test scenario: Added StoryStructureBoundaryScenarios to the StratumScenarios suite. This scenario uses reflection to test the actual patch methods against real-world heights (128, 256, 448), covering all edge cases raised during the review: overflow when starting at Y=0, skipping the underground branch (centered at Y=1), and upper/lower branch functionality (including verification of the 150-block university from BetterRuins).

The build completes successfully, and the smoke test proceeds to the GameReady stage without errors.
All 18 scenarios (including the new one) concluded successfully.

Don't scold me for the whitespaces; there's nothing I can do about it.😥

[Zaldaryon]

The boundary arithmetic and the committed regression scenario are now present

git diff --check origin/indev...HEAD reports 12 trailing-whitespace errors in
patches/VSSurvivalMod/Systems/WorldGen/Standard/ChunkGen/6.GenStructures/Story/GenStoryStructures.cs.patch.
Each reported line is an added blank line containing a space. Remove those
spaces from the generated patch and rerun git diff --check before requesting
the final review.

Verification

• Reviewed the complete current head 22afd1.
• Confirmed the PR targets indev.
• Confirmed the exact-height boundary test is committed in
  tests/StratumScenarios/StoryStructureBoundaryScenarios.cs.
• The PR reports no repository CI checks.

[Zaldaryon]

tehtelev, I want to apologize for the whitespace requests in my reviews on this PR today, especially reviews 5190483161 and 5191378847. I treated formatting in the generated patch as a blocking issue, and that was not appropriate. Please disregard those requests. I am sorry for the unnecessary churn.

Whitespace requirement is void.

[Zaldaryon]

Approving the current head. The boundary guard now rejects structures that cannot fit in the usable world range, and the committed regression scenario covers the exact-height, lower-boundary, upper-boundary, and underground cases. The earlier whitespace finding is withdrawn and is not a blocker.

Superseded by the current head and the new approval. The earlier review is no longer an active blocker.

[Zaldaryon]

Pixnop, the current head 172a783 has Zaldaryon's approval. Your review is the only remaining review gate for this PR. Please re-review the current head when you can.

[Pixnop]

The fix itself is right, and I checked it rather than reading the description. From a clean vanilla tree the patch applies, the post-image hash matches the header, every hunk count is consistent, and the added lines carry no tabs. The guard is the one we asked for, sizeY > maxY - stratumMinValidY, computed once in InitWorldGen behind the same genStoryStructures early return that protects the column pass, so the underground path is covered and the cache can never be read before it exists. The clamp recomputes the ceiling after lifting the floor, the dead store is gone, the config lookup sits after Intersects again, and the three formatting edits from the first round were reverted. I recalculated all eighteen rows of the new test by hand and they are correct, and the reflection fails loudly when a method is missing instead of going green.

One thing blocks the merge, and it is not about this fix.

The branch carries all of #313. Commit 89220a1, "Fix vanished player privacy leaks", brings in 10 files and 953 lines of the vanish work: both server patches, the staff command state, the privacy scenarios and their docs. That is about three quarters of what this PR now diffs against indev. The content is byte identical to the head of #313 I approved earlier, so nothing wrong would land, but two open PRs would be shipping the same commit under different names, and the second one to merge would either conflict or turn out empty. My guess is a merge from a local branch that already had #313 checked out. Once #313 lands, a rebase onto indev drops that commit on its own; if you would rather not wait, an interactive rebase that removes it does the same today.

Two things to do at the same time as that rebase:

• Run the suite on the head you actually push. The last comment reports 18 scenarios passing, but the branch holds 20, the 17 from the suite plus the two vanish ones plus yours, so that run predates the merge that pulled #313 in.
• Bring the suite README along. It says nineteen scenarios and nine server boots, which is #313's count; with your class it is twenty and ten.

Smaller, none of it blocking. Three vanilla blank lines are dropped outside any marked block, one before blockLayerConfig and two around ExecuteOrder; I would not have raised whitespace again after today's thread, except that these are the only edits in the patch that a future vanilla bump has to carry for no reason, so reverting them in the working tree and re-running extract-patches keeps the diff honest. stratumMinValidY never changes and could be a const. The performance section says "No changes", but the change is real and in the good direction: the per-column path went from a linear scan plus a size check on every generated column to one HashSet.Contains, and that is worth stating rather than hiding. And the five commits, "Fix unfixable" included, would read better squashed into one with a subject that says what the fix does.

Once the branch contains only its own work and the suite has run on that head, this has my approval.

[Pixnop]

Approved.

The fix was already verified in the last round: the patch applies to a clean vanilla tree, the post-image hash matches, the guard and the clamp do what the issue asked, and the eighteen rows of the boundary test check out by hand. What blocked was the branch, not the code, and that is settled: it now carries only its own four commits on top of indev, with the same tree as before, and the merge commit is gone.

The suite README count moves out of this pull request: #333 drops the number altogether, as you suggested, since the run prints it. I am running the scenario suite on this exact head on my side and will post the numbers here when it finishes.

[Pixnop]

Suite run on this head (a5b74f2, same tree as the branch after the rebase), through make scenarios on Linux with the patched build embedded (the prepare launch reported 10 patched files applied, so this was not the vanilla fallback): 20 passed, 0 failed, 1 min 53 s of test time. All ten classes ran, StoryStructureBoundaryScenarios included. Nothing left from the review on my side.