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

Fix isPointInTriangle in 3d (#6882) #7106

Merged
merged 2 commits into from
Mar 27, 2023

Conversation

HydrolienF
Copy link
Contributor

@HydrolienF HydrolienF commented Mar 17, 2023

Solution have been find by @tylerhasman & @tommyettinger but never merged. Closes #6882

I've add fiew more tests.

I think I's usefull to warn user that because of float use, function may return a bad answer with realy small and realy big x,y,z. But if you think it's not usefull, I can revert comment of isPointInTriangle().

Let me know if anything else should be changed.

Solution have been find by tylerhasman & tommyettinger but never added
@tommyettinger
Copy link
Member

Thanks for revisiting this; I had forgotten about it. I'll run the Spotless tasks on this myself, since that's probably why the checks are failing, then commit to your PR (which I think should work). I'll also fix any typos I find.

Copy link
Member

@tommyettinger tommyettinger left a comment

Choose a reason for hiding this comment

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

I didn't change any code; I just ran spotlessApply and the tests, as well as cleaned up some typos, unclear phrasing, and punctuation mistakes in comments. The expanded tests all pass, and I believe the algorithm has been known to work since the original issue. I don't see any reason this can't be merged.

@HydrolienF
Copy link
Contributor Author

@tommyettinger thanks for formating & english check !
Float warning is easier to understand that way.

I thought formating was done by a Github action. When I had run it on my computer (Linux) it edit all .java file. It probably only edit end line, but I was afraid that my merge request show change in every .java file.

@tommyettinger
Copy link
Member

Formatting is run by a GH Action, but to avoid the potential for spammers creating many PRs and running problematic actions on GitHub's servers, no actions will run for PRs by first-time contributors (just to be sure that it's running actions on PRs from someone it knows). You can run spotlessApply and it will claim all files have changes, but when you actually submit the PR only a few files will have actually been altered (the ones you changed by hand, usually). As you said, it is probably just changing the line endings, which Git will usually ignore when it pushes the changes.

@SimonIT
Copy link
Member

SimonIT commented Mar 18, 2023

The automatic spotless fix runs on your own repository. If you don't have it enabled for the fork, it doesn't. It wasn't (and probably still isn't) possible to push from the PR build

@HydrolienF
Copy link
Contributor Author

Do I need to do something else before merge ?

@SimonIT SimonIT merged commit 9c6a913 into libgdx:master Mar 27, 2023
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.

isPointInTriangle returning true when it shouldn't
3 participants