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

Add epsilon to float comparison #3635

Merged
merged 2 commits into from
Apr 3, 2024

Conversation

bsheedy-work
Copy link
Contributor

Adds an epsilon when comparing float values in
conformance/extensions/webgl-depth-texture.html. As-is, it is possible for tests to fail with output such as:

At 2,0, expected within [0.2,0.6], was 0.600
FAIL At 2,0, expected within [0.2,0.6], was 0.600

@CLAassistant
Copy link

CLAassistant commented Apr 3, 2024

CLA assistant check
All committers have signed the CLA.

Adds an epsilon when comparing float values in
conformance/extensions/webgl-depth-texture.html. As-is, it is possible
for tests to fail with output such as:

At 2,0, expected within [0.2,0.6], was 0.600
FAIL At 2,0, expected within [0.2,0.6], was 0.600
Copy link
Contributor

@kainino0x kainino0x left a comment

Choose a reason for hiding this comment

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

Some of the expectations already have epsilons in them (where the expected min and max are equal). This will double epsilon for those.

It seems the original test was trying to avoid overapplying epsilons but it doesn't seem very important to me. So the epsilons could be removed above (lines 327-328, 330-341) and applied here only.

Removes the epsilon value added to certain parts of the expected min/max
values in conformance/extensions/webgl-depth-texture.html, instead using
only the epsilon applied when actually comparing values.
@bsheedy-work
Copy link
Contributor Author

Some of the expectations already have epsilons in them (where the expected min and max are equal). This will double epsilon for those.

It seems the original test was trying to avoid overapplying epsilons but it doesn't seem very important to me. So the epsilons could be removed above (lines 327-328, 330-341) and applied here only.

Removed.

Copy link
Member

@kenrussell kenrussell left a comment

Choose a reason for hiding this comment

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

Looks good to me too, but let's give @kainino0x a chance to review.

Copy link
Contributor

@kainino0x kainino0x left a comment

Choose a reason for hiding this comment

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

This is what I suggested, so LGTM, but I suggested it, so approving based on Ken's secondary review.

@kenrussell
Copy link
Member

Thanks Kai for suggesting this and reviewing. Merging now.

@kenrussell kenrussell merged commit bc3c8ba into KhronosGroup:main Apr 3, 2024
2 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.

5 participants