Skip to content

ray: Fix ray/box intersection on systems without isnanf#224

Closed
sbstnk wants to merge 2 commits into
ebassi:masterfrom
sbstnk:fix-ray-box-non-isnanf
Closed

ray: Fix ray/box intersection on systems without isnanf#224
sbstnk wants to merge 2 commits into
ebassi:masterfrom
sbstnk:fix-ray-box-non-isnanf

Conversation

@sbstnk

@sbstnk sbstnk commented Apr 11, 2021

Copy link
Copy Markdown
Contributor

Fixes #223

Proposed changes:

  • Fix the typo in the non-HAVE_ISNANF code path to match the behavior of the HAVE_ISNANF code path.

There was a typo in the code copied from the HAVE_ISNANF path that
caused systems without isnanf (such as those using musl) to wrongly
detect an intersection when there is none.

Fixes ebassi#223
@Gottox

Gottox commented Apr 11, 2021

Copy link
Copy Markdown
Contributor

It would be awesome to add a testcase for this issue.

@ebassi

ebassi commented Apr 11, 2021

Copy link
Copy Markdown
Owner

Indeed, it would be awesome to have a test case. :-)

@ericonr

ericonr commented Apr 11, 2021

Copy link
Copy Markdown
Contributor

I think merging this as a simple patch is reasonable, but I would recommend going with #225 later. I can rebase it.

@sbstnk

sbstnk commented Apr 11, 2021

Copy link
Copy Markdown
Contributor Author

I wasn't sure about adding a test case, because due to the nature of the bug that would just be some random numbers for which it happens to fail instead of a real class of issues. But I can add the example from the bug report as a test. Can't think of a good name for such a test though.

@sbstnk

sbstnk commented Apr 11, 2021

Copy link
Copy Markdown
Contributor Author

I was thinking of turning the code I used to find these random numbers into a test that does picking for ~1000 random coordinates like it would be done in clutter/mutter. Not sure how useful such non-deterministic tests would be though.

@sbstnk

sbstnk commented Apr 11, 2021

Copy link
Copy Markdown
Contributor Author

I couldn't find a way to make a useful test with random positions, so I went with some hardcoded ones.

This includes code based on Mutter/Clutter which was written by @GeorgesStavracas. Since Mutter is GPL and graphene is MIT, are you okay with using that code under MIT @GeorgesStavracas?

This test is based on the picking code in Mutter/Clutter.
@sbstnk sbstnk force-pushed the fix-ray-box-non-isnanf branch from bc80fc9 to e262a98 Compare April 11, 2021 21:59
@GeorgesStavracas

Copy link
Copy Markdown
Contributor

I'm fine with it 🙂

@ebassi

ebassi commented Jun 6, 2021

Copy link
Copy Markdown
Owner

I think both #225 and #220 have fixed most of these issues.

@ebassi ebassi closed this Jun 6, 2021
@sbstnk

sbstnk commented Jun 6, 2021

Copy link
Copy Markdown
Contributor Author

Would it make sense to open a separate PR with just the test since a test was specifically requested here and the merged fix did not have one?

@ebassi

ebassi commented Jun 6, 2021

Copy link
Copy Markdown
Owner

Yes, sure.

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.

Wrong results for ray/box intersection on systems without isnanf

5 participants