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

Clamp NaN to white when converting to integer #2381

Merged
merged 5 commits into from
Nov 19, 2024

Conversation

FlareFlo
Copy link
Contributor

Linked issue: #2275

@FlareFlo
Copy link
Contributor Author

No clue why clippy throws a fit, this PR does not touch those failing files at all.

@fintelia
Copy link
Contributor

There's new clippy complaints every time a new clippy version comes out. Don't worry about them.

Do you know the performance implications of this change? I'm not sure performance was great to start with, but I don't want to unintentionally add guarantees that will make it unnecessary slow forever.

@FlareFlo
Copy link
Contributor Author

There's new clippy complaints every time a new clippy version comes out. Don't worry about them.

Do you know the performance implications of this change? I'm not sure performance was great to start with, but I don't want to unintentionally add guarantees that will make it unnecessary slow forever.

https://godbolt.org/z/7318KPd4f Not great, but im not sure what else to do

src/color.rs Outdated Show resolved Hide resolved
@kornelski
Copy link
Contributor

It should be possible to replace clamp with custom comparison that handles NaN at zero cost:

if !(x < 1.0) { 1.0 } handles both NaN and >1.0.

@FlareFlo
Copy link
Contributor Author

It should be possible to replace clamp with custom comparison that handles NaN at zero cost:

if !(x < 1.0) { 1.0 } handles both NaN and >1.0.

Something like this appears to produce better codegen indeed https://godbolt.org/z/qnbjWhr3c

@FlareFlo
Copy link
Contributor Author

There's new clippy complaints every time a new clippy version comes out. Don't worry about them.

Do you know the performance implications of this change? I'm not sure performance was great to start with, but I don't want to unintentionally add guarantees that will make it unnecessary slow forever.

I have a few ideas for speeding things up, too much for this PR though. Ill experiment and open a PR when i have something that might make sense.

@fintelia fintelia merged commit 2c986d3 into image-rs:main Nov 19, 2024
31 of 32 checks passed
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