-
Notifications
You must be signed in to change notification settings - Fork 141
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
New "style" encoding to replace "linewidth" #389
Conversation
84c0ef7
to
d2715d2
Compare
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.
A few tiny style nits but otherwise lgtm, thanks!
crates/encoding/src/path.rs
Outdated
/// ``` | ||
/// | ||
/// - miter_limit: u16 - The miter limit for a stroke, encoded in binary16 (half) floating | ||
/// point representation. This field is on meaningful for the `Join::Miter` |
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.
on -> only
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.
Done.
crates/encoding/src/path.rs
Outdated
pub const FLAGS_END_CAP_MASK: u32 = 0x0300_0000; | ||
pub const MITER_LIMIT_MASK: u32 = 0xFFFF; | ||
|
||
pub fn from_fill(fill: &Fill) -> Self { |
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 think it’d be slightly cleaner to take this parameter by value.
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.
Done.
crates/encoding/src/path.rs
Outdated
} | ||
|
||
#[cfg(test)] | ||
fn get_fill(&self) -> Option<Fill> { |
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.
Small style nit: I know these are only for tests now but it seems likely they’ll become generally useful enough to be exposed at some point and the get_
prefix is generally elided for accessors.
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.
Done.
This will be used to pack the f64 miter_limit field in `kurbo::Stroke` into a 16-bit encoding for GPU consumption.
Style encodes stroke vs fill style and their related parameters in an 8-byte data structure as described in vello#303. This data structure will replace the existing "linewidth" stream.
Added a test scene specifically for fill rules, drawing a star shape and intersecting arcs with opposite winding.
* The linewidth stream is now the "style" stream. This has been wired up all the way up to the `flatten` pipeline which uses the new encoding to extract the fill rule. * Pipelines downstream of `flatten` still use the old linewidth scheme. They will be fixed up in a follow-up change.
d2715d2
to
a6b3f23
Compare
This PR replaces the existing floating-point "linewidth" stream with a new "style" stream based on the proposal in #303.
flatten
have been wired up to extract the fill style using the new encoding.