search

ebassi / graphene

A thin layer of graphic data types
http://ebassi.github.io/graphene
Other
373 stars 80 forks source link

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

Closed sbstnk closed 3 years ago

sbstnk commented 3 years ago

Fixes https://github.com/ebassi/graphene/issues/223

Proposed changes:

Gottox commented 3 years ago

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

ebassi commented 3 years ago

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

ericonr commented 3 years ago

I think merging this as a simple patch is reasonable, but I would recommend going with https://github.com/ebassi/graphene/pull/225 later. I can rebase it.

sbstnk commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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?

GeorgesStavracas commented 3 years ago

I'm fine with it :slightly_smiling_face:

ebassi commented 3 years ago

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

sbstnk commented 3 years ago

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 commented 3 years ago

Yes, sure.