Skip to content

Commit

Permalink
Move unit tests of boxes_overlap out of obb_test (#21546)
Browse files Browse the repository at this point in the history
The code in boxes_overlap.cc was indirectly tested in obb_test.cc.
Now boxes_overlap_test.cc takes the responsibility for testing
the code directly (and obb_test.cc has become that much simpler).

This also includes a tweak to MaybePauseForUser() to include an
optional prompt message. The prompt message only gets printed if
the pause is enabled.
  • Loading branch information
SeanCurtis-TRI authored Jun 13, 2024
1 parent 5361645 commit 66be6c1
Show file tree
Hide file tree
Showing 8 changed files with 332 additions and 235 deletions.
5 changes: 4 additions & 1 deletion common/test_utilities/maybe_pause_for_user.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
namespace drake {
namespace common {

void MaybePauseForUser() {
void MaybePauseForUser(std::string_view message) {
bool is_test = (std::getenv("TEST_TMPDIR") != nullptr);
bool is_invoked_by_bazel_run =
(std::getenv("BUILD_WORKSPACE_DIRECTORY") != nullptr);
Expand All @@ -16,6 +16,9 @@ void MaybePauseForUser() {
// program will hang, failing to notice user keyboard input.
return;
}
if (!message.empty()) {
std::cout << message << std::endl;
}
std::cout << "[Press RETURN to continue]." << std::endl;
std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
}
Expand Down
7 changes: 6 additions & 1 deletion common/test_utilities/maybe_pause_for_user.h
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
#pragma once

#include <string>

namespace drake {
namespace common {

Expand All @@ -8,6 +10,9 @@ namespace common {
as a bazel test, but when running as a command-line executable, it will enable
users to see the visualization outputs.
The caller can provide additional user instructions in `message`. The message
will only be written to the console if the pause is actually enabled.
The complete behavior is somewhat more subtle, depending on the bazel rules
used to build the program, and the way it is invoked:
Expand All @@ -19,7 +24,7 @@ namespace common {
- invoked with "bazel run" -- does pause
- invoked directly ("bazel/bin/[PROGRAM]") -- does pause
*/
void MaybePauseForUser();
void MaybePauseForUser(std::string_view message = {});

} // namespace common
} // namespace drake
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
#include "drake/common/test_utilities/maybe_pause_for_user.h"

// Manual test users should verify that:
// - the prompt string is shown
// - the generic prompt string is shown
// - a user-typed RETURN is consumed and the program exits
// - A second prompt string is shown, this time with custom elaboration.
// These results should work for both direct ("bazel-bin/blah") invocation and
// for invocation with "bazel run".

Expand All @@ -12,6 +13,7 @@ namespace {

int do_main() {
MaybePauseForUser();
MaybePauseForUser("Please ignore this test prompt.");
return 0;
}

Expand Down
1 change: 1 addition & 0 deletions common/test_utilities/test/maybe_pause_for_user_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ namespace {

GTEST_TEST(MaybePauseForUserTest, Test) {
MaybePauseForUser();
MaybePauseForUser("With message");
}

} // namespace
Expand Down
25 changes: 23 additions & 2 deletions geometry/proximity/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ drake_cc_package_library(
name = "proximity",
visibility = ["//visibility:public"],
deps = [
":boxes_overlap",
":bv",
":bvh",
":bvh_updater",
Expand Down Expand Up @@ -66,19 +67,28 @@ drake_cc_package_library(
],
)

drake_cc_library(
name = "boxes_overlap",
srcs = ["boxes_overlap.cc"],
hdrs = ["boxes_overlap.h"],
deps = [
"//common:essential",
"//math:geometric_transform",
],
)

drake_cc_library(
name = "bv",
srcs = [
"aabb.cc",
"boxes_overlap.cc",
"obb.cc",
],
hdrs = [
"aabb.h",
"boxes_overlap.h",
"obb.h",
],
deps = [
":boxes_overlap",
":posed_half_space",
":triangle_surface_mesh",
":volume_mesh",
Expand Down Expand Up @@ -958,6 +968,17 @@ drake_cc_googletest(
],
)

drake_cc_googletest(
name = "boxes_overlap_test",
deps = [
":boxes_overlap",
"//common/test_utilities:maybe_pause_for_user",
"//geometry:meshcat",
"//geometry:rgba",
"//geometry:shape_specification",
],
)

drake_cc_googletest(
name = "bvh_test",
deps = [
Expand Down
3 changes: 0 additions & 3 deletions geometry/proximity/boxes_overlap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,6 @@ using Eigen::Matrix3d;
using Eigen::Vector3d;
using math::RigidTransformd;

// TODO(SeanCurtis-TRI) This code is tested in obb_test.cc for historical
// reasons. If that causes confusion/difficulty, move it into its own unit test
// and rework the Obb tests.
bool BoxesOverlap(const Vector3d& half_size_a, const Vector3d& half_size_b,
const RigidTransformd& X_AB) {
// We need to split the transform into the position and rotation components,
Expand Down
Loading

0 comments on commit 66be6c1

Please sign in to comment.