Skip to content

Add support for float colours on the GPU #970

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

DJMcNab
Copy link
Member

@DJMcNab DJMcNab commented May 6, 2025

As requested by @LaurenzV in #967.

This probably shouldn't be merged. Note that a tolerance of zero still doesn't work, for unknown reasons.

One potential reason is that the GPU is getting better precision; because it might do blending in f32. But I don't know.

@DJMcNab
Copy link
Member Author

DJMcNab commented May 6, 2025

The messages for those test failures could be a lot more helpful - linking to the diff image, for example. We probably should also set up #652 for this (and update it to reflect #892 as well)

@LaurenzV
Copy link
Contributor

LaurenzV commented May 6, 2025

Using my branch I'm only getting 14 failures with 0 tolerance, and they are mostly at the edges, e.g.
image

However, there is one image where the difference is quite big:
image

Will see whether I can investigate what is going on here.

@LaurenzV
Copy link
Contributor

LaurenzV commented May 7, 2025

Guess we just got unlucky... At some point we end up with the number 0.7509804 on the CPU, which multiplies to 191.500002, and therefore gets rounded up to 192. On the GPU, it gets rounded down apparently. Not sure there is much point in investigating this further though, so I think the best thing would be to just stick with the default tolerance threshold of 1.

It might still be nice to merge the change in this PR at some point, for the sake of consistency across the renderers.

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