-
Notifications
You must be signed in to change notification settings - Fork 24
Refactor Brush to use generic types for image and gradient data #134
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
Conversation
1eba552
to
203b17e
Compare
I think we want this for vello_cpu so we can remove: https://github.com/linebender/vello/blob/0640012afc5a5454644ca51f13ee4128529c5ffd/sparse_strips/vello_common/src/paint.rs#L82 cc @LaurenzV |
@DJMcNab Is this intended to land before 0.5.0? |
We talked about this in office hours yesterday. My thinking is to not do so - this is something which it only makes sense to think about once we get Vello API working, probably. If this would be useful for Vello CPU, it could land though. I don't think it has a huge cost. |
This change is backwards compatible, so it can be done in patch release.
My idea is to start passing this type down to vello_cpu renderer (instead of own type) and deal with opacity, BGRA and alpha on sampling, but we could always just continue to roll own sparse strips types (and extend them) like we do now. |
Maybe I am missing something obvious, but how is it backwards-compatible? The PR introduces generic parameters to a public enum, which seems like a pretty breaking change to me, no?" 🤔 |
Generic parameter have default values that are same as before, so they are backwards compatible AFAIK. |
Ahh okay, fair enough, I missed that. |
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 mean to me, that seems reasonable, but I think it might be a good idea to get a draft PR in vello_cpu up working before we actually merge this, to make sure it works as intended.
203b17e
to
a13d6cc
Compare
Hm, I just rebased and docs are failing. |
Signed-off-by: sagudev <[email protected]>
a13d6cc
to
89e44d3
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.
I think this now blocks #140, because I do suspect this is probably still breaking, but only in niche scenarios.
The changes look fine to me
I'm happy enough keeping this out of the changelog; it's not a functionality I particularly want to advertise, because it's a bit messy for our interstitial state.
Signed-off-by: sagudev <[email protected]>
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.
Ah, it hadn't occured to me that this would enable Brush to be used in sparse strips vello. This is now making sense to me.
This is my main motivation. Given that CI passes in linebender/vello#1253 and the amount of support for this I am going to land this (so we can unblock release). |
demo for linebender/peniko#134 --------- Signed-off-by: sagudev <[email protected]>
As discussed in #133 (review) we can also generalize
Brush
over image data and gradient. This makesBrushRef<'a> = Brush<ImageBrushRef<'a>, &'a Gradient>
which allows to share more From impls, most notably various color implementations (even owned version can now support conversion from &color because color is copy anyway).