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.

Sign up for GitHub

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

test(geom): add tests for Rectanglei #23

Closed
wants to merge 1 commit into from
Closed

test(geom): add tests for Rectanglei #23

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
248 changes: 169 additions & 79 deletions joml-geometry/src/test/java/org/terasology/joml/geom/RectangleiTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,97 +2,168 @@
// SPDX-License-Identifier: Apache-2.0
package org.terasology.joml.geom;

import org.joml.Vector2f;
import org.joml.Vector2i;
import org.joml.Vector2ic;
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;

import java.util.stream.Stream;

import static org.junit.jupiter.api.Assertions.assertAll;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.terasology.joml.test.VectorAssert.assertEquals;

/**
* Tests for the {@link Rectanglei} class.
*/
public class RectangleiTest {
@Test
public void testRectangleContainsPoints() {
Rectanglei rect = new Rectanglei(0, 0, 3, 3);

assertTrue(rect.isValid());
assertTrue(rect.containsPoint(new Vector2i(0, 0)));
assertTrue(rect.containsPoint(new Vector2i(1, 1)));
assertFalse(rect.containsPoint(new Vector2i(-1, -1)));
assertFalse(rect.containsPoint(new Vector2i(4, 4)));
class RectangleiTest {
static final Vector2ic ZERO = new Vector2i(0, 0);
static final Vector2ic MIN = new Vector2i(-1, -1);
static final Vector2ic MAX = new Vector2i(1, 1);
static final Rectangleic BASE_RECT = new Rectanglei(MIN, MAX);

static Stream<Arguments> containedRectangles() {
return Stream.of(
Arguments.of(new Rectanglei(BASE_RECT)),
Arguments.of(new Rectanglei(MIN, ZERO)),
Arguments.of(new Rectanglei(ZERO, MAX))
);
}
@Test
public void testRectangleIntersection() {
Rectanglei first = new Rectanglei(0, 0, 3, 3);
Rectanglei second = new Rectanglei(-1, -1, 2, 2);

// is valid
assertTrue(first.isValid());
assertTrue(second.isValid());

assertFalse(first.containsRectangle(second));
assertFalse(second.containsRectangle(first));
/**
* Rectangles overlapping with BASE_RECT and the intersecting area.
*/
static Stream<Arguments> overlappingRectangles() {
return Stream.of(
Arguments.of(
new Rectanglei(MIN.x() - 1, MIN.y() - 1, 0, 0),
new Rectanglei(MIN, ZERO)),
Arguments.of(
new Rectanglei(0, 0, MAX.x() + 1, MAX.y() + 1),
new Rectanglei(ZERO, MAX))
);
}

assertTrue(first.intersectsRectangle(second));
assertTrue(second.intersectsRectangle(first));
assertEquals(first.intersection(second, new Rectanglei()), new Rectanglei(0, 0, 2, 2));
/**
* Rectangles touching one side of BASE_RECT.
*/
static Stream<Arguments> touchingRectangles() {
Rectanglei right = new Rectanglei(MAX.x(), MIN.y(), MAX.x() + 1, MAX.y());
Rectanglei left = new Rectanglei(MIN.x() - 1, MIN.y(), MIN.x(), MAX.y());
Rectanglei top = new Rectanglei(MIN.x(), MAX.y(), MAX.x(), MAX.y() + 1);
Rectanglei bottom = new Rectanglei(MIN.x(), MIN.y() - 1, MAX.x(), MIN.y());
return Stream.of(
Arguments.of(right),
Arguments.of(left),
Arguments.of(top),
Arguments.of(bottom)
);
}

static Stream<Arguments> invalidRectangles() {
return Stream.of(
Arguments.of(new Rectanglei()), // invalid by default
Arguments.of(new Rectanglei(ZERO)), // a "point"
Arguments.of(new Rectanglei(MIN.x(), MIN.y(), MAX.x(), MIN.y())) // a "line"
Copy link

Choose a reason for hiding this comment

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

If the rectangles are inclusive on all sides, a rectangle whose min and max are the same has positive area, and should be valid. I also think that a rectangle with 0 area should be valid too (i.e. in this case, new Rectanglei(0, 0, -1, -1)), although that's a little more subjective.

Copy link
Member Author

Choose a reason for hiding this comment

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

I also think that a rectangle with 0 area should be valid too

This would mean that there are no invalid rectangles, and we could just remove that property altogether, wouldn't it?

Copy link

Choose a reason for hiding this comment

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

No, a rectangle where either of the lengths is strictly less than 0 would still be invalid.

Copy link

Choose a reason for hiding this comment

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

The use of MIN and MAX here is an excessive and unnecessary degree of abstraction.

);
}

@Test
public void testRectangleiEdgeIntersection() {
Rectanglei center = new Rectanglei(0, 0, 1, 1);
Rectanglei right = new Rectanglei(1, 0, 2, 1);
Rectanglei left = new Rectanglei(-1, 0, 0, 1);
Rectanglei top = new Rectanglei(0, 1, 1, 2);
Rectanglei bottom = new Rectanglei(0, -1, 1, 0);

assertTrue(center.isValid());
assertTrue(right.isValid());
assertTrue(left.isValid());
assertTrue(top.isValid());
assertTrue(bottom.isValid());

assertFalse(center.containsRectangle(right));
assertFalse(center.containsRectangle(left));
assertFalse(center.containsRectangle(top));
assertFalse(center.containsRectangle(bottom));

assertTrue(center.intersectsRectangle(right));
assertTrue(center.intersectsRectangle(left));
assertTrue(center.intersectsRectangle(top));
assertTrue(center.intersectsRectangle(bottom));

assertTrue(right.intersectsRectangle(center));
assertTrue(left.intersectsRectangle(center));
assertTrue(top.intersectsRectangle(center));
assertTrue(bottom.intersectsRectangle(center));
void containsPoint() {
Rectanglei rect = new Rectanglei(BASE_RECT);

assertAll(
() -> assertTrue(rect.containsPoint(MIN), "minimum corner is inclusive"),
() -> assertTrue(rect.containsPoint(MAX), "maximum corner is inclusive"),
() -> assertTrue(rect.containsPoint(0, 0)),
() -> assertFalse(rect.containsPoint(-2, -2)),
() -> assertFalse(rect.containsPoint(2, 2))
Copy link

Choose a reason for hiding this comment

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

Including a test that fails on only one axis seems like a good idea.

);
}
@Test
public void testRectangleiContainsPoint() {
Rectanglei center = new Rectanglei(0, 0, 3, 3);

assertTrue(center.isValid());

assertTrue(center.containsPoint(0, 0));
assertTrue(center.containsPoint(1, 0));
assertTrue(center.containsPoint(0, 1));
assertTrue(center.containsPoint(1, 1));
@Nested
class ContainsRectangle {
@Test
void containsRectangle() {
Rectanglei rect = new Rectanglei(BASE_RECT);

assertAll(
() -> assertTrue(rect.containsRectangle(rect),
"contains itself"),
() -> assertTrue(rect.containsRectangle(new Rectanglei(MIN.x(), MIN.y(), 0, 0)),
"minimum corner is inclusive"),
() -> assertTrue(rect.containsRectangle(new Rectanglei(0, 0, MAX.x(), MAX.y())),
"maximum corner is inclusive"),
Copy link

Choose a reason for hiding this comment

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

These would still be true if the corners were exclusive. The tests are correct, it's just the labels that aren't.

() -> assertFalse(rect.containsRectangle(new Rectanglei(MIN.x() - 1, MIN.y() - 1, MAX.x() + 1,
Copy link

Choose a reason for hiding this comment

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

Again, a test that is false by a smaller margin (fewer directions of non-containment) would be a stronger test. There is only 1 obvious implementation for containsRectangle anyway, but if you're going to test it, I think there are better options.

Copy link
Member

Choose a reason for hiding this comment

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

I have some of those test written up in here:

https://github.com/JOML-CI/joml-primitives/blob/main/test/org/joml/primitives/AABBiTest.java

MAX.x() + 1)))
);
}

@ParameterizedTest
@MethodSource("org.terasology.joml.geom.RectangleiTest#invalidRectangles")
void containsRectangleWithInvalid(Rectanglei invalid) {
Rectanglei rect = new Rectanglei(BASE_RECT);
assertFalse(rect.containsRectangle(invalid));
Copy link

Choose a reason for hiding this comment

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

I don't think this function actually should return false. Even if we assume that nobody would ever want to have an invalid rectangle, this method returning false is unlikely to alert them to the issue better than just letting them run into whatever issue the invalid rectangle naturally causes, and this seems like a pretty non-obvious result.

}
}
@Test
public void testRectangleContains() {
Rectanglei first = new Rectanglei(-1, -1, 2, 2);
Rectanglei second = new Rectanglei(0, 0, 1, 1);
assertTrue(first.containsRectangle(second));
assertFalse(second.containsRectangle(first));

assertTrue(first.intersectsRectangle(second));
assertTrue(second.intersectsRectangle(first));

assertEquals(first.intersection(second, new Rectanglei()), new Rectanglei(0, 0, 1, 1));
@Nested
class Intersection {
@Test
void intersectionWithSelf() {
Rectanglei rect = new Rectanglei(BASE_RECT);

assertEquals(rect, rect.intersection(rect, new Rectanglei()),
"intersection with self is identity");
}

@Test
void intersectionWhenContained() {
Rectanglei rect = new Rectanglei(BASE_RECT);
Rectanglei bigger = new Rectanglei(MIN.x() - 1, MIN.y() - 1, MAX.x() + 1, MAX.x() + 1);

assertEquals(rect, rect.intersection(bigger, new Rectanglei()),
"intersection when contained in other yields self");
Copy link

Choose a reason for hiding this comment

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

Maybe test this the other way around too?

}

@Test
void intersectionWithDisjoint() {
Rectanglei rect = new Rectanglei(BASE_RECT);
Rectanglei disjoint = new Rectanglei(7, 7, 42, 42);

assertFalse(rect.intersection(disjoint, new Rectanglei()).isValid());
}

@ParameterizedTest
@MethodSource("org.terasology.joml.geom.RectangleiTest#invalidRectangles")
void intersectionWithInvalid(Rectanglei invalid) {
Rectanglei rect = new Rectanglei(BASE_RECT);
assertFalse(rect.intersection(invalid, new Rectanglei()).isValid());
}

@ParameterizedTest
@MethodSource("org.terasology.joml.geom.RectangleiTest#containedRectangles")
void intersectionWhenContaining(Rectanglei other) {
Rectanglei rect = new Rectanglei(BASE_RECT);

assertEquals(other, rect.intersection(other), "intersection with contained rectangle yields other");
}

@ParameterizedTest
@MethodSource("org.terasology.joml.geom.RectangleiTest#overlappingRectangles")
void intersectionWhenPartialOverlap(Rectanglei other, Rectanglei result) {
Rectanglei rect = new Rectanglei(BASE_RECT);

assertEquals(result, rect.intersection(other), "intersection with contained rectangle yields other");
}

@ParameterizedTest
@MethodSource("org.terasology.joml.geom.RectangleiTest#touchingRectangles")
public void intersectionWhenTouching(Rectanglei touching) {
Rectanglei rect = new Rectanglei(BASE_RECT);
assertFalse(rect.intersection(touching, new Rectanglei()).isValid());
}
}

@Test
Expand All @@ -104,9 +175,28 @@ public void testRectangleSetSize() {
assertEquals(v1, new Rectanglei(-1, -1, 1, 1));
}

@Test
public void testZeroSizeRectangle() {
Rectanglei rect = new Rectanglei(0, 0, 0, 0);
assertFalse(rect.isValid());
@Nested
class RectangleFromPoint {
Copy link

Choose a reason for hiding this comment

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

My earlier point that this rectangle actually should be valid changes all of these tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's basically the point of asking for your review 😉


@Test
public void isInvalid() {
Rectanglei rect = new Rectanglei(ZERO);
assertFalse(rect.isValid());
}

@Test
void containsPoint() {
Vector2i point = new Vector2i(ZERO);
Rectanglei rect = new Rectanglei(point);
assertFalse(rect.containsPoint(point),
"rectangle from point is invalid and does not contain itself");
}

@Test
void intersectionWithSelf() {
Rectanglei rect = new Rectanglei(ZERO);
assertFalse(rect.intersection(rect, new Rectanglei()).isValid(),
"rectangle from point is invalid and does not intersect with itself");
}
}
}