[encoding] Inflate estimates for offset curves #522
Merged
Conversation
This file contains 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
raphlinus
approved these changes
Mar 15, 2024
crates/encoding/src/estimate.rs
Outdated
| const MIN_THETA: f32 = 1e-4; | ||
| const TOL: f32 = 0.1; | ||
| const MIN_THETA: f32 = 1e-6; | ||
| const TOL: f32 = 0.25; |
There was a problem hiding this comment.
Yep. We may want to make this a tunable parameter, perhaps at runtime, though I think the constant value is good enough for now.
There was a problem hiding this comment.
A config uniform for tolerances might be a good idea. It may also make sense to have a common set of constant declarations that are shared by cpu_shaders and the estimate code (though they live in different crates).
crates/encoding/src/estimate.rs
Outdated
| @@ -288,22 +293,22 @@ fn transform_scale(t: Option<&Transform>) -> f32 { | |||
| } | |||
| } | |||
|
|
|||
| fn approx_arc_length_cubic(p0: Vec2, p1: Vec2, p2: Vec2, p3: Vec2) -> f64 { | |||
| fn approx_arc_length_cubic(p0: Vec2, p1: Vec2, p2: Vec2, p3: Vec2) -> f32 { | |||
There was a problem hiding this comment.
My gut feeling would be to keep these f64 to emphasize that they're CPU-side computations and to have as much accuracy as possible, but I don't think it makes a difference in practice. The later ones where we defer quantization are a good idea for sure though.
- Added a highly crude scale to segment and line counts in the presence of a non-zero offset (i.e. stroked curves). This is done to account for the inflated flattening of an offset curve around high-curvature zones. I believe this could get refined but that requires some work to find the best way to analyze the complexity of the curve with few CPU cycles. - Fixed a bug in join calculation that was wrongfully incrementing the line count instead of segments. This makes the estimate robust for tricky strokes but also increases the margin of the estimate over the true bump counts. I expect some of this will improve when the estimation moves to resolve time, such that the crude bloating of the counts due to any scale applied to a scene fragment will no longer be necessary.
armansito
force-pushed
the
bump-estimate-offset
branch
from
79382b4 to
ea47eae
Compare
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.
Added a highly crude scale to segment and line counts in the presence of a non-zero offset (i.e. stroked curves). This is done to account for the inflated flattening of an offset curve around high-curvature zones. I believe this could get refined but that requires some work to find the best way to analyze the complexity of the curve with few CPU cycles.
Fixed a bug in join calculation that was wrongfully incrementing the line count instead of segments.
This makes the estimate robust for tricky strokes but also increases the margin of the estimate over the true bump counts. I expect some of this will improve when the estimation moves to resolve time, such that the crude bloating of the counts due to any scale applied to a scene fragment will no longer be necessary.