-
Notifications
You must be signed in to change notification settings - Fork 41
Add automated unit tests for ray intersection and collision functions #151
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
base: main
Are you sure you want to change the base?
Conversation
Introduces comprehensive unit tests for ray intersection with rectangles, circles, triangles, and quads in unit_test_geometry.cpp, including hit point and distance checks. Adds bitmap ray collision detection tests in unit_test_bitmap.cpp, covering various scenarios and bitmap cells. These tests improve coverage for ray-based collision and intersection logic.
Expanded the pull request template to include details about new automated unit tests for ray intersection functions, replacing manual interactive tests. Added sections for test details, reproduction steps, and updated checklists to reflect the new testing approach.
rory-cd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General Information
Type of Change:
- New unit tests
This PR provides well-structured unit tests translated from the geometry integration test, using sections to avoid duplicating code and cleanly separating test scenarios. While some changes are required to ensure tests are robust, this will make a solid foundation for testing ray intersection and collision functions.
Requested changes are marked inline, along with some optional improvements.
Code Quality
- Repository: The Pull Request is made to the correct repository
- Readability: The code is generally easy to read and follow, with useful comments provided
- Maintainability: This code could easily be maintained or extended in the future
Functionality
- Correctness: While the code nicely translates much of the integration test into unit testing format, one of the tests fails, and some of the tests cannot fail.
- Impact on Existing Functionality: The impact on existing functionality been considered
Testing
- Test Results: Ran
skunit_tests. One test failed, due to lines 1070 and 1071 ofunit_test_geometry.cpp. Details in inline comments.
Pull Request Details
- PR Description: The problem being solved is clearly described
- Checklist Completion: All relevant checklist items have been reviewed and completed
| bitmap bmp_1 = load_bitmap("on_med", "on_med.png"); | ||
| bitmap bmp_2 = load_bitmap("rocket_sprt", "rocket_sprt.png"); | ||
| bitmap bmp_3 = load_bitmap("up_pole", "up_pole.png"); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional improvement: To check whether the rays should be intersecting these bitmaps, the image sizes must be checked. Consider creating bitmaps and drawing shapes onto them manually, as this would make the sizes explicit, and reduce room for error.
If the resources are kept, consider adding null checks here (e.g. REQUIRE(bmp_1 != nullptr);). This would give consistency with the other tests in this file, and make any errors more specific (for example, when a bitmap is not null, but is not valid).
| // Ray heading towards first bitmap | ||
| vector_2d ray_heading = vector_point_to_point(ray_origin, bmp_1_position); | ||
| bool collision_1 = bitmap_ray_collision(bmp_1, 0, bmp_1_position, ray_origin, ray_heading); | ||
|
|
||
| // Ray heading towards second bitmap | ||
| ray_heading = vector_point_to_point(ray_origin, bmp_2_position); | ||
| bool collision_2 = bitmap_ray_collision(bmp_2, 0, bmp_2_position, ray_origin, ray_heading); | ||
|
|
||
| // Ray heading towards third bitmap | ||
| ray_heading = vector_point_to_point(ray_origin, bmp_3_position); | ||
| bool collision_3 = bitmap_ray_collision(bmp_3, 0, bmp_3_position, ray_origin, ray_heading); | ||
|
|
||
| // At least one should be true depending on bitmap transparency | ||
| REQUIRE((collision_1 || collision_2 || collision_3)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue:
This section seems to be meant for testing ray collisions with multiple bitmaps simultaneously. At the moment, this is three separate rays testing three separate bitmaps, and the test only fails if one of these rays doesn't intersect.
Suggested fix:
- To avoid transparency issues, bitmaps should be created/drawn rather than loaded.
- The ray should intersect all three bitmaps simultaneously
- The assertion should be
REQUIRE((collision_1 && collision_2 && collision_3))
| // This depends on bitmap dimensions, so we just verify it runs without error | ||
| REQUIRE((collision_below == true || collision_below == false)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue:
This assertion always passes, and therefore isn't improving the test case.
Suggested fix:
- Change to
REQUIRE(collision_below);and ensure the bitmap dimensions are static. - The section only tests rays from different locations (rather than edge cases or different functionality), and may be better off as part of the "can detect ray collision with bitmap" section. This would also fit with how you approached "can perform rectangle ray intersection" in unit_test_geometry.cpp.
| // Test with cell 0 (default) | ||
| bool collision_cell_0 = bitmap_ray_collision(bmp_2, 0, bmp_position, ray_origin, ray_heading); | ||
| REQUIRE((collision_cell_0 == true || collision_cell_0 == false)); | ||
|
|
||
| // Test with different cells (if bitmap has animation cells) | ||
| // For single-cell bitmaps, this should behave the same as cell 0 | ||
| bool collision_cell_1 = bitmap_ray_collision(bmp_2, 1, bmp_position, ray_origin, ray_heading); | ||
| REQUIRE((collision_cell_1 == true || collision_cell_1 == false)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue:
This is a good case to test, but again the assertions always pass.
Suggested fix:
Make assertions more specific, e.g. REQUIRE(collision_cell_0);
Optional improvement:
Test bitmaps with multiple cells (e.g. A case where cell 1 should have a collision but cell 2 shoudn't)
| // Ray that misses (extremely small x component) | ||
| REQUIRE_FALSE(rectangle_ray_intersection(point_at(90.0, 110.0), vector_to(__DBL_MIN__, 0.0), r1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of __DBL_MIN__ here doesn't seem necessary unless the test relates to near-zero values. Since the purpose of the test is to check missed rays, could this be rectangle_ray_intersection(point_at(95.0, 95.0), vector_to(1.0, 0.0), r1) instead?
| // Ray that doesn't intersect | ||
| intersects = rectangle_ray_intersection(point_at(50.0, 50.0), vector_to(-1.0, -1.0), r1, hit_point, distance); | ||
| REQUIRE_FALSE(intersects); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional improvement: This test was already done in the previous test case. Instead, consider covering the case where the ray starts from inside the rectangle (as noted in the function documentation)
| REQUIRE(hit_point.x == Catch::Detail::Approx(360.0).margin(EPSILON)); | ||
| REQUIRE(distance == Catch::Detail::Approx(60.0).margin(EPSILON)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue:
These lines were causing the test to fail, due to incorrect values. According to the API Docs, hit_point.x should be 300, and distance should be 0.
Suggested fix:
Change to:
REQUIRE(hit_point.x == 300.0);
REQUIRE(distance == 0);| REQUIRE(hit_point.x > 350.0); | ||
| REQUIRE(hit_point.y == Catch::Detail::Approx(450.0).margin(EPSILON)); | ||
| REQUIRE(distance > 0.0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue:
- Line 1106 is too vague to test correctness - This is understandable, since the triangle is quite skewed, so the distance is difficult to calculate exactly. But at the moment, this is saying "as long as the hit point is detected after the ray origin, that's a success"
- Line 1108 is also too vague - as per above
Suggested fix:
- Provide a more specific equality assertion, or an upper bound
- Use an axis-aligned, equilateral triangle to make calculations/assertions simpler
| REQUIRE(hit_point.x > 50.0); | ||
| REQUIRE(distance > 0.0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue:
- Line 1143 assertion is too vague - As per the triangle test, this is understandable, since the quad is skewed, but at the moment this assertion doesn't test correctness.
- Line 1144 assertion is too vague - as per above
- Assertion for
hit_point.yis missing
Suggested fix:
- Provide more specific equality assertions, or upper bounds
- Use an axis-aligned quad to make calculations/assertions simpler
- Add assertion for
hit_point.y
| } | ||
| } | ||
|
|
||
| TEST_CASE("can detect closest ray intersection among multiple shapes", "[geometry][ray_intersection]") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test case reflects the integration test, but as a unit test it doesn't pertain to a particular SplashKit function, and doubles up on some functionality already tested. We know from previous tests that each of these functions returns an accurate value, so nothing new is being tested. The implementation is effective but I would suggest removing it from the unit tests.
Description
Added automated unit tests for ray intersection functions, migrating interactive tests from
test_geometry.cppto proper unit tests that can run in CI/CD pipelines.Changes:
rectangle_ray_intersectioninunit_test_geometry.cppcircle_ray_intersectioninunit_test_geometry.cpptriangle_ray_intersectioninunit_test_geometry.cppquad_ray_intersectioninunit_test_geometry.cppbitmap_ray_collisioninunit_test_bitmap.cppMotivation:
The ray intersection functionality previously only had interactive visual tests that required manual inspection. These new automated tests enable continuous integration testing and prevent regressions.
Fixes # (issue)
Type of change
How Has This Been Tested?
Test Details:
To reproduce:
Testing Checklist
Checklist