-
Notifications
You must be signed in to change notification settings - Fork 25
Add Compose::PlusDarker
#112
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
|
I think it's ok to land this now as we already have breaking changes on main. |
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.
Implementation wise this looks correct.
I've requested Chad's review for awareness, in case there's something I'm missing about why this wasn't previously included. But I'm happy landing this, and if something else comes up, we can always revert.
|
I think there was some discussion around this mode when we did the initial blend work but I don’t recall the details. I’m good with landing this assuming support will be added to the vello renderers after this crate is released. |
|
I did quick search in vello and found linebender/vello#169:
|
|
Thanks for digging that up. I suppose this might still be worth adding if it is required for web compatibility even if the specification is invalid. @raphlinus thoughts? |
|
Some more links: |
The results produced by that formula don't match what I see in Figma, Sketch, and iOS. This formula, at least in my tests, produces correct color. |
Yeah, this is also talked in spec: w3c/fxtf-drafts#447 |
|
That formula is weird. If two opaque colors were mixed, the result will be a transparent color |
|
I still think it might be worth landing this, even if exact schematics are not clearly defined yet or else would need new breaking change if we ever want to bring it in again. We can also note this in documentation. |
|
I'd like to see your proposed final doc comment here before approving, but otherwise I'm happy with this going in to unblock Servo. Would we implement it as described in w3c/fxtf-drafts#447 (comment), or is there a different formula? If this is only required for web compatibility, and isn't well-defined, we should probably note that (by linking to w3c/fxtf-drafts#447). |
Signed-off-by: sagudev <[email protected]>
5cfb1db to
35d01d6
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.
If there's consensus around something of the form of my new comment, then I'd be happy to get this in.
I feel that it is valuable for this not to be something which is renderer-defined, even if we attach ourselves to an implementation we have to change as a breaking change. As such, I've proposed a comment which outlines it pretty exactly, which still noting that you shouldn't use it.
The fact that this is only implemented by Safari is something which should have come up earlier in this thread; it's hard to argue that it's an important thing to implement for spec compatibility in this case, given that it only lives in an unapproved, unimplemented draft.
| /// The behavior of this mode is currently not exactly defined, | ||
| /// as spec discussions are still ongoing [w3c/fxtf-drafts#447], | ||
| /// but it's present here to prevent future breaking changes. | ||
| /// | ||
| /// [w3c/fxtf-drafts#447](https://github.com/w3c/fxtf-drafts/issues/447) |
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.
What do you think of a statement like:
/// The upstream specification of this blend mode is not well-formed, and so we
/// cannot recommend using it at present. See the discussion in [w3c/fxtf-drafts#447].
/// This blend mode is currently only implemented on the web by Safari, and is currently
/// provided only for compatibility with rendering on Apple platforms.
/// As such, we define this mode as described in [David Baron's comment in that issue][comment].
/// However, this implementation is potentially disputed, and so we expect that it might change.
///
/// [comment]: https://github.com/w3c/fxtf-drafts/issues/447#issuecomment-1699293766
I feel that the current comment is both too vague for end-users, and too vague for renderer implementers.
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 agree with discouragement message, but I purposefully wrote vague message so we can let render implementers define what plusdarker does (so that it is implementation defined).
Anyway given that this will be deferred to next breaking peniko release (along with mix clip stuff), maybe we can check which formula is actually implemented on apple.
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.
@dbaron if you're willing to confirm whether w3c/fxtf-drafts#447 (comment) is still the best source (externally to Apple) for implementing plus-darker as a renderer, that would be very helpful here.
so we can let render implementers define what plusdarker does (so that it is implementation defined).
Can you clarify why? To me, that seems like the worst of all worlds:
- If we do change to define it exactly, that then has to go into a breaking change anyway.
- Any renderer is going to implement it as specified in the comment I linked, as that is the most up-to-date source (as far as I'm aware).
- We want the Vello renderers to have the same rendering outputs (barring epsilon difference), which this not being well-defined doesn't facilitate.
In terms of the underlying principles to reach these conclusions, I can see why you'd reach this conclusion if you thought that the formula is extremely likely to change from the one provided in the linked comment (which is currently 2 years old). I think that to be unlikely (if nothing else, based on the Lindy effect), so think defining it exactly is low-risk and is very advantageous.
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.
@dbaron if you're willing to confirm whether w3c/fxtf-drafts#447 (comment) is still the best source (externally to Apple) for implementing
plus-darkeras a renderer, that would be very helpful here.
Yeah, I think it is. I haven't had the chance to finish up what I was supposed to do to clean up the spec there, though...
This is the only composite/blending mode that we are missing per spec: https://drafts.fxtf.org/compositing/#compositemode
This is a breaking change.