Updates to spawnPlacer for LastTerrain's and fixes to symmtry lines. #474
Conversation
WalkthroughReplaces parameterless symmetry-line drawing with a symmetry-parameterized API, updates generators and spawn placement to use it, subtracts a symmetry-inflated region from spawn masks before further constraints, and narrows which symmetry types trigger the mountain-heightmap pipeline. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (2)
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: 0
🧹 Nitpick comments (2)
shared/src/main/java/com/faforever/neroxis/map/placement/SpawnPlacer.java (2)
84-96: Check teamSeparationMask inflation math and symmetry-count edge casesThe new mask logic looks coherent, but a couple of edge cases are worth double‑checking:
teamSeparationMaskusesmap.getSize() + 1while you already havemapSize; consider usingmapSize + 1for consistency and to avoid accidental divergence ifmapSizeever changes.- For non‑perfect symmetry,
inflate((float) teamSeparation / spawnSymmetry.getNumSymPoints())can become very small for largenumSymPoints(e.g. 7/9‑way). That may effectively remove most of the intended team separation from the lines, leaving only the centralfillCircleto do the work. A smallmax(…, minRadius)clamp or at least an assertion thatgetNumSymPoints() > 0might make this more robust.- It might also be worth confirming via tests that the odd‑symmetry cases you care about (3, 5, 7, 9) still produce the expected visual separation at both small and large
teamSeparationvalues.
98-101: Spawn area restriction order and POINT2 double fillTwo related points here:
The new chain
fillSides(...).subtract(teamSeparationMask).fillEdge(mapSize / 32, false).limitToSymmetryRegion()
changes the effective playable area compared to the previous center‑fill approach. This looks intentional, but becausefillEdgehere uses/32while the otherplaceSpawnsoverload still uses/16, it’s worth confirming that the extra near‑edge freedom for this path is desired and tested for small maps and highspawnCount.In the loop below, for
Symmetry.POINT2you callfillCircle(symmetryPoint, teammateSeparation, false)twice (once unconditionally, once inside theifblock). That second pass is effectively a no‑op and appears to be leftover from an earlier version where radii differed. You can safely drop theif (spawnSymmetry == Symmetry.POINT2)block, or repurpose it if you actually intended a different radius there (e.g.teamSeparation).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
shared/src/main/java/com/faforever/neroxis/map/placement/SpawnPlacer.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
| BooleanMask teamSeparationMask = new BooleanMask(map.getSize() + 1, random.nextLong(), | ||
| spawnMask.getSymmetrySettings()); | ||
| teamSeparationMask.drawSymmetryLines(); | ||
| if (spawnSymmetry.isPerfectSymmetry()) { | ||
| teamSeparationMask.inflate(teamSeparation / 2f); | ||
| } else { | ||
| teamSeparationMask.inflate((float) teamSeparation / spawnSymmetry.getNumSymPoints()) | ||
| .fillCircle(mapSize / 2f, mapSize / 2f, mapSize / 4f, true); | ||
| } | ||
|
|
||
| BooleanMask spawnMaskCopy = spawnMask.copy(); | ||
| spawnMaskCopy.fillSides(map.getSize() / spawnCount * 3 / 2, false) | ||
| .fillCenter(teamSeparation, false) | ||
| .fillEdge(map.getSize() / 32, false) | ||
| spawnMaskCopy.fillSides(mapSize / spawnCount * 3 / 2, false) | ||
| .subtract(teamSeparationMask) | ||
| .fillEdge(mapSize / 32, false) |
There was a problem hiding this comment.
it is a rather big difference to change it from fillCenter to fillCircle. Fill center was filling along the symmetry center. So this would have a big impact on non point symmetry separation. I think just the symmetry lines is enough without changing from fill center to fill circle
There was a problem hiding this comment.
The cause could also be an impropely chosen team symmetry
There was a problem hiding this comment.
Hmm yep, so take2 attempt.
Keeping the fillCenter(), which yep, makes more sense to keep it.
And just subtract the symmerty lines.
Which has highlighted some bugs in the drawSymmetryLines() function actually...
Seems line the QUAD and DIAG symmetries weren't implemented proplerly in drawSymmetryLines(), which I have fixed now, noice....
And yeah, now I see why we need to pass the SymmetryType as a parameter to the drawSymmetryLines(), so I've updated that aswell, noice...
But yep anyway, this seems to fix these egde cases now where the fillCenter() isn't pushing the teams apart enough.
So the above neroxis_map_generator_snapshot_htauvztrkz3kc_bifq now does this:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
shared/src/main/java/com/faforever/neroxis/mask/BooleanMask.java (1)
803-841: Clarify coupling betweensymmetryparameter andteamSymmetry().getNumSymPoints()The new
drawSymmetryLines(Symmetry symmetry)nicely adds QUAD and DIAG support, but it still drives the “slice” logic offsymmetrySettings.teamSymmetry().getNumSymPoints()rather than the passed-insymmetry. That means:
- For
numSymPoints == 3or>= 5, the method ignores thesymmetryvalue entirely and always draws radial slices.- In the POINT3/5/7/9/11/13/15 case, the number of slices still comes from team symmetry, not from the
symmetryargument.If all callers always pass a symmetry consistent with
teamSymmetry(), this preserves old behavior; otherwise this can be surprising and hard to reason about. Consider either:
- Deriving
numSymPointsfrom thesymmetryparameter (or a helper onSymmetry), or- Documenting in Javadoc that this API is intentionally tied to
teamSymmetryand that thesymmetryargument only selects orientation/axes.This is mostly about API clarity and future maintainability rather than an immediate bug.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
generator/src/main/java/com/faforever/neroxis/generator/terrain/FractalNoiseLastTerrainGenerator.java(2 hunks)generator/src/main/java/com/faforever/neroxis/generator/terrain/MultiLevelLastTerrainGenerator.java(1 hunks)shared/src/main/java/com/faforever/neroxis/map/placement/SpawnPlacer.java(1 hunks)shared/src/main/java/com/faforever/neroxis/mask/BooleanMask.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- shared/src/main/java/com/faforever/neroxis/map/placement/SpawnPlacer.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
generator/src/main/java/com/faforever/neroxis/generator/terrain/MultiLevelLastTerrainGenerator.java (1)
121-129: Explicit terrain symmetry for water masks looks correctPassing
symmetrySettings.terrainSymmetry()intowaterArea.drawSymmetryLines(...)for bothSYMMETRY_LINEandHOUR_GLASSkeeps behavior aligned with terrain symmetry and matches the new API without changing the surrounding logic.Also applies to: 130-146
generator/src/main/java/com/faforever/neroxis/generator/terrain/FractalNoiseLastTerrainGenerator.java (2)
54-56: Symmetry lines initialization matches new BooleanMask APIUsing
symmetryLines.drawSymmetryLines(symmetrySettings.terrainSymmetry())is consistent with the new signature and with how symmetry lines are later used for mountains and blurring.
140-153: Confirm intended restriction of mountain ridge pipeline to POINT symmetriesThe mountain-heightmap pipeline now only runs when
terrainSymmetryis one ofPOINT2–POINT16, and falls back to “just addheightmapLand” for other symmetries (e.g., QUAD/DIAG). That’s a non-trivial behavioral change; please confirm this is intentional for spawn-last generators and that you’re happy to lose symmetry-line mountain ridges for non-POINT terrain symmetries.
.../src/main/java/com/faforever/neroxis/generator/terrain/FractalNoiseLastTerrainGenerator.java
Show resolved
Hide resolved
generator/src/test/java/com/faforever/neroxis/generator/MapGeneratorTest.java
Show resolved
Hide resolved
clivepaterson
changed the title
Sheikah45
force-pushed
the
team-separation-for-spawn-last
branch
from
7756066 to
75d075b
Compare
2 participants
As reported by Ravandel:
mapName = "neroxis_map_generator_snapshot_htauvztrkz3kc_bifq";
The
fillCircle()call inSpawnPlacer.java:tryPlaceSpawns()will either draw a symmetry line or a circle.So the above mapgen is doing this:
Summary by CodeRabbit
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.