Skip to content

Proximity numeric limits - #24944

Open
castor639 wants to merge 3 commits into
RobotLocomotion:masterfrom
castor639:proximity-numeric-limits
Open

Proximity numeric limits#24944
castor639 wants to merge 3 commits into
RobotLocomotion:masterfrom
castor639:proximity-numeric-limits

Conversation

@castor639

@castor639 castor639 commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

This change is Reviewable

Document valid ranges (including NaN/∞) on DefaultProximityProperties
fields and share ThrowIfInvalid* helpers between scene_graph_config and
proximity_properties (also used for margin in proximity_engine).
@SeanCurtis-TRI

Copy link
Copy Markdown
Contributor

@drake-jenkins-bot test this please

@SeanCurtis-TRI

Copy link
Copy Markdown
Contributor

+a:@SeanCurtis-TRI for feature review

@SeanCurtis-TRI SeanCurtis-TRI self-assigned this Sep 1, 2026
@SeanCurtis-TRI

Copy link
Copy Markdown
Contributor

@drake-jenkins-bot retest this please

1 similar comment
@SeanCurtis-TRI

Copy link
Copy Markdown
Contributor

@drake-jenkins-bot retest this please

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@SeanCurtis-TRI reviewed 8 files and all commit messages, and made 3 comments.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on castor639).


a discussion (no related file):
This adds a number of throwing methods. The fact that they throw leads to some changes that subvert the styleguide (catching and rethrowing).

One alternative:

  1. Instead of, e.g., ThrowIfInvalidFoo, simply implement IsValidFoo() that returns a bool.
  2. In places where you are currently calling ThrowIfInvalidFoo(), instead call: DRAKE_DEMAND(IsValidFoo(x), x).

The downside of that proposal is that it's not clear what "validity" means. In the code today, we tend to enumerate the requirements.

Second alternative:

  1. Define methods that are clear in their requirements, IsPositiveFinite(), IsNonNegativeFinite(), IsPositive().
  2. Invoke them as: DRAKE_DEMAND(IsPositiveFinite(x), x);. Assuming that x is an appropriately named variable (like margin, slab_thickness, etc.), then the error message will report the named quantity, the requirements on it, and the current value.

geometry/scene_graph_config.h line 60 at r1 (raw file):

  When present, the value must satisfy `0 < resolution_hint < ∞` (finite and
  positive). NaN and ±∞ are not allowed.

nit: Not clear if this elaboration is helpful. It is already implicity excluded from the requirement 0 < resolution_hint < infinity.

Similar to the documentation modifications on the other fields.

Code quote:

NaN and ±∞ are not allowed.

geometry/scene_graph_config.cc line 24 at r1 (raw file):

  try {
    validate(*property);
  } catch (const std::exception& e) {

nit; This is against style guide; we should not be using exceptions to communicate.

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.

2 participants