-
Notifications
You must be signed in to change notification settings - Fork 3
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
Add public docs to the main color structs #19
Conversation
I'll have to review this. The terminology seems inconsistent
Pretty unreliable lint overall. Some false positives, lots of false negatives.
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.
Thanks, looks good, just a few clarifications.
/// Contains an RGB image in a linearized RGB color space. | ||
/// | ||
/// The image is stored as pixels made of three 32-bit floating-point RGB components which are | ||
/// considered to be in an unspecified linearized color space. |
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.
Currently when converting to linear, we convert to BT709 primaries. Though when a user builds this struct, it could be in any set of color primaries. I do wonder what the most "technically correct" thing to do is here. Not something to worry about for this PR, but a thought I had.
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.
Maybe include the color primaries in LinearRgb? You could make the case for a new crate that just provides correct color space metadata and handling (without the burden of implementing transfer functions, maybe via a trait)... idk
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.
Adding a field here could make sense. I suppose it only truly matters if the conversion from LinearRgb to the new color model depends on specific primaries. With HSL it should in theory work with whatever the LinearRgb is using, but some like XYB may be expecting specific primaries.
Anyway, not something we need to change right at this time.
@@ -5,6 +5,7 @@ | |||
#![allow(clippy::inline_always)] | |||
#![allow(clippy::module_name_repetitions)] | |||
#![allow(clippy::similar_names)] | |||
#![allow(clippy::doc_markdown)] |
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.
Was there some sort of issue that required this to be allow
ed?
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.
This seems like a pretty unreliable lint overall. A lot (>50%) of cases that should have been marked weren't, and it also seems to just mark anything in camelCase or PascalCase as needing backticks. YCbCr was one such victim.
Co-authored-by: Josh Holmer <[email protected]>
Color theory is complex, and I am not an expert. Go easy on me.
I think @shssoichiro should proofread the docs...
There are some things I am not 100% sure about because I think some things like gamma correction are implicitly assumed? It is a bit difficult to figure all this out, so it would be good to have this information explicitly mentioned somewhere.