Skip to content

Conversation

@virtulis
Copy link
Contributor

This is quite a mess right now and nowhere near ready, but making a PR in case you have an opinion on the matter :)

(Plus, surprisingly, it does seem to work somewhat already)

My main concern is the fact that CMYK colors don't make much sense in RGB when decoupled from color profile. So I see at least three ways of handling this:

  • Just make sure CMYK and RGB colors/pixels don't mix, ever, and leave any conversion between the two to the user. This is what I did right now with a bunch of todo!s, which could just become panic!s instead.
  • Add a feature to integrate with lcms2 and/or a trait for custom conversion implementation, to be used by the renderer(?) when needed. I worry this can get very slow and/or inaccurate, and will probably require otherwise pointless arguments to a bunch of functions.
  • Transparently convert between CMYK/RGB linearly. This will avoid panics but also lead to entirely wrong results in ways that might not be obvious.

The first option seems to be the simplest and safest, if not pretty.

@Logicalshift
Copy link
Owner

Finally had enough time to look through this :-)

This looks really good so far: very much looks like the approach I would have taken here.

I'd probably try to find a way to convert between RGB and CMYK even if the results weren't perfect as it's a bit more polite than panicking. That said, I haven't researched or thought too deeply about this, so this might not make sense.

One way to use a more configurable conversion would be a filter/conversion stream much earlier on in the rendering, similar to how drawing_with_laid_out_text and friends work: any parameters needed to get an accurate conversion can be kept with the stream so they don't need to be passed around in any other functions.

(Fonts don't show up if this stream isn't set up, so perhaps an equivalent behaviour here would be to pick a fixed colour wherever a conversion isn't available)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants