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

Geometric test whether ray intersects sphere #83

Merged
merged 6 commits into from
Feb 25, 2019

Conversation

becheran
Copy link
Contributor

Added this feature since I needed it myself for another project where I use Pyrr. Thought it makes sense to add it here.
Function return list of intersection points (possible 0 to 2) calculated via quadratic equation solver added to misc.

@adamlwgriffiths
Copy link
Owner

adamlwgriffiths commented Jan 11, 2019

There's a bit of syntax touching in there. Is that a PEP-8 formatter?
Most of it is fine, but there are a couple that are off:
6c21afd#diff-a1fa0401c2195fa7c8fd9dce2c6c4000R77
6c21afd#diff-c2907d153021da1cec3549a759ba733fR7

Admittedly this library isn't PEP8 formatted, it originates from before I knew of it. I just haven't had a need to update it but I am happy to. However, it would be better if we could do a PEP8 separately.
Would you mind reducing this to just the code changes?

Apart from that, looks good. Thanks for including tests.
If you can undo the syntax we can do the merge.

@adamlwgriffiths
Copy link
Owner

Created #84 to cover syntax changes.

@becheran
Copy link
Contributor Author

Yes sure. Sorry for useless overriding of original format.
Removed those sections from commit.
I am using Pycharm with an PEP 8 auto-formatter. If you want I can re-format the code and assign myself to #84.

@adamlwgriffiths
Copy link
Owner

Sorry been busy, hopefully I'll get some time to look soon.
Appreciate it's not a big change.

pyrr/utils.py Outdated Show resolved Hide resolved
pyrr/utils.py Outdated Show resolved Hide resolved
pyrr/geometric_tests.py Outdated Show resolved Hide resolved
@becheran
Copy link
Contributor Author

Sorry for very(!) late reply to this small issue. Branch should be clean now.

@adamlwgriffiths
Copy link
Owner

Nice man, thanks for coming back and doing that =)
Those changes aren't pointless, the code needs a spring clean, but I just want to keep things atomic.

@adamlwgriffiths adamlwgriffiths merged commit 6aea0ca into adamlwgriffiths:master Feb 25, 2019
@adamlwgriffiths
Copy link
Owner

adamlwgriffiths commented Feb 25, 2019

We should add you to the contributors too. Is that ok with you to put your name in the authors section?

@becheran
Copy link
Contributor Author

Thanks man. Of course, would be nice. Just add me.
I will hopefully find the time to contribute more.

@becheran becheran deleted the ray_intersect_sphere branch February 25, 2019 09:37
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