fix(unified-water): validate water cells and clear stale LOD#2425
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds exterior/interior state and recursive LOD water culling across worldspace/terrain/shader hooks; introduces IsValidCellHeight, sentinel initialization, precache fallback/missing-data/invalid-height counters in BuildDiskCache, and filters/skips invalid or out-of-bounds runtime cache instructions. ChangesUnified water LOD culling and exterior state
Water cache validation and runtime guarding
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.22.0)OpenGrep fatal error (exit code 2): [00.20][ERROR]: Error: exception Unix_error: No such file or directory stat src/Features/UnifiedWater.cpp 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 |
|
No actionable suggestions for changed features. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Features/UnifiedWater/WaterCache.cpp`:
- Around line 378-389: The current validHeights only checks finiteness and
FLT_MIN/MAX but the absolute-value range check (fabs(waterHeight) < 50000.0f) is
applied only to waterHeight later, allowing garbage landHeight to slip through;
update the validHeights computation to include both fabs(waterHeight) < 50000.0f
and fabs(landHeight) < 50000.0f so the later conditional can rely on
validHeights, keep the same branch that assigns worldSpace->worldWater->formID
when validHeights && waterHeight > landHeight, and ensure
skippedInvalidHeightCount continues to increment when !validHeights (so
out-of-range waterHeight/landHeight are counted).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5d4a3ad0-f969-4083-b76d-d6580a130a00
📒 Files selected for processing (1)
src/Features/UnifiedWater/WaterCache.cpp
|
✅ A pre-release build is available for this PR: |
FIocker
force-pushed
the
fix/unified-water-cell-height-validation
branch
from
f0f1fcf to
257b358
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Features/UnifiedWater/WaterCache.cpp (1)
1-1: Add an issue-closing reference in the PR description if there is a tracked bug.For traceability, consider adding
Fixes #<id>orCloses #<id>in the PR body.As per coding guidelines, "Issue References (if PR fixes bugs or implements features): Suggest adding appropriate GitHub keywords".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Features/UnifiedWater/WaterCache.cpp` at line 1, The PR is missing an issue-closing reference for tracked bugs; update the PR description/body to include a GitHub keyword like "Fixes #<id>" or "Closes #<id>" referencing the relevant issue number so the PR will auto-close the tracked bug (no code changes needed; just edit the PR body to add the appropriate line).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Features/UnifiedWater/WaterCache.cpp`:
- Around line 387-390: When assigning landHeight and waterHeight from the
precache array, guard the access by validating idx against the container size:
before using preCache.heights[idx] (the code around preCache, heights, idx,
landHeight, waterHeight, precacheFallbackCount), check that idx <
preCache.heights.size(); if not, handle the malformed precache by skipping the
read and applying a safe fallback (e.g., leave default heights, log or mark the
entry as invalid, still increment precacheFallbackCount as intended) to avoid
out-of-bounds reads and crashes during cache generation.
---
Nitpick comments:
In `@src/Features/UnifiedWater/WaterCache.cpp`:
- Line 1: The PR is missing an issue-closing reference for tracked bugs; update
the PR description/body to include a GitHub keyword like "Fixes #<id>" or
"Closes #<id>" referencing the relevant issue number so the PR will auto-close
the tracked bug (no code changes needed; just edit the PR body to add the
appropriate line).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 69373218-2091-4beb-8d02-54f77d6c1488
📒 Files selected for processing (1)
src/Features/UnifiedWater/WaterCache.cpp
FIocker
force-pushed
the
fix/unified-water-cell-height-validation
branch
from
257b358 to
cdb725b
Compare
FIocker
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Features/UnifiedWater.cpp`:
- Line 543: UpdateWaterLODCull() iterates the whole generated water subtree and
is being called from hot paths (geometry setup and displacement update hooks),
causing O(n) per-frame/per-pass work; modify the callers to avoid repeated full
reculls by caching the last applied exterior and cull state (e.g., store
lastExteriorState and lastCullState on the singleton or an associated
WaterLODManager) and only call singleton.UpdateWaterLODCull() when those cached
values differ from the new state; update the geometry setup and displacement
update hooks to check and update the cache before invoking UpdateWaterLODCull(),
and add a config/user toggle to disable automatic LOD recull or run it
asynchronously to limit render-path performance impact.
- Around line 366-371: UpdateWaterLODCull currently calls SetAppCulledRecursive
on *gWaterLOD which clears child-level SetAppCulled flags set by
BGSTerrainNode_UpdateWaterMeshSubVisibility::thunk; change the call so it only
sets the root node's cull state (e.g., call SetAppCulled on *gWaterLOD instead
of SetAppCulledRecursive) so child explicit cull flags are preserved when
exterior water comes back.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e7977f73-117f-49c6-9bb2-3421fedfb745
📒 Files selected for processing (3)
src/Features/UnifiedWater.cppsrc/Features/UnifiedWater.hsrc/Features/UnifiedWater/WaterCache.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Features/UnifiedWater/WaterCache.cpp
FIocker
force-pushed
the
fix/unified-water-cell-height-validation
branch
from
cdb725b to
1f6a178
Compare
FIocker
force-pushed
the
fix/unified-water-cell-height-validation
branch
from
70092a0 to
60c8f3d
Compare
FIocker
changed the title
FIocker
changed the title
2 participants
Fixes bad Unified Water tiles being generated from cells where height data could not be read cleanly and cull UW LODs when entering interiors
Changes: