added circle-triangle intersection functions

[matt439]

Description

There is no triangle-circle intersection test currently available in SplashKit, despite
being a useful test for game development.
I created two functions which will be expand the utility of the SplashKit library.
The first detects whether or not a circle and triangle are intersecting and returns
the Boolean result
The second has the functionality of the first but also supplies the closest point on the
triangle to the circle's center. This is achieved by passing a point_2d reference to the
function. Passing a value by reference to receive output may be complicated to
beginner programmers. Fortunately, the first function operates in a standard manner
and will be easily understandable.

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as
  expected)
• Documentation (update or new)

How Has This Been Tested?

I created a graphical test of my functions in the test_geometry.cpp file.
Four static triangles are present on the screen. A circle follows the mouse
cursor across the screen. The circle's size can be increased and decreased by
pressing the up and down arrow keys. If a triangle is intersecting with the
circle, it will change colour. The point_2d returned by my function is displayed
as a smaller circle on the surface of each triangle.

Testing Checklist

• Tested with sktest
• Tested with skunit_tests

Checklist

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have requested a review from ... on the Pull Request

[Liquidscroll]

Looks really good to me! I'd like one small change in the function description, just to clear up a tiny bit of ambiguity.

Great work overall! I really like the fact that you've made your test to be interactive!

[Liquidscroll]

I think this should say something like "The closest point on the triangle is assigned to the input parameter p, even if...", since this function will return a bool not a point.

[matt439]

Thanks for the feedback. I agree that the use of 'return' is incorrect in this case so I updated the description wording.

[hugglesfox]

Just curious as to why these are seperate functions?

[matt439]

My thinking is that a user may not care what the closest point is, instead they simply want to know the intersection result. With only one function, they would be forced to create a point_2d and pass it as an argument. By having separate functions, the user can select the function which is applicable to them.

[hugglesfox]

That's fair. In that case would it make sense to have circle_triangle_intersect_closest_point return the point of intersection (I'm using the term return loosely here, the function could return void and instead update a pointer passed in as an argument) and circle_triangle_intersect return a boolean of whether they are intersecting?

My goal here is to potentially better distinguish the purpose for each function to avoid confusion for when one should be used over the other.

[matt439]

To cover all use cases, there would need to be three functions: one that returns only the Boolean, one that returns only the point, and a third which returns both. That seems a bit excessive, however, so I think cutting it down to one or two functions total is ideal. Ultimately, I think consistency with the other SplashKit functions is the most important factor.

[WhyPenguins]

I think having closest_point_on_triangle_from_circle and circle_triangle_intersect (which uses the other one internally) would match the current API the best, though I understand it's less efficient when both the closest position and intersection test are wanted. For SplashKit I don't think that matters too much😋, and this way would match the existing API better.

I think it it'd be alright to have a circle_triangle_intersect_closest_point in addition to those two, though I'd be tempted to just make it an overload of circle_triangle_intersect instead haha. Does this seem like a decent option?

[hugglesfox]

Looks good.

See on going conversation around code structure, however I don't feel the conversation should block the PR from mentor review.

[hugglesfox]

That's fair. In that case would it make sense to have circle_triangle_intersect_closest_point return the point of intersection (I'm using the term return loosely here, the function could return void and instead update a pointer passed in as an argument) and circle_triangle_intersect return a boolean of whether they are intersecting?

My goal here is to potentially better distinguish the purpose for each function to avoid confusion for when one should be used over the other.

[WhyPenguins]

Sorry for the delay in checking this. Looks good overall!

Just had a question regarding the implementation (perhaps it can re-use some existing functions a bit more), and some thoughts on the API. Even as-is I think it'd be alright to consider done, but a couple of changes would make it match SplashKit's codebase better and make it more likely to be merged upstream😄

[WhyPenguins]

The logic in this looks good, but I do wonder if it could be re-using existing functions a bit more. Just looking at how closest_point_on_rect_from_circle handles things, I think it could be turned into something like this:

if (point_in_triangle(c.center, tri))
{
    p = c.center;
    return true;
}

p = closest_point_on_lines(c.center, lines_from(tri), idx);
return vector_magnitude_squared(vector_point_to_point(p, c.center)) < c.radius * c.radius;
// Untested xD

Just interested what your thoughts are on this? I think the logic is fine as-is, but perhaps something like this might make it clearer and easier to extend to other shapes later on.

(it does admittedly make an unnecessary allocation, but I'm not too worried about SplashKit being performant haha)

[matt439]

I altered my function to use closest_point_on_lines but it produced incorrect results. I inspected the function and it appears to be faulty. For instance, pt is returned rather than result. This means that the last line in the vector will always be the "closest" line. It seems to me that there needs to be a check within the for loop for when i == 0 that then assigns pt to result, i to line_idx, and dst to min_dist. I am not sure how to proceed. Should I create a separate pull request to fix closest_point_on_lines or keep my own version in my intersection function?

[WhyPenguins]

Oh wow, that's not good 😅 Yeah if you want to fix the function, feel free to make a task and a PR for it 😄 I'm happy to mentor review it quickly and get it merged in so it can be used in conjunction with this PR. Otherwise feel free to keep your current method, and we can add the task and get that function fixed later on, either way is fine by me 😋

[WhyPenguins]

I think having closest_point_on_triangle_from_circle and circle_triangle_intersect (which uses the other one internally) would match the current API the best, though I understand it's less efficient when both the closest position and intersection test are wanted. For SplashKit I don't think that matters too much😋, and this way would match the existing API better.

I think it it'd be alright to have a circle_triangle_intersect_closest_point in addition to those two, though I'd be tempted to just make it an overload of circle_triangle_intersect instead haha. Does this seem like a decent option?

[WhyPenguins]

Awesome work on this!😃