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

adding const fn with_a8() to Color #525

Merged
merged 5 commits into from
Jul 24, 2023
Merged

Conversation

flxzt
Copy link
Contributor

@flxzt flxzt commented Jul 7, 2022

similar to with_alpha(), which unfortunately can't currently be const fn due to floating point arithmetic's, I am suggesting to add const fn with_a8().

My personal use case for it: I have a palette of predefined colors (think Gnome Colors), which I use as the basis for other default colors definitions (which is why I need 'const' ).

Related: the discussion that Color probably won't be anything else than u32 so the const is unlikely to become a problem in the future.

With the same thought, as_rgba_u32() can then also converted to be const.

I could also add analogous methods with_r8(), with_g8() and with_b8() to the PR for completeness sake, if the idea is approved.

@xStrom
Copy link
Member

xStrom commented Jul 7, 2022

That discussion is about Copy and f64 implements Copy. If floats cause issues for const then this here is a different decision.

The code itself looks good.

@flxzt
Copy link
Contributor Author

flxzt commented Jul 7, 2022

Sorry, poorly worded. What I meant: Right now Color simply holds an int. When piet would switch to a more complex data type (to support HDR for example) it could be challenging to keep the methods I am introducing here const, because a const fn cannot use regular fn's inside it, and the methods for floating point arithmetics are currently all regular fn's .

So I see two (potential) decisions:

  • Is this change worth it even though it could cause potential problems in the future, although it seems unlikely that the change to a more complex data type that does not allow this any more is ever coming anyway, but if yes:
    • would it be okay to potentially break compatibility then by converting these back to regular fn's if it is impossible to keep them const

piet/src/color.rs Outdated Show resolved Hide resolved
Copy link
Member

@xStrom xStrom left a comment

Choose a reason for hiding this comment

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

Thank you for your effort!

Let's merge this and see how it goes.

@xStrom xStrom merged commit 88e8e6c into linebender:master Jul 24, 2023
15 of 16 checks passed
@flxzt flxzt deleted the color_with_a8 branch July 24, 2023 22:07
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