Skip to content
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

Derive PartialEq for AlphaColor, OpaqueColor, PremulColor #76

Merged
merged 1 commit into from
Dec 10, 2024

Conversation

waywardmonkeys
Copy link
Collaborator

No description provided.

@waywardmonkeys
Copy link
Collaborator Author

A possible issue here is that DynamicColor has a manual impl for PartialEq where it directly does a comparison of the bits.

Copy link
Contributor

@raphlinus raphlinus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Riiight, that's the reason I didn't do this before.

I think we need a PartialEq, so I'm approving this. What's far less clear is what semantics it should have. In particular, I don't think we should guarantee that a color component of 0.0 is equal to a color component of -0.0, ie reserve the right to make that change. Of course we're making no stability guarantees at this point.

@tomcur
Copy link
Member

tomcur commented Dec 2, 2024

I wonder whether it makes sense to follow #75 and impl PartialEq, Hash based on the bits. There is some divergence between the types, with DynamicColor not being bytemuck::Pod, so maybe that informs the decision. An alternative is to provide the required functionality on DynamicColor (maybe to_bits) so users can themselves implement Hash, Eq on some wrapping type.

Copy link
Member

@tomcur tomcur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Happy for this to land as-is.)

@waywardmonkeys
Copy link
Collaborator Author

Landed this as is ... I need to do some stuff for Default as well so that we can use this with Parley which requires that a Brush impl PartialEq and Default.

Merged via the queue into linebender:main with commit 1f8f23f Dec 10, 2024
15 checks passed
@waywardmonkeys waywardmonkeys deleted the equality branch December 10, 2024 02:43
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.

3 participants