gz_waves: core - Part 1/4#882
Conversation
… Eval facade Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Carlos Aguero <caguero@honurobotics.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Carlos Agüero <caguero@honurobotics.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Carlos Agüero <caguero@honurobotics.com>
GZ_WAVES_PARAM_TABLE is now the single source of truth for the WaveParameters fields. operator<< / operator>>, ParseSdf, and the set_parameters service ApplyParam all expand it, so adding a parameter means editing one list instead of four. Field order and string-quoting are guaranteed consistent between the stream operators. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Carlos Agüero <caguero@honurobotics.com>
cd5cbc6 to
9698e33
Compare
….md) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Carlos Aguero <caguero@honurobotics.com>
9698e33 to
33bb4e8
Compare
|
@bsb808 , as we discussed, added WAVES_DESIGN.md and AGENTS.md. |
|
Are there any user end-to-end tests that we should do to verify the PR? |
| @@ -0,0 +1,645 @@ | |||
| # VRX Wave Simulation — Design & Contributor Reference | |||
|
|
|||
| This document is the single reference for the VRX wave-simulation stack. It | |||
There was a problem hiding this comment.
The doc uses several near-synonyms for overlapping concepts: "stack", "engine", "backend", "provider", "wave model" / "wave rendering model", and "generator". Suggest standardizing, with a one-line glossary near the top:
- wave field engine (WFE) = a concrete
IWaveFieldbackend (Gerstner, FFT). (Retire "backend" and "wave (rendering) model" as synonyms — "rendering model" especially misleads, since an engine drives physics too, not just the visual.) - provider = the package that ships an engine (
gz_waves_provider_*). - stack = the collection of
gz_waves*packages (the integration layer). - wave field = the water-surface state the engine produces (see the note at L17).
Optionally, describe what an engine computes using the three orthogonal choices from the VRX waves roadmap: (1) inverse transform (FFT vs direct summation), (2) kinematics (e.g. Gerstner), (3) statistics (deterministic vs random).
There was a problem hiding this comment.
Added a Terminology glossary standardizing on wave field / wave field engine (WFE) / provider / consumer (left "stack" out per the above): 2ed9d0fc.
| **per-package implementation details**, and a step-by-step **guide to | ||
| contributing a new wave engine** (a new "wave rendering model"). | ||
|
|
||
| The stack is made of these packages: |
There was a problem hiding this comment.
Because the PR is split into parts, a reader of this PR sees only gz_waves/ on disk, so this package table reads as if all four packages are present when three aren't yet. Suggest noting that all the gz_waves* packages live in this repo (vrx), landing across the four-part series — core here, then Gerstner, rendering, and FFT in the follow-up PRs — while EncinoWaves is the one external dependency (its own repo, installed separately, not vendored).
Since I'm reviewing these in order, it is tough to say if all the elements of the wave field engine are externalized in the EncinoWaves example.
There was a problem hiding this comment.
Now states all gz_waves* packages live in this repo with EncinoWaves as the lone external dependency: 2ed9d0fc.
| This document is the single reference for the VRX wave-simulation stack. It | ||
| captures the **requirements**, the **architecture and design decisions**, the | ||
| **per-package implementation details**, and a step-by-step **guide to | ||
| contributing a new wave engine** (a new "wave rendering model"). |
There was a problem hiding this comment.
I think we need to sort out the details here. I can't make sense if it, so hoping we can decide on a "what" we want to do before the implementation. The "wave field engine" includes the 5 choices above. We mix and match these terms (my fault, legacy from my earlier work). I think what we want is for the WFE to externalize all of these choices. Currently it isn't clear to me (or to Claude) of the choice of wave kinematics is locked into this repo (there are lots of references to Gerstner) or can external engines implement different kinematic models?
There was a problem hiding this comment.
Clarified that IWaveField is agnostic to all five choices, so any engine (in this repo or external) picks its own kinematics. See 2ed9d0fc.
Address bsb808's review of WAVES_DESIGN.md / AGENTS.md:
- add a Terminology glossary (wave field, WFE, provider, consumer) and
retire the loose synonyms ("backend", "wave (rendering) model", "stack")
- frame a WFE by three orthogonal design axes (inverse transform /
kinematics / statistics); state IWaveField is agnostic to all of them
- make the VRX-vs-EncinoWaves boundary explicit (§6.1)
- spell out POD (Plain Old Data)
- make engine-count phrasings count-agnostic; drop PR-split scaffolding
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Carlos Aguero <caguero@honurobotics.com>
|
I confirmed that kinematics is fully pluggable, e.g,. a Stokes (or any) can be used on the WFE side of the interface as a a self-contained provider with no core (VRX) changes, as long as it configures from the existing WaveParameters. This is great. |
| namespace gz::sim::systems | ||
| { | ||
| /// \brief Shared base for the per-engine wave systems | ||
| /// (`gz-sim-waves-fft-system`, `gz-sim-waves-gerstner-system`). It owns all |
There was a problem hiding this comment.
More semantics nits.
The tokens 'fft' and 'gerstner' are apples and oranges - both are components of the WFE, but are different elements. FFT is the inverse transform used and gerstner is the kinematics used. E.g., EncinoWaves uses FFT and gerstner while the legacy system uses summation and gerstner.
Suggest modification to...
Each engine
/// token names a *whole synthesis technique* (a fixed bundle of inverse
/// transform, kinematics, and statistics), not a single design axis:
/// `gerstner` is the analytic, direct-summation, deterministic engine and
/// `fft` is the FFT-synthesized, random-spectrum engine. The names are
/// conventional shorthand, not a transform-vs-kinematics contrast (the `fft`
/// engine also applies Gerstner-style displacement)
There was a problem hiding this comment.
Applied your suggested wording in a1fd739a: the token now reads as a whole synthesis technique (transform, kinematics, and statistics), not a single axis, with the note that fft also applies Gerstner displacement. Also dropped the retired "backend" for "engine".
bsb808
left a comment
There was a problem hiding this comment.
I read through the code and think it is good-to-go, realizing we can't really test it until we finish the batch of PRs.
Per bsb808's review: a token names a whole synthesis technique (a bundle of inverse transform, kinematics, and statistics), not a single design axis, so `fft` vs `gerstner` is not a transform-vs-kinematics contrast (the fft engine also applies Gerstner displacement). Also drop the retired "backend" term in favour of "engine". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Carlos Aguero <caguero@honurobotics.com>
|
Thanks for going through it! |
First of a series of patches that splits the wave-simulation work. This PR lands the engine-agnostic core that every later layer builds on.
Summary
IWaveField— the abstract wave-field interface (Elevation,Normal,ParticleVelocity,Jacobian,Update,SetParameters,Kind).CreateWaveSimulation,RegisterWaveEngineFactory); concrete engines register themselves from their own packages, so the core depends on none of them.WaveParameters— the shared input recipe.WavefieldECM component (+ serialization) — the channel on the world entity carrying the recipe and the liveshared_ptr<IWaveField>. Only the recipe is serialized; a replicated copy reconstructs its engine via thefactory.
Eval.hh/Eval.cc— the null-safe free-function query facade consumers use (falls back to still water when no engine is present). Bodies are out-of-line soIWaveFieldstays opaque to consumers; includes a newAdvance()helper so a consumer never has to deref the engine directly.WavesSystemBase— base class for the per-engine source plugins.The core ships only the contract — no consumers. Buoyancy and the water visual live in their own packages
gz_waves_buoyancy,gz_waves_rendering), each a downstream consumer of the wave field.