Skip to content

Conversation

@ralphweng2023
Copy link

Description

Investigates and implements strategies for unit testing SDL2-based graphics functionality in SplashKit. Previously, testing drawing procedures was awkward, had lower accuracy, and required too many lines of code.

This PR adds a reusable helper library and sample tests demonstrating:

New Helper Functions (graphics_test_helpers.h):

  • color_difference(c1, c2) - Calculate difference between two colors (0.0-1.0 scale)
  • colors_match(c1, c2, tolerance) - Check if colors match within tolerance
  • bitmaps_match(a, b, tolerance) - Pixel-by-pixel bitmap comparison
  • bitmap_diff_count(a, b) - Count differing pixels for debugging
  • verify_filled_circle(bmp, center, radius, fill_color, bg_color) - Validate circle drawing
  • verify_filled_rectangle(bmp, rect, fill_color) - Validate rectangle drawing

Sample Tests (unit_test_graphics.cpp):

  • Tests for helper functions (color comparison, bitmap comparison)
  • Drawing tests for fill_circle, fill_rectangle, fill_ellipse, draw_line
  • Uses create_bitmap + option_draw_to() for offscreen rendering (no window required)
  • Calls setup_collision_mask after drawing to bitmaps as required

This approach reduces test code from ~30 lines to ~10 lines per drawing test case, with better accuracy and maintainability.

Type of change

  • New feature (helper library for testing)
  • New tests

How Has This Been Tested?

Built the test project using CMake as per build instructions. All tests pass successfully.

Automated Testing:

cd projects/cmake
cmake --preset macOS
cmake --build build/

cd ../../bin
./skunit_tests "[graphics]"
# Result: All tests passed (28 assertions in 7 test cases)

./skunit_tests --order rand
# Result: All tests passed (1291 assertions in 86 test cases)

Testing Checklist

  • Code compiles without errors
  • All 7 new graphics test cases pass (28 assertions)
  • All existing tests still pass (86 total test cases, 1291 assertions)
  • Helper functions correctly normalize color values (0-255 to 0-1 scale)

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
  • My changes generate no new warnings
  • New tests have been added to prove the functionality works
  • Helper functions are inline for header-only usage

- Create graphics_test_helpers.h with bitmap comparison utilities
- Add tests for fill_circle, fill_rectangle, fill_ellipse, draw_line
- 7 test cases with 28 assertions, all passing
Copy link

@Broccoli811 Broccoli811 left a comment

Choose a reason for hiding this comment

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

This PR provides a well-designed and reusable strategy for unit testing graphics functionality in SplashKit. The bitmap-based, offscreen approach significantly improves test reliability and directly addresses the issues described in the task card.

The helper functions are clearly documented and flexible, particularly the tolerance-based color comparison and bitmap matching utilities. The shape tests demonstrate clean, maintainable patterns that future contributors can easily extend when needed.

Consider briefly documenting why bitmap-based testing was chosen over window-based SDL2 rendering, to strengthen the investigation aspect of this card.

Also, the elaborate title comments are probably unneeded, if you look into other .cpp files there aren't any big section labels to signify the intention of the code blocks.

As for the new helper functions, I am unsure if adding a separate file to the unit_tests folder is appropriate or not. I'll have to ask someone else the conventions behind creating a new .h file for the purposes of a unit test.

I have tested this on my machine, and it comes out a success so no problems there.

Overall, this is a strong contribution that meaningfully improves graphics unit testing. Approved.

using namespace splashkit_lib;

// =============================================================================
// Helper Function Tests

Choose a reason for hiding this comment

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

This title comment is probably unneeded. To keep the styling concise, remove these.

}

// =============================================================================
// Shape Drawing Tests

Choose a reason for hiding this comment

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

This title comment is probably unneeded. To keep the styling concise, remove these.

}

// =============================================================================
// Screen Dimension Tests

Choose a reason for hiding this comment

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

This title comment is probably unneeded. To keep the styling concise, remove these.

Copy link

@rory-cd rory-cd left a 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 feature (helper library)
  • New unit tests

Code Quality

  • Repository: Pull Request is made to the correct repository
  • Readability: The code is easy to read and follow
  • Maintainability: This code could easily be maintained or extended in the future

Functionality

  • Correctness: The ticket called for investigation/implementation of unit testing strategies for SDL2 drawing procedures. The PR is a useful step in the right direction, however does not address the key concern outlined in the ticket. More on this below.
  • Impact on Existing Functionality: The impact on existing functionality has been considered and tested

Testing

  • Test Coverage: Unit tests provided for new or modified code
  • Test Results: All tests passed

Documentation

  • Documentation: Applicable inline and external documentation is updated and clear

Pull Request Details

  • PR Description: The problem is clearly described
  • Checklist Completion: All relevant checklist items have been reviewed and completed

Discussion

Ticket Overview

The ticket aims to accurately test drawing procedures, rather than approximating by sampling pixels. It shows trigonometry-based sampling as an example of what to avoid, stating "to test the drawing, it is awkward, has lower than perfect accuracy, and requires too many lines of code".

The ticket suggests researching alternatives, and offers two potential options for testing drawing procedures:

  1. Reverse engineer the dimensions of the shape using drawn pixels
  2. Do a pixel-by-pixel comparison of a SplashKit-drawn bitmap to a control bitmap (drawn using SDL code)

Test Overview

🎯 "fill_circle draws correctly"

This test uses trigonometry-based sampling. The main loop is done in a helper library function (verify_filled_circle), which samples points in a circle at 80% of the radius, and then 120%. The test itself is split into sections checking:

  1. Circle center point
  2. A point inside the radius
  3. A point outside the radius
  4. Sample points in a circle at 80% of the radius, and then 120% (the helper function)

The helper function already checks points 2 and 3, making their inclusion redundant. However, it is helpful having separate assertions for points inside and outside of the circle. Since this is the only test where the verify_filled_circle function is called, the function's body could be split up into sections in the test itself.

🎯 "fill_circle with point_2d parameter works correctly"

Similar functionality to the previous test, but without checks 2 and 3. The only additional assertion from the previous test is that the center point is checked before drawing the circle. Therefore, this test appears redundant.

🎯 "fill_rectangle draws correctly"

This test samples:

  1. The center point
  2. Five points in a dice-like shape (including the center point) via verify_filled_rectangle
  3. A point in the background

Like the fill_circle test, the verify_filled_rectangle function is only called in this one test.

🎯 "fill_ellipse draws correctly"

This test samples:

  1. The center point
  2. A point in the background

Therefore it is not a strong measure of an accurately drawn shape.

🎯 "draw_line draws correctly"

As above.

Key Issue

While there are some improvements which could be made to avoid redundant checks, or bring niche helper functions (like verify_filled_circle) into the .cpp file, the key issue is that the majority of the tests use approximate sampling techniques, rather than the more accurate suggestions offered by the ticket.

The PR includes the function bitmaps_match. The function is useful and well implemented, but not utilised outside of testing itself. It could be used to implement the methods suggested by the ticket.

Changes Requested:

graphics_test_helpers.h

  • Remove the encapsulating namespace splashki_lib {}. The helpers aren't part of the core SDK, so don't need to be part of the splashkit_lib namespace. This approach is used by logging_handling.h too.
  • Move verify_filled_circle and verify_filled_rectangle to the .cpp, as these are specific to one test.
  • color_difference and colors_match are useful functions, but they are very similar to the pixel_is() function written by @Broccoli811 in #148. Suggest splitting this PR, by opening a new PR just for graphics_test_helpers.h. The small PR could be merged quickly, allowing both this and 148 to utilise the shared helper functions.

unit_test_graphics.cpp

Use the bitmap comparison helpers in graphics_test_helpers.h to rewrite the tests, using one of the approaches suggested by the ticket. Either:

  • Reverse engineer the shapes
  • Compare with a "control bitmap" drawn using pure SDL
  • A different, yet still accurate method

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.

4 participants