test(geom): add tests for Rectanglei #23

skaldarnar · 2021-02-15T21:00:08Z

feat(geom): add getMin/getMax to Rectanglei/f/d
test(geom): Overhaul tests for Rectanglei
- containsPoint
- containsRectangle
- intersection
- (invalid) RectangleFromPoint

The tests also describe (desirable) contracts for the implemenation. Some tests are currently failing!

4Denthusiast

I've mostly only reviewed the RectangleiTest changes. Although I think that a half-inclusive interval (and by extension, rectangle and cuboid) is a more useful primitive, making it all inclusive is at least consistent with BlockRegion, which is more important. There should be tests for isValid too (both rectangles that should and shouldn't be valid).

I would otherwise be fine with leaving it without a test, because it should be pretty obvious, but they're actually currently incorrect: the length and area methods could do with tests too. Ideally this would count the number of points in some rectangles and verify that this matches the reported area, rather than just checking the expected value directly. The containsPoint methods could do with tests too, ideally checking the consistency with the other methods (containsRectangle, union, intersection). The correct semantics for the float version of containsPoint is particularly ambiguous. I feel like the area of the rectangle of floats that a Rectanglei represents ought to have an area equal to the number of integer points it contains, but that leaves open the question of in which direction should the set of included float points extend beyond the included integer points. If it were half-inclusive, the answer would be obvious, but in this case, not so much. Extending beyond each edge by 0.5 is probably best, for consistency with the block grid. Extending it by 1 in the +x and +y directions is also possible, but then you need to have it exclude the next integer coordinate, which is a little inconsistent with all the shapes otherwise being inclusive of their edges (this is another advantage of it extending by 0.5 in each direction). Simply not having a Rectanglei.containsPoint(float, float) method would also be a good option, as (given that we're using inclusive boundaries) there is no option that wouldn't be odd or confusing somehow.

As a general point, a lot of these tests are sort of trivial, and it doesn't seem like getting the tests correct is actually easier than implementing the class itself correctly. A testing strategy that seems somewhat more productive would be testing the consistency of the different methods, e.g. testing that the intersection of two rectangles contains a point iff both original rectangles do, if rectangle A contains rectangle B, which contains rectangle or point C, then A contains C too, the number of integer points contained in a Rectanglei is the same as its area, and the union of two rectangles contains both original rectangles. Statements like these are true for all rectangles, so you can just test them for all the rectangles that fit inside a certain area in a loop, and I think they would be more likely than the current tests to actually reveal errors.

4Denthusiast · 2021-03-07T23:15:17Z

joml-geometry/src/main/java/org/terasology/joml/geom/Rectangled.java

     * @return <code>true</code> iff this rectangle contains the rectangle; <code>false</code> otherwise
     */
    @Override
    public boolean containsRectangle(Rectangleic rectangle) {
        return rectangle.minX() >= minX && rectangle.maxX() <= maxX &&
-               rectangle.minY() >= minY && rectangle.maxY() <= maxY;
+                rectangle.minY() >= minY && rectangle.maxY() <= maxY;


Why this change? It seems to have been nicely aligned before. Is it just to make it an integer multiple of a tab, 4 spaces? That doesn't really make sense if there's no particular reason to have 2 tabs here. It should be either 4 spaces (1 tab, the next standard indentation level) or preferably 7 (so that it actually lines up). (This same comment applies in a lot of other places too.)

That's what the IntelliJ auto-formatter does 🤷 Since we have a common configuration committed in all our repos using that should be fine, or we should put some gradlew plugin to ensure consistent formatting.

I did not change anything by hand here.

4Denthusiast · 2021-03-07T23:25:19Z