parabola ray demos#25
Open
tiye wants to merge 22 commits into
Open
Conversation
… symmetric sin accel
|
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:
WalkthroughAdds many new mirror/arc/parabola/gravity apps and WGSL shaders, extends UI tab routing/imports to expose them, tightens WGSL storage qualifiers to read-only across numerous shaders, updates paint bind-group entries, and bumps several devDependencies. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI / App Router
participant App as SolubleApp Module
participant GPU as GPU (buffers / bind groups)
participant Shader as WGSL Shader
participant Render as GPU Render Pass
UI->>App: instantiate selected mirror app
App->>GPU: createGlobalPointsBuffer / createSecondaryDataBuffer (upload)
App->>GPU: bind read-only storage buffers + uniforms
Render->>Shader: draw/frame -> vertex_main -> fragment_main
Shader-->>Render: fragment color (multi-bounce, arc/parabola/gravity)
Render-->>UI: present frame
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
Adds new “parabola ray” mirror demos for multiple polyhedra by introducing a shared WGSL shader that bends interior ray segments with a gravity-like acceleration, and wiring new apps into the app menu/registry.
Changes:
- Added a reusable parabola-capable mirror shader (
rhombic-dodecahedron-parabola-mirror.wgsl) that switches from linear to parabolic kinematics after the first bounce. - Added new polyhedra demo apps (tetrahedron/cube/octahedron/rhombic dodecahedron) that reuse the shared shader with different geometry + palettes.
- Registered the new demos in
compact.cirruand made a small numeric-formatting tweak incrystal-refraction.mts.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/apps/tetrahedron-parabola-mirror.mts | New tetrahedron parabola demo config + geometry/segments setup. |
| src/apps/cube-parabola-mirror.mts | New cube parabola demo config, including diagonal gravity direction. |
| src/apps/octahedron-parabola-mirror.mts | New octahedron parabola demo config + geometry/segments setup. |
| src/apps/rhombic-dodecahedron-parabola-mirror.mts | New rhombic dodecahedron parabola demo config + geometry/segments setup. |
| src/apps/rhombic-dodecahedron-parabola-mirror.wgsl | New shared shader implementing linear first leg + parabolic interior legs and parabolic mirror intersections. |
| src/apps/crystal-refraction.mts | Minor numeric literal formatting cleanup in params return. |
| compact.cirru | Adds new demos to the Polyhedra menu/tabs and app registry/imports. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+108
to
+109
| var best_distance = 1e30; | ||
| var best_i = 1u; |
There was a problem hiding this comment.
In curved_reach, the uniform sampling loop starts at i = 1u, so t = 0 is never evaluated. If the closest approach happens right at the leg start (e.g., immediately after a bounce), the minimum distance can be biased upward. Include a sample at t = 0 (or initialize the best distance from t = 0) to make the boundary case correct.
Suggested change
| var best_distance = 1e30; | |
| var best_i = 1u; | |
| let start_p = parabola_point(origin, velocity, 0.0, gravity); | |
| var best_distance = point_to_segment_distance(start_p, s); | |
| var best_i = 0u; |
Comment on lines
+254
to
+256
| let attenuation = pow(f32(in_mirror) / 2.0 + 2.0, 3.0); | ||
| let sample_limit = select(420.0, nearest.travel, hit_mirror); | ||
|
|
There was a problem hiding this comment.
sample_limit falls back to the hard-coded value 420.0 when no mirror is hit. This makes the visible ray length dependent on a magic number and can truncate the glow if geometry scale changes. Consider deriving this from scene scale (e.g., a bound based on vertex radius) or passing it as a parameter so it’s explicit and easier to tune per demo.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/apps/octahedron-parabola-mirror.mts`:
- Around line 23-33: The face winding for some octants is inward; update
makeCell (and/or the mirrors array initialization that calls makeCell) to ensure
each Cell's vertex order yields an outward-pointing normal: compute the intended
outward sign vector from the octant inputs (px/nx, py/ny, pz/nz), compute the
generated face normal, and if dot(generatedNormal, outwardSignVector) < 0 then
swap two vertices of that Cell to reverse winding; apply this fix where mirrors
is created (entries calling makeCell) so all 8 cells consistently have outward
winding without relying on the shader flip.
In `@src/apps/rhombic-dodecahedron-parabola-mirror.wgsl`:
- Around line 102-138: The loop sampling count in curved_reach is hardcoded as
96u — introduce a top-level constant (e.g. const CURVED_REACH_SAMPLES: u32 =
96u) and replace the literal 96u in curved_reach (the local variable steps, the
loop bounds, and the step calculation/uses) with that constant so future
perf/quality tuning is done by changing one named value; keep the algorithm
identical and ensure any casts still use the constant (e.g.,
f32(CURVED_REACH_SAMPLES)) where needed.
- Around line 37-41: The bindings base_points and secondary_points are declared
as var<storage, read_write> but are only read by fragment_main; change both
declarations to var<storage, read> to tighten storage access and avoid
validation warnings, and either remove the no-op compute_main or add a short
comment/todo inside compute_main indicating it is an intentional placeholder
(e.g., "// noop placeholder for future compute work") so its emptiness is
documented.
In `@src/apps/tetrahedron-parabola-mirror.mts`:
- Around line 48-51: CURVE_MAX in tetrahedron-parabola-mirror.mts is
inconsistent with its comment: either reduce CURVE_MAX to match the comment
(e.g., set CURVE_MAX ≈ 0.0004 to get the ~4 unit drop over half-edge) or keep
CURVE_MAX = 0.0256 and rewrite the comment to show the correct math and
consequence (0.5 * 0.0256 * 140² ≈ 251 units) and explain why that larger
amplitude is intentional; update the tetrahedron comment accordingly and, if
choosing to change the value, pick a CURVE_MAX consistent with other demos
(e.g., similar magnitude to octahedron 0.0045 or rhombic-dodecahedron 0.0128)
and run a quick visual/approximate check to confirm the resulting peak drop is
within the intended “clearly visible but not chaotic” range.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 35f5fd20-edab-45fc-b800-c4d69291e59f
📒 Files selected for processing (7)
compact.cirrusrc/apps/crystal-refraction.mtssrc/apps/cube-parabola-mirror.mtssrc/apps/octahedron-parabola-mirror.mtssrc/apps/rhombic-dodecahedron-parabola-mirror.mtssrc/apps/rhombic-dodecahedron-parabola-mirror.wgslsrc/apps/tetrahedron-parabola-mirror.mts
…REACH_SAMPLES constant, fix comment
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/apps/cubic-fire.wgsl (1)
33-46:⚠️ Potential issue | 🔴 CriticalCritical:
base_pointsdeclared read-only butcompute_mainwrites to it.Lines 39, 41, and 43 perform assignments to
base_points[index].positionfields, which are disallowed when the binding's access mode isread. WGSL validation will reject this module—the shader will fail at pipeline creation. Unlike the other shaders updated in this PR (rt-mirror,dodecahedron-mirror,tetrahedron-mirror), this shader's compute stage has non-empty write operations on the buffer, so it should not have been converted to read-only.Proposed fix
-@group(1) `@binding`(0) var<storage, read> base_points: array<BaseCell>; +@group(1) `@binding`(0) var<storage, read_write> base_points: array<BaseCell>;Alternatively, move the position update logic out of the GPU compute stage (CPU-side update or a separate write-back buffer).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/apps/cubic-fire.wgsl` around lines 33 - 46, The shader declares `@group`(1) `@binding`(0) var<storage, read> base_points but compute_main writes to base_points[index].position and related fields; change the storage access to var<storage, read_write> (or a writable storage class) for base_points or move the position updates out of compute_main into a CPU-side update or a separate writable buffer; update the declaration for base_points and any binding metadata to match writable usage and ensure compute_main remains consistent with the buffer access mode.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/apps/cuboctahedron-mirror.mts`:
- Around line 34-39: Replace the module-level mutable declaration "let store = {
... }" with a constant binding so its reference isn't reassigned; change the
declaration for the "store" object to use const, leaving the object properties
mutable as before, and apply the same change to the analogous "store" in
octahedron-parabola-mirror.mts for consistency.
In `@src/apps/orbit-spheres-mirror.wgsl`:
- Line 28: The base_points storage binding (var<storage, read> base_points) is
unused and likely dead code since sphere positions are produced by
make_spheres(); remove the unused binding declaration to reduce clutter, or if
the framework requires a placeholder binding, annotate it with a clear comment
and/or convert it to a minimal no-op type so it remains but signals intent;
locate the declaration named base_points and either delete that var<storage,
read> line or replace it with the required placeholder pattern for your runtime.
---
Outside diff comments:
In `@src/apps/cubic-fire.wgsl`:
- Around line 33-46: The shader declares `@group`(1) `@binding`(0) var<storage,
read> base_points but compute_main writes to base_points[index].position and
related fields; change the storage access to var<storage, read_write> (or a
writable storage class) for base_points or move the position updates out of
compute_main into a CPU-side update or a separate writable buffer; update the
declaration for base_points and any binding metadata to match writable usage and
ensure compute_main remains consistent with the buffer access mode.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9187493e-9165-469c-a306-3d4cdf4b5796
📒 Files selected for processing (38)
compact.cirrusrc/apps/box-mirror.wgslsrc/apps/circles.wgslsrc/apps/clocking.wgslsrc/apps/cubic-fire.wgslsrc/apps/cuboctahedron-mirror.mtssrc/apps/dodecahedron-mirror.wgslsrc/apps/dots-clock.wgslsrc/apps/glass-mirror.wgslsrc/apps/hex-prism-mirror.wgslsrc/apps/hollow-mirror.wgslsrc/apps/icosahedron-mirror.wgslsrc/apps/image.wgslsrc/apps/inversion-circles.wgslsrc/apps/kaleidoscope-mirror.wgslsrc/apps/kaleidoscope.wgslsrc/apps/octahedron-mirror.wgslsrc/apps/octahedron-parabola-mirror.mtssrc/apps/orbit-spheres-mirror.wgslsrc/apps/parallel-mirror.wgslsrc/apps/polyhedra-color-mirror.wgslsrc/apps/polyhedra-mirror.wgslsrc/apps/prism-mirror.wgslsrc/apps/pyramid-mirror.wgslsrc/apps/quaternion-fractal.wgslsrc/apps/rhombic-dodecahedron-parabola-mirror.wgslsrc/apps/rhombic-mirror.wgslsrc/apps/rings.wgslsrc/apps/ripple.wgslsrc/apps/rt-mirror.wgslsrc/apps/sphere-fractal.wgslsrc/apps/sphere-mirror.wgslsrc/apps/stars.wgslsrc/apps/surround-mirror.wgslsrc/apps/tetrahedron-mirror.wgslsrc/apps/tetrahedron-parabola-mirror.mtssrc/apps/wedge-mirror.wgslsrc/paint.mts
Comment on lines
+34
to
+39
| let store = { | ||
| startedAt: performance.now(), | ||
| maxReflections: 5, | ||
| angleY: 0, | ||
| lastTickAt: performance.now(), | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use const for module-level state holder.
store is a container whose reference never gets reassigned — prefer const. Same pattern exists in octahedron-parabola-mirror.mts; keep them consistent.
♻️ Proposed refactor
-let store = {
+const store = {
startedAt: performance.now(),
maxReflections: 5,
angleY: 0,
lastTickAt: performance.now(),
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let store = { | |
| startedAt: performance.now(), | |
| maxReflections: 5, | |
| angleY: 0, | |
| lastTickAt: performance.now(), | |
| }; | |
| const store = { | |
| startedAt: performance.now(), | |
| maxReflections: 5, | |
| angleY: 0, | |
| lastTickAt: performance.now(), | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/apps/cuboctahedron-mirror.mts` around lines 34 - 39, Replace the
module-level mutable declaration "let store = { ... }" with a constant binding
so its reference isn't reassigned; change the declaration for the "store" object
to use const, leaving the object properties mutable as before, and apply the
same change to the analogous "store" in octahedron-parabola-mirror.mts for
consistency.
|
|
||
| // base_points is present but unused here (we compute sphere positions analytically) | ||
| @group(1) @binding(0) var<storage, read_write> base_points: array<BaseCell>; | ||
| @group(1) @binding(0) var<storage, read> base_points: array<BaseCell>; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if base_points binding is referenced in JS/TS setup code
# Search for base_points references in binding setup
rg -n --type=ts --type=js -C3 'base_points|binding.*0.*group.*1|orbit.*spheres.*mirror'Repository: WebGPU-Art/soluble
Length of output: 44
🏁 Script executed:
# Check if the file exists and read the content around line 28
cat -n src/apps/orbit-spheres-mirror.wgsl | head -40Repository: WebGPU-Art/soluble
Length of output: 1294
🏁 Script executed:
# Search for any references to this shader file in the codebase
rg -l "orbit-spheres-mirror" --type=ts --type=js --type=wgslRepository: WebGPU-Art/soluble
Length of output: 91
🏁 Script executed:
# Search more broadly for BaseCell and base_points in all files
rg -n "base_points|BaseCell" --type=wgsl --type=ts --type=js -C2Repository: WebGPU-Art/soluble
Length of output: 91
🏁 Script executed:
# Search the entire WGSL file for any use of base_points
rg -n "base_points" src/apps/orbit-spheres-mirror.wgslRepository: WebGPU-Art/soluble
Length of output: 224
🏁 Script executed:
# Get full file to see if base_points is used anywhere
wc -l src/apps/orbit-spheres-mirror.wgsl && cat -n src/apps/orbit-spheres-mirror.wgslRepository: WebGPU-Art/soluble
Length of output: 5088
🏁 Script executed:
# Search for references to this shader in the codebase
rg -n "orbit-spheres-mirror"Repository: WebGPU-Art/soluble
Length of output: 3020
Buffer is already correctly scoped as read-only.
The base_points binding at line 28 is already declared with var<storage, read>, which correctly applies the principle of least privilege. Since the comment on line 27 confirms the buffer is unused and the shader computes sphere positions analytically via make_spheres() (line 96), the read-only access mode is appropriate.
Consider removing the unused buffer entirely.
The buffer declaration appears to be dead code. If the binding structure doesn't require this placeholder, removing it would reduce clutter and potential confusion. The compute shader (lines 31-32) is empty, suggesting this may be boilerplate from a template. Verify whether the framework requires the binding structure to be present.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/apps/orbit-spheres-mirror.wgsl` at line 28, The base_points storage
binding (var<storage, read> base_points) is unused and likely dead code since
sphere positions are produced by make_spheres(); remove the unused binding
declaration to reduce clutter, or if the framework requires a placeholder
binding, annotate it with a clear comment and/or convert it to a minimal no-op
type so it remains but signals intent; locate the declaration named base_points
and either delete that var<storage, read> line or replace it with the required
placeholder pattern for your runtime.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/apps/arc-mirror.wgsl`:
- Around line 117-141: The fixed EPS must be replaced with an arc-length–based
threshold: compute an effective_radius = max(sqrt(R_sq), 1.0) (and clamp it to a
reasonable max if you want to guard against make_arc_ray degenerate values),
pick a small arc-length epsilon (e.g. EPS_LEN = 3.0 units), then compute
EPS_angle = EPS_LEN / effective_radius (and optionally clamp EPS_angle to a sane
[MIN, MAX] range) and use EPS_angle in the candidate-angle filter instead of the
constant EPS; update references in this block (symbols: EPS, R_sq, sqrt, best,
phi, delta, arc_point, MAX_ARC, make_arc_ray, arc_radius, arc_phase/abs_phase)
so the self-intersection guard works in Euclidean units rather than raw radians.
In `@src/apps/tetrahedron-arc-mirror.mts`:
- Around line 28-46: The Cell type and factory makeCell use misleading field
names (position/velocity/arm) for triangle vertices; rename the Cell shape to {
a: Number4; b: Number4; c: Number4 } (or import the shader's BaseCell type if
available), update makeCell to return { a,b,c }, and update all local
references/variables (makeCell calls, baseMirrors, baseSegments) to use the new
property names so the data maps directly to the shader's BaseCell a/b/c fields.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cc1f87fa-9bca-4066-85ed-cf1f1750dd34
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
compact.cirrupackage.jsonsrc/apps/arc-mirror.wgslsrc/apps/tetrahedron-arc-mirror.mts
Comment on lines
+117
to
+141
| let ratio = C / sqrt(R_sq); | ||
| if abs(ratio) > 1.0 { return arc_hit_none(); } | ||
|
|
||
| let phi = atan2(B, A); | ||
| let delta = acos(clamp(ratio, -1.0, 1.0)); | ||
| let TWO_PI = 6.28318530718; | ||
| let EPS = 0.005; | ||
|
|
||
| var best = 1e30; | ||
|
|
||
| // Two candidate angles; normalise into (0, 2π) then filter by (EPS, MAX_ARC) | ||
| for (var k = 0u; k < 2u; k++) { | ||
| var th = select(phi + delta, phi - delta, k == 1u); | ||
| th = th - TWO_PI * floor(th / TWO_PI); | ||
| if th > EPS && th < MAX_ARC && th < best { | ||
| // Point-in-triangle test (sign-agnostic in n orientation) | ||
| let pt = arc_point(arc, th); | ||
| let d0 = dot(cross(mirror.b - mirror.a, pt - mirror.a), n); | ||
| let d1 = dot(cross(mirror.c - mirror.b, pt - mirror.b), n); | ||
| let d2 = dot(cross(mirror.a - mirror.c, pt - mirror.c), n); | ||
| let inside = (d0 >= 0.0 && d1 >= 0.0 && d2 >= 0.0) | ||
| || (d0 <= 0.0 && d1 <= 0.0 && d2 <= 0.0); | ||
| if inside { best = th; } | ||
| } | ||
| } |
There was a problem hiding this comment.
EPS threshold in θ-radians becomes too aggressive at large effective radii.
EPS = 0.005 is in arc-angle units, which works for the nominal arc_radius = 600 (≈3 units of arc-length), but degenerates badly when the effective radius grows:
- When
arc_phaseis near a zero crossing,abs_phaseis clamped to0.004(line 171), givingeffective_radius ≈ 150_000. ThenEPS = 0.005 rad ≈ 750units of arc-length — larger than the entire scene, so every mirror hit is filtered out. - In
make_arc_ray's degenerate fallback (line 86) the radius jumps to1e6, making the situation strictly worse.
Net effect: every ~20 s (half period of sin(lifetime * 0.0196) at the current lifetime rate) the secondary bounces briefly find no mirrors, so the loop terminates after the first straight bounce — a visible flicker / dimming pulse. Consider expressing the self-intersection guard in arc-length (or Euclidean) units instead.
🔧 Suggested fix
- let phi = atan2(B, A);
- let delta = acos(clamp(ratio, -1.0, 1.0));
- let TWO_PI = 6.28318530718;
- let EPS = 0.005;
+ let phi = atan2(B, A);
+ let delta = acos(clamp(ratio, -1.0, 1.0));
+ let TWO_PI = 6.28318530718;
+ // Self-intersection guard expressed as arc-length, so it stays meaningful
+ // when arc.radius blows up near sin() zero-crossings.
+ let MIN_TRAVEL = 0.05;
+ let EPS = MIN_TRAVEL / max(arc.radius, 1.0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/apps/arc-mirror.wgsl` around lines 117 - 141, The fixed EPS must be
replaced with an arc-length–based threshold: compute an effective_radius =
max(sqrt(R_sq), 1.0) (and clamp it to a reasonable max if you want to guard
against make_arc_ray degenerate values), pick a small arc-length epsilon (e.g.
EPS_LEN = 3.0 units), then compute EPS_angle = EPS_LEN / effective_radius (and
optionally clamp EPS_angle to a sane [MIN, MAX] range) and use EPS_angle in the
candidate-angle filter instead of the constant EPS; update references in this
block (symbols: EPS, R_sq, sqrt, best, phi, delta, arc_point, MAX_ARC,
make_arc_ray, arc_radius, arc_phase/abs_phase) so the self-intersection guard
works in Euclidean units rather than raw radians.
Comment on lines
+28
to
+46
| type Cell = { position: Number4; velocity: Number4; arm: Number4 }; | ||
| const makeCell = (a: Number4, b: Number4, c: Number4): Cell => ({ position: a, velocity: b, arm: c }); | ||
|
|
||
| const u = 100; | ||
| const baseMirrors: Cell[] = [ | ||
| makeCell([u, u, u, 0], [u, -u, -u, 0], [-u, u, -u, 0]), | ||
| makeCell([u, u, u, 0], [-u, -u, u, 0], [u, -u, -u, 0]), | ||
| makeCell([u, u, u, 0], [-u, u, -u, 0], [-u, -u, u, 0]), | ||
| makeCell([-u, u, -u, 0], [u, -u, -u, 0], [-u, -u, u, 0]), | ||
| ]; | ||
|
|
||
| const baseSegments: Cell[] = (() => { | ||
| const zero: Number4 = [0, 0, 0, 0]; | ||
| const a: Number4 = [u, u, u, 0]; | ||
| const b: Number4 = [u, -u, -u, 0]; | ||
| const c: Number4 = [-u, u, -u, 0]; | ||
| const d: Number4 = [-u, -u, u, 0]; | ||
| return [makeCell(a, b, zero), makeCell(a, c, zero), makeCell(a, d, zero), makeCell(b, c, zero), makeCell(b, d, zero), makeCell(c, d, zero)]; | ||
| })(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Misleading field names for triangle geometry.
Cell stores triangle vertices but labels them position / velocity / arm. The shader’s BaseCell { a, b, c } and the rest of this file refer to them as a/b/c. Renaming the local type to { a, b, c } (or reusing an existing shared cell type) would make intent obvious and avoid the cognitive load of mapping velocity → "second vertex" while reading makeCell(a, b, c).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/apps/tetrahedron-arc-mirror.mts` around lines 28 - 46, The Cell type and
factory makeCell use misleading field names (position/velocity/arm) for triangle
vertices; rename the Cell shape to { a: Number4; b: Number4; c: Number4 } (or
import the shader's BaseCell type if available), update makeCell to return {
a,b,c }, and update all local references/variables (makeCell calls, baseMirrors,
baseSegments) to use the new property names so the data maps directly to the
shader's BaseCell a/b/c fields.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@compact.cirru`:
- Line 111: The tabs definition (def tabs) is missing the :gravity-spheres entry
that exists in tab-groups, causing inconsistent tab behavior; update the def
tabs $ [] declaration to include the :gravity-spheres symbol with the same
label/style used in tab-groups so both lists stay synchronized (search for "def
tabs" and "tab-groups" to find the two definitions and add the :gravity-spheres
tuple into the tabs list).
In `@src/apps/gravity-spheres.wgsl`:
- Around line 88-95: The gravity_at function (and the other block that
normalizes/divides by diff at lines around 161-165) can produce zero-length
vectors when p equals a sphere center, causing NaNs; fix by guarding
normalization/division: compute a safe squared length (e.g., r2 =
dot(diff,diff)) and if r2 is below a small epsilon use vec3f(0.0) (or skip
adding acceleration) instead of calling normalize(diff) or dividing by r2, or
replace r2 with max(r2, epsilon) and conditionally handle normalize(diff) when
diff is nearly zero; apply the same guard to the other normalization/division
sites so diff==0 never yields NaNs.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5e66620b-a9da-4245-8247-5394461ecc86
📒 Files selected for processing (3)
compact.cirrusrc/apps/gravity-spheres.mtssrc/apps/gravity-spheres.wgsl
Comment on lines
+88
to
+95
| fn gravity_at(p: vec3f, spheres: array<Sphere, 4>) -> vec3f { | ||
| var a = vec3f(0.); | ||
| for (var i = 0u; i < 4u; i++) { | ||
| let diff = spheres[i].center - p; | ||
| // Clamp denominator so the ray never diverges when inside a sphere. | ||
| let r2 = max(dot(diff, diff), spheres[i].radius * spheres[i].radius * 0.16); | ||
| a += normalize(diff) * spheres[i].gm / r2; | ||
| } |
There was a problem hiding this comment.
Guard zero-length vectors before normalization/division.
Line 94 and Lines 161-165 can hit zero-length vectors (diff == 0), producing NaNs in vel/pos/color and unstable pixels.
Suggested patch
fn gravity_at(p: vec3f, spheres: array<Sphere, 4>) -> vec3f {
var a = vec3f(0.);
for (var i = 0u; i < 4u; i++) {
let diff = spheres[i].center - p;
- // Clamp denominator so the ray never diverges when inside a sphere.
- let r2 = max(dot(diff, diff), spheres[i].radius * spheres[i].radius * 0.16);
- a += normalize(diff) * spheres[i].gm / r2;
+ let raw_r2 = dot(diff, diff);
+ // Clamp denominator so the ray never diverges when inside a sphere.
+ let r2 = max(raw_r2, spheres[i].radius * spheres[i].radius * 0.16);
+ let inv_r = inverseSqrt(max(raw_r2, 1e-6));
+ a += diff * inv_r * spheres[i].gm / r2;
}
return a;
}
@@
let dist = sqrt(dist2);
if dist < sp.radius {
// Ray hit a sphere — mirror reflect and continue.
- let n = diff / dist;
+ let safe_dist = max(dist, 1e-4);
+ let n = diff / safe_dist;
vel = reflect(vel, n);
pos = sp.center + n * (sp.radius + 1.2);
color += vec4f(sp.color * 0.20 * fade, 0.0);
} else {Also applies to: 160-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/apps/gravity-spheres.wgsl` around lines 88 - 95, The gravity_at function
(and the other block that normalizes/divides by diff at lines around 161-165)
can produce zero-length vectors when p equals a sphere center, causing NaNs; fix
by guarding normalization/division: compute a safe squared length (e.g., r2 =
dot(diff,diff)) and if r2 is below a small epsilon use vec3f(0.0) (or skip
adding acceleration) instead of calling normalize(diff) or dividing by r2, or
replace r2 with max(r2, epsilon) and conditionally handle normalize(diff) when
diff is nearly zero; apply the same guard to the other normalization/division
sites so diff==0 never yields NaNs.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
compact.cirru (1)
111-111:⚠️ Potential issue | 🟡 Minor
tabsis still missing the new gravity demos.
tab-groupsnow exposes:gravity-spheres,:gravity-cubes, and:gravity-octahedronat Lines 106-107, buttabson Line 111 still omits all three. That leaves the two navigation sources out of sync again, so any flow still readingtabswill miss these demos. Please add the three entries in the source Cirru and regenerate this snapshot.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@compact.cirru` at line 111, The tabs list defined by def tabs is missing the three new gravity demos (:gravity-spheres, :gravity-cubes, :gravity-octahedron) that were added to tab-groups; update the def tabs vector to include entries for (:: :gravity-spheres "|Gravity Spheres" :dark), (:: :gravity-cubes "|Gravity Cubes" :dark), and (:: :gravity-octahedron "|Gravity Octahedron" :dark) so both navigation sources match, then regenerate the Cirru snapshot.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/apps/gravity-cubes.mts`:
- Around line 6-7: The header comment in src/apps/gravity-cubes.mts incorrectly
states "4 rotating cubes" while the shader builds array<Cube, 3> in
src/apps/gravity-cubes.wgsl; update the module comment in gravity-cubes.mts to
say "3 rotating cubes (OBBs) on elliptical orbits" so it matches the shader's
array<Cube, 3> setup and the documented 3-cube scene.
In `@src/apps/gravity-cubes.wgsl`:
- Around line 92-96: The gravity_at function can call normalize(diff) when diff
is zero (p equals cm), producing NaNs; modify gravity_at to guard the
normalization by computing the vector length (length(diff)) or using an epsilon
(e.g., 1e-6) and only normalize when length > epsilon, otherwise return a zero
vec3f (zero force) or use diff * (1.0 / max(length, epsilon)) to produce a safe
direction; reference gravity_at, cm, diff and r2 in your change.
---
Duplicate comments:
In `@compact.cirru`:
- Line 111: The tabs list defined by def tabs is missing the three new gravity
demos (:gravity-spheres, :gravity-cubes, :gravity-octahedron) that were added to
tab-groups; update the def tabs vector to include entries for (::
:gravity-spheres "|Gravity Spheres" :dark), (:: :gravity-cubes "|Gravity Cubes"
:dark), and (:: :gravity-octahedron "|Gravity Octahedron" :dark) so both
navigation sources match, then regenerate the Cirru snapshot.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 06383691-4788-4a98-9a7c-67ec2ceaf431
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
compact.cirrudeps.cirrupackage.jsonsrc/apps/gravity-cubes.mtssrc/apps/gravity-cubes.wgslsrc/apps/gravity-octahedron.mtssrc/apps/gravity-octahedron.wgsl
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Refactor
Chores