Skip to content

Implement buoyancy sampling from FFT ocean surface in BuoyancySystem#395

Open
drsnuggles8 wants to merge 2 commits into
masterfrom
feature/ocean-fft-buoyancy
Open

Implement buoyancy sampling from FFT ocean surface in BuoyancySystem#395
drsnuggles8 wants to merge 2 commits into
masterfrom
feature/ocean-fft-buoyancy

Conversation

@drsnuggles8

@drsnuggles8 drsnuggles8 commented Jun 25, 2026

Copy link
Copy Markdown
Owner
  • Added OceanFFTField support to BuoyancySystem for accurate buoyancy calculations.
  • Introduced SampleHeightFFT method in WaterSurface for FFT-based height sampling.
  • Updated WaterBuoyancyTest to validate buoyancy behavior with FFT water.
  • Enhanced WaterSurfaceSamplerTest to ensure consistency between CPU and GPU height calculations.
  • Documented changes in WATER_FUTURE_IMPROVEMENTS.md to reflect new buoyancy sampling capabilities.

Summary by CodeRabbit

  • New Features

    • Buoyancy now follows FFT-based ocean surface heights when available, improving object floating behavior on FFT water.
    • Added CPU-side FFT water height sampling for consistent world-space surface queries.
  • Bug Fixes

    • Improved stability by handling invalid or non-finite water height values safely.
    • Added fallback behavior to keep buoyancy working when FFT sampling is unavailable.
  • Tests

    • Added coverage for FFT water surface sampling and buoyancy settling at FFT-driven heights.

- Added OceanFFTField support to BuoyancySystem for accurate buoyancy calculations.
- Introduced SampleHeightFFT method in WaterSurface for FFT-based height sampling.
- Updated WaterBuoyancyTest to validate buoyancy behavior with FFT water.
- Enhanced WaterSurfaceSamplerTest to ensure consistency between CPU and GPU height calculations.
- Documented changes in WATER_FUTURE_IMPROVEMENTS.md to reflect new buoyancy sampling capabilities.
@coderabbitai

coderabbitai Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@drsnuggles8, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 43 minutes and 24 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits.

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: ad69fc04-7eaa-46b1-89f0-bf09c327a79b

📥 Commits

Reviewing files that changed from the base of the PR and between 3507af9 and 6bc850d.

📒 Files selected for processing (1)
  • OloEngine/tests/Functional/AnimationPhysics/WaterBuoyancyTest.cpp
📝 Walkthrough

Walkthrough

Buoyancy now samples FFT-based ocean heights when an FFT field is available. A new CPU FFT height sampler was added, buoyancy tiles select and scale FFT fields at update time, and renderer and physics tests cover the new sampling path. Documentation was updated to mark the FFT buoyancy path as shipped.

Changes

FFT water height and buoyancy

Layer / File(s) Summary
WaterSurface FFT sampler
OloEngine/src/OloEngine/Renderer/WaterSurface.h, OloEngine/src/OloEngine/Renderer/WaterSurface.cpp
Adds SampleHeightFFT and the FFT field forward declaration, with finite-value fallback handling in the CPU sampler.
Buoyancy FFT integration
OloEngine/src/OloEngine/Physics3D/BuoyancySystem.cpp, docs/WATER_FUTURE_IMPROVEMENTS.md
Stores optional FFT field data on water volumes, selects FFT sampling during update, switches per-probe buoyancy height sampling when available, and updates the buoyancy documentation to reflect the shipped FFT path.
FFT sampler validation
OloEngine/tests/Rendering/PropertyTests/WaterSurfaceSamplerTest.cpp
Adds FFT sampler helpers and property tests for FFT height mapping, fallback behavior, scaling, and finiteness.
Buoyancy FFT validation
OloEngine/tests/Functional/AnimationPhysics/WaterBuoyancyTest.cpp
Adds FFT water setup helpers and a buoyancy functional test that checks a body settles near the sampled FFT surface height.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: buoyancy now samples the FFT ocean surface in BuoyancySystem.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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


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

❤️ Share

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

@drsnuggles8

Copy link
Copy Markdown
Owner Author

@CodeRabbit review

@coderabbitai

coderabbitai Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 `@OloEngine/tests/Functional/AnimationPhysics/WaterBuoyancyTest.cpp`:
- Around line 235-258: The FFT-vs-flat-plane discriminator in WaterBuoyancyTest
is too weak because the sampled height threshold in the candidate search is
lower than the final near-equality tolerance, so a body resting at y=0 can still
pass. Update the test around the SampleHeightFFT loop and the EXPECT_NEAR/Y(box)
check so the selected FFT displacement is guaranteed to exceed the rest
tolerance with margin, or add an explicit assertion that the final buoyant box
height is not near 0.
🪄 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: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: a28af123-bc3b-4c2b-b75a-7f43ca7ce4f3

📥 Commits

Reviewing files that changed from the base of the PR and between 288cfc9 and 3507af9.

📒 Files selected for processing (6)
  • OloEngine/src/OloEngine/Physics3D/BuoyancySystem.cpp
  • OloEngine/src/OloEngine/Renderer/WaterSurface.cpp
  • OloEngine/src/OloEngine/Renderer/WaterSurface.h
  • OloEngine/tests/Functional/AnimationPhysics/WaterBuoyancyTest.cpp
  • OloEngine/tests/Rendering/PropertyTests/WaterSurfaceSamplerTest.cpp
  • docs/WATER_FUTURE_IMPROVEMENTS.md

Comment thread OloEngine/tests/Functional/AnimationPhysics/WaterBuoyancyTest.cpp Outdated
CodeRabbit (PR #395) correctly flagged that RestsAtTheFFTSurfaceHeight's
search threshold (|h| > 0.3) was smaller than the rest-height tolerance
(0.6), so the EXPECT_NEAR band around expectedSurfaceY could include y=0 —
a body that fell back to the flat Gerstner plane would still pass, making
the FFT-vs-plane discriminator vacuous.

Now scan the whole patch for the most-displaced column (a crest/trough,
where the box also tracks best since an extremum is a local gradient zero)
and assert the displacement clears 2x the tolerance, so the EXPECT_NEAR
band provably excludes y=0.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@drsnuggles8

Copy link
Copy Markdown
Owner Author

🤖 Self-review (finish-pr) @ 6bc850d32dcbc36d2ba4826684cfa2f8e48f1dd4

Reviewed the full PR diff (origin/master...HEAD) at high effort — 8 finder angles (correctness ×3, cleanup ×3, altitude, conventions) + verification. Rebuilt OloEngine-Tests and re-ran the Water/Ocean/Buoyancy suite (146 pass) plus the FFT cases after each change.

Findings: 0 hard correctness bugs. CodeRabbit's one thread (weak flat-plane discriminator) was valid and is fixed in this commit.

Dismissed (with reasons, no silent drops):

  • FFT proxy is one render-frame behind physics if a frame ticks physics without rendering — by design (buoyancy samples the renderer-maintained CPU proxy; scoped as plumbing, not re-simulation), documented in WATER §5.1, sub-frame bob lag only; invalid fields still fall back to Gerstner.
  • m_FFTHeightScale clamp (0,20,1) duplicated across 5 sites — real drift risk but the fix touches Components.h + Scene/Serializer/Lua (out of this PR's scope). Recommended follow-up: a shared WaterComponent::ClampFFTHeightScale() helper. This PR mirrors the render-path clamp exactly and comments why.
  • per-probe sampler ternary is loop-invariant; asymmetric sampler signatures; duplicated column-scan scaffolding — marginal nits (8 trivial branch checks; the two samplers take genuinely different inputs; the two scans have different semantics).
  • headless tests use the full-spectrum CPU field, not the shipped band-limited GPU proxySampleHeightFFT reads field.SampleHeight() regardless of how the field was built (contract test pins that mapping); the band-limiting path is covered separately by OceanFFTSpectrumTest.BandLimitedFieldTracksFullResolutionSurface.

Behavior check: diff changes physics sampling only — zero rendering code — so the rendered frame is unchanged; runtime behavior (floater rest height on FFT water) is verified by WaterBuoyancyTest.RestsAtTheFFTSurfaceHeight. Editor visual pass not applicable.

@drsnuggles8 drsnuggles8 added the self-reviewed finish-pr self-review done label Jun 25, 2026
@sonarqubecloud

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
B Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

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

Labels

self-reviewed finish-pr self-review done

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant