Fix artifacts when sampling AMR volume#35
Conversation
There was a problem hiding this comment.
Pull request overview
Updates AMR sampling to avoid boundary-related artifacts (notably at relBrickPos == 1.0) by clamping brick-relative coordinates and simplifying shared dual-cell interpolation helpers across CPU/GPU paths.
Changes:
- Clamp
relBrickPosto< 1.0(1 ULP below) before computing brick-cell indices (CPU ISPC and GPU compute). - Refactor
initDualCelloverloads andlerp()helpers to reduce duplication and align CPU/GPU implementations. - Remove the
nextafter()-shrunkf_dimsworkaround and rely on explicit coordinate clamping instead.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| openvkl/devices/gpu/compute/amr/DualCell.h | Refactors initDualCell and lerp helpers to reduce duplication. |
| openvkl/devices/gpu/compute/amr/CellRef.h | Adds explicit clamp of brick-relative coordinates to avoid out-of-range indexing at the upper boundary. |
| openvkl/devices/cpu/volume/amr/DualCell.ih | Mirrors DualCell refactors (overload forwarding + lerp helper reuse). |
| openvkl/devices/cpu/volume/amr/CellRef.ispc | Adds the same boundary clamp in CPU ISPC code paths. |
| openvkl/devices/cpu/volume/amr/AMRData.cpp | Removes nextafter() shrink for f_dims and uses exact dims as floats. |
| CHANGELOG.md | Documents the AMR sampling artifact fix for 2.0.3. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const vec3f relBrickPos = | ||
| min(relBrickPosUnclamped, vec3f(0x1.fffffep-1f)); |
There was a problem hiding this comment.
The clamp uses a hard-to-read hex float literal (0x1.fffffep-1f) to represent the largest float < 1.0. Consider defining this as a named constant (or computing it via nextafter(1.f, 0.f)) to make the intent clearer and avoid duplicating the literal elsewhere.
No description provided.