Improve tolerance calculation for stroke expansion #1180
Conversation
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
tomcur
commented
Aug 27, 2025
There was a problem hiding this comment.
I believe we can calculate a simpler conservative bound, perhaps something like the Frobenius norm, sqrt(a^2 + b^2 + c^2 + d^2) ignoring the translation part of the transform; or dropping the sqrt to get the simpler but more conservative expression |a| + |b| + |c| + |d|.
It would be nice to have a quicker-to-calculate bound than the ellipse (which does a singular value decomposition), though it has to be balanced with the additional downstream work a conservative bound causes.
tomcur
added a commit
to tomcur/kurbo
that referenced
this pull request
Sep 3, 2025
We're especially interested in marking `Ellipse::radii` inline, motivated by linebender/vello#1180.
This was referenced Sep 3, 2025
github-merge-queue bot
pushed a commit
to linebender/kurbo
that referenced
this pull request
Sep 4, 2025
We're especially interested in marking `Ellipse::radii` inline, motivated by linebender/vello#1180.
github-merge-queue bot
pushed a commit
to linebender/kurbo
that referenced
this pull request
Sep 4, 2025
This adds methods to `Ellipse` to get the major and minor radii. This documents the (private) `Affine::svd` method to guarantee the part of its behavior we're using here for efficiency. The `Ellipse::radii()` method effectively already allows for this, by taking the `x` component for the major radius and the `y` component for the minor radius; however, we do not currently guarantee that behavior, and may not want to, to keep the option of changing internal representations. Plus, within crate boundaries and the specified inlining, the compiler should be able to eliminate the dead singular value decomposition code. Quickly looking over the assembly, there are just the two `sqrtsd` instructions as expected. Like #496, this is motivated by linebender/vello#1180, to allow eliding more computations. <details> <summary>x86 assembly</summary> ```assembly // kurbo/src/ellipse.rs:132 pub fn major_radius(&self) -> f64 { .cfi_startproc sub rsp, 104 .cfi_def_cfa_offset 112 // kurbo/src/ellipse.rs:133 self.inner.svd().0.x movupd xmm0, xmmword ptr [rdi] movupd xmm4, xmmword ptr [rdi + 16] // kurbo/src/affine.rs:401 let a2 = a * a; movapd xmm3, xmm0 mulpd xmm3, xmm0 // kurbo/src/affine.rs:405 let ab = a * b; movapd xmm2, xmm0 unpcklpd xmm2, xmm4 unpckhpd xmm0, xmm4 // kurbo/src/affine.rs:403 let c2 = c * c; mulpd xmm4, xmm4 // kurbo/src/affine.rs:405 let ab = a * b; mulpd xmm0, xmm2 // kurbo/src/affine.rs:407 let angle = 0.5 * (2.0 * (ab + cd)).atan2(a2 - b2 + c2 - d2); movapd xmm1, xmm0 unpckhpd xmm1, xmm0 addsd xmm1, xmm0 movapd xmmword ptr [rsp + 80], xmm1 movapd xmm0, xmm1 addsd xmm0, xmm1 movapd xmmword ptr [rsp + 48], xmm3 // kurbo/src/affine.rs:408 let s1 = a2 + b2 + c2 + d2; movapd xmm2, xmm3 unpckhpd xmm2, xmm3 movapd xmmword ptr [rsp + 32], xmm2 // kurbo/src/affine.rs:407 let angle = 0.5 * (2.0 * (ab + cd)).atan2(a2 - b2 + c2 - d2); movapd xmm1, xmm3 subsd xmm1, xmm2 movapd xmmword ptr [rsp + 16], xmm4 addsd xmm1, xmm4 // kurbo/src/affine.rs:408 let s1 = a2 + b2 + c2 + d2; unpckhpd xmm4, xmm4 movapd xmmword ptr [rsp], xmm4 // kurbo/src/affine.rs:407 let angle = 0.5 * (2.0 * (ab + cd)).atan2(a2 - b2 + c2 - d2); subsd xmm1, xmm4 movapd xmmword ptr [rsp + 64], xmm1 call qword ptr [rip + atan2@GOTPCREL] movapd xmm0, xmmword ptr [rsp + 32] // kurbo/src/affine.rs:408 let s1 = a2 + b2 + c2 + d2; addsd xmm0, qword ptr [rsp + 48] addsd xmm0, qword ptr [rsp + 16] addsd xmm0, qword ptr [rsp] movapd xmm1, xmm0 movapd xmm0, xmmword ptr [rsp + 80] mulsd xmm0, xmm0 // kurbo/src/affine.rs:409 let s2 = ((a2 - b2 + c2 - d2).powi(2) + 4.0 * (ab + cd).powi(2)).sqrt(); mulsd xmm0, qword ptr [rip + .LCPI116_0] movapd xmm2, xmmword ptr [rsp + 64] mulsd xmm2, xmm2 // kurbo/src/affine.rs:409 let s2 = ((a2 - b2 + c2 - d2).powi(2) + 4.0 * (ab + cd).powi(2)).sqrt(); addsd xmm0, xmm2 sqrtsd xmm0, xmm0 // kurbo/src/affine.rs:412 x: (0.5 * (s1 + s2)).sqrt(), addsd xmm0, xmm1 mulsd xmm0, qword ptr [rip + .LCPI116_1] sqrtsd xmm0, xmm0 // kurbo/src/ellipse.rs:134 } add rsp, 104 .cfi_def_cfa_offset 8 ret ``` </details>
flaviotore
added a commit
to flaviotore/kurbo
that referenced
this pull request
Oct 9, 2025
We're especially interested in marking `Ellipse::radii` inline, motivated by linebender/vello#1180.
flaviotore
added a commit
to flaviotore/kurbo
that referenced
this pull request
Oct 9, 2025
This adds methods to `Ellipse` to get the major and minor radii. This documents the (private) `Affine::svd` method to guarantee the part of its behavior we're using here for efficiency. The `Ellipse::radii()` method effectively already allows for this, by taking the `x` component for the major radius and the `y` component for the minor radius; however, we do not currently guarantee that behavior, and may not want to, to keep the option of changing internal representations. Plus, within crate boundaries and the specified inlining, the compiler should be able to eliminate the dead singular value decomposition code. Quickly looking over the assembly, there are just the two `sqrtsd` instructions as expected. Like linebender/kurbo#496, this is motivated by linebender/vello#1180, to allow eliding more computations. <details> <summary>x86 assembly</summary> ```assembly // kurbo/src/ellipse.rs:132 pub fn major_radius(&self) -> f64 { .cfi_startproc sub rsp, 104 .cfi_def_cfa_offset 112 // kurbo/src/ellipse.rs:133 self.inner.svd().0.x movupd xmm0, xmmword ptr [rdi] movupd xmm4, xmmword ptr [rdi + 16] // kurbo/src/affine.rs:401 let a2 = a * a; movapd xmm3, xmm0 mulpd xmm3, xmm0 // kurbo/src/affine.rs:405 let ab = a * b; movapd xmm2, xmm0 unpcklpd xmm2, xmm4 unpckhpd xmm0, xmm4 // kurbo/src/affine.rs:403 let c2 = c * c; mulpd xmm4, xmm4 // kurbo/src/affine.rs:405 let ab = a * b; mulpd xmm0, xmm2 // kurbo/src/affine.rs:407 let angle = 0.5 * (2.0 * (ab + cd)).atan2(a2 - b2 + c2 - d2); movapd xmm1, xmm0 unpckhpd xmm1, xmm0 addsd xmm1, xmm0 movapd xmmword ptr [rsp + 80], xmm1 movapd xmm0, xmm1 addsd xmm0, xmm1 movapd xmmword ptr [rsp + 48], xmm3 // kurbo/src/affine.rs:408 let s1 = a2 + b2 + c2 + d2; movapd xmm2, xmm3 unpckhpd xmm2, xmm3 movapd xmmword ptr [rsp + 32], xmm2 // kurbo/src/affine.rs:407 let angle = 0.5 * (2.0 * (ab + cd)).atan2(a2 - b2 + c2 - d2); movapd xmm1, xmm3 subsd xmm1, xmm2 movapd xmmword ptr [rsp + 16], xmm4 addsd xmm1, xmm4 // kurbo/src/affine.rs:408 let s1 = a2 + b2 + c2 + d2; unpckhpd xmm4, xmm4 movapd xmmword ptr [rsp], xmm4 // kurbo/src/affine.rs:407 let angle = 0.5 * (2.0 * (ab + cd)).atan2(a2 - b2 + c2 - d2); subsd xmm1, xmm4 movapd xmmword ptr [rsp + 64], xmm1 call qword ptr [rip + atan2@GOTPCREL] movapd xmm0, xmmword ptr [rsp + 32] // kurbo/src/affine.rs:408 let s1 = a2 + b2 + c2 + d2; addsd xmm0, qword ptr [rsp + 48] addsd xmm0, qword ptr [rsp + 16] addsd xmm0, qword ptr [rsp] movapd xmm1, xmm0 movapd xmm0, xmmword ptr [rsp + 80] mulsd xmm0, xmm0 // kurbo/src/affine.rs:409 let s2 = ((a2 - b2 + c2 - d2).powi(2) + 4.0 * (ab + cd).powi(2)).sqrt(); mulsd xmm0, qword ptr [rip + .LCPI116_0] movapd xmm2, xmmword ptr [rsp + 64] mulsd xmm2, xmm2 // kurbo/src/affine.rs:409 let s2 = ((a2 - b2 + c2 - d2).powi(2) + 4.0 * (ab + cd).powi(2)).sqrt(); addsd xmm0, xmm2 sqrtsd xmm0, xmm0 // kurbo/src/affine.rs:412 x: (0.5 * (s1 + s2)).sqrt(), addsd xmm0, xmm1 mulsd xmm0, qword ptr [rip + .LCPI116_1] sqrtsd xmm0, xmm0 // kurbo/src/ellipse.rs:134 } add rsp, 104 .cfi_def_cfa_offset 8 ret ``` </details>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Motivated by the discussion in and around #977. Opened as a new PR as not to block #977.
We'd want to mark the
Ellipse::radiimethod as#[inline(always)]in Kurbo.