-
Notifications
You must be signed in to change notification settings - Fork 204
Fix non-deterministic GPU stroke artifacts (vello#1314) #1323
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
The miter-limit predicate used (hypot + d) but it must use (hypot - d).\n\nWith the wrong sign, near-collinear joins could incorrectly pass the\nmiter-limit check, producing extremely large miter points (division by a\nvery small cross product). This led to unstable GPU output in vello#1314.
8a4fe4e to
3e0a40d
Compare
|
To improve my own abilities, I am curious how you figure this sort of thing out! :) |
|
I've tested it on my end and it does seem to fix linebender/xilem#1128. So thanks a lot for that! |
It seems like there's an underlying problem behind the underlying problem yet to be fixed; the fact that extremely large geometry manifest as non-deterministic corruption seems like it could easily hide similar errors. Regardless, I guess we're not likely to fix that before sparse strips lands.
Speaking for myself, I'm especially curious if this is something that could have been caught with fuzzing. Probably not since this is GPU rendering anyway. |
|
vello/vello_shaders/src/cpu/flatten.rs Line 460 in 4ea47b4
It would be useful to keep the rust flatten shader up to date as well |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm quite sure you've found the source of the non-determinism.
At first I thought the change you suggested was correct, but after testing similar code in Kurbo I believe this now finds the wrong cut-off for the miter limit. I believe the original hypot + d actually is the right calculation (it is related to the miter length r / |cos(theta/2)| with r the stroke half-width and theta the angle described by the vectors prev_tan and next_tan arranged tail-to-tail).
I believe the issue is instead caused when the cross product (in cr) is very nearly zero. We check for exact zero, but that's going to be flakey.
I believe the following would be correct. Perhaps the check introduced on cr can be better tuned still. What I suggest here is right dimensionally, but Kurbo does something like 0.01 * hypot * half_width < cr.abs(). Perhaps @raphlinus has thoughts here, as we can be a bit more efficient by foregoing drawing the miter if it's within rendering tolerance.
// Given the two tangent vectors `tan_prev` and `tan_next` arranged
// tail-to-tail, the miter length (distance from the vertex join to
// the miter tip) is `1 / |cos(theta/2)| * half_width` with `theta`
// the angle described by the tangents and `half_width` the stroke
// half-width.
//
// For near-collinear joins, the ideal miter point goes to infinity; the
// miter limit must clamp those cases to avoid extreme geometry.
//
// The following calculates this limit efficiently. `hypot` is
// `|tan_prev| * |tan_next|` (since `cr^2 + d^2 = |a|^2 |b|^2`).
// `hypot + d` is `2 * |tan_prev| * |tan_next| * cos^2(theta/2)`,
// After rearranging, the following tests whether
// `1/|cos(theta/2)| < miter_limit` (i.e., the miter length is
// limited to a ratio of the stroke half-width: `miter_limit *
// half_width`).
if 2. * hypot < (hypot + d) * miter_limit * miter_limit
&& abs(cr) > TANGENT_THRESH * TANGENT_THRESH
{|
First, excellent analysis, I'm impressed. I'm currently in a Quaker business meeting, but hope to be able to take a closer look soon. Based on Tom's comment, it looks like this requires deeper consideration. |
Good call — I’ve now mirrored the same join logic change in |
Thanks for testing this against xilem#1128 — appreciated. |
Happy to share! What helped most was turning it into a deterministic “same inputs, many frames” |
Thanks! Based on Tom’s note I revisited the math and updated the PR to keep the |
Summary
Fix non-deterministic rendering artifacts when GPU-stroking certain rounded rectangles (vello#1314).
The issue could manifest as severe frame-to-frame corruption even when rendering
the exact same scene. This PR keeps the change minimal (stroker join logic in
WGSL plus the CPU mirror) and includes a standalone repro program in the PR
description for reviewers to copy/paste and run locally.
Reproduction (standalone, not committed)
This PR intentionally does not add a new
examples/crate. Instead, below is a standalonerepro_1314program. Reviewers can copy it into a scratch Cargo project and run it against a local checkout of this branch.How to run
From your Vello workspace root (the directory that contains
vello/):/tmp).Cargo.tomlandsrc/main.rs.On affected revisions this tends to fail very quickly (often within the first couple frames). With this patch it should remain stable and print
ok: 250 frames matched./tmp/repro_1314/Cargo.toml/tmp/repro_1314/src/main.rsOptions
--iters Nnumber of frames to compare (default: 250)--width Nrender target width (default: 512)--height Nrender target height (default: 512)--shapes Nnumber of copies of the triggering geometry (default: 96)Root cause
In
vello_shaders/shader/flatten.wgsl, the GPU stroker's miter-join path uses a miter-limit predicate to decide whether to emit a miter or fall back to bevel.For tangents
a = tan_prevandb = tan_next:d = dot(a, b)cr = cross(a, b)hypot = length(vec2(cr, d)) = |a| * |b|(sincecr^2 + d^2 = |a|^2 |b|^2)Given the two tangents arranged tail-to-tail, the miter length ratio is
1 / |cos(θ/2)|, where θ is the angle between tangents. After rearranging, themiter-limit predicate can be expressed in terms of
hypot + d.The bug is that we only guarded on
cr != 0. Whencris extremely small butnon-zero (near-collinear tangents), the miter-point intersection math divides by
cr, which can produce extremely large geometry. That pathological geometry candestabilize later pipeline stages and show up as frame-to-frame corruption even
when the scene is unchanged.
Fix
Keep the miter-limit predicate as-is, but treat sufficiently small
craseffectively zero (using
TANGENT_THRESH^2) and skip the miter computation inthat case (fall back to bevel), avoiding pathological miter points.
On my machine this stabilizes the repro for 1000 iterations (
ok: 1000 frames matched).Files changed
vello_shaders/shader/flatten.wgsl: robustify miter join near-collinearityvello_shaders/src/cpu/flatten.rs: keep CPU shader mirror in syncRelated