Skip to content

Clamp NaN to white when converting to integer#2381

Merged
fintelia merged 5 commits intoimage-rs:mainfrom
FlareFlo:2275-float-nan-conversion
Nov 19, 2024
Merged

Clamp NaN to white when converting to integer#2381
fintelia merged 5 commits intoimage-rs:mainfrom
FlareFlo:2275-float-nan-conversion

Conversation

@FlareFlo
Copy link
Copy Markdown
Contributor

Linked issue: #2275

@FlareFlo
Copy link
Copy Markdown
Contributor Author

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

@fintelia
Copy link
Copy Markdown
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
Copy Markdown
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

@kornelski
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
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