Make margin required

[amcastro-tri]

Right now DefaultProximityProperties::margin can be std::nullopt, which essentially means the default margin is zero.
Therefore the use of std::optional for margin is overkill and it should simply be a plain double with default value of zero.
Then ProximityProperties will always have margin defined when reifying geometry.

With this change we can also resolve the TODO's in this Reviewable comment.

[SeanCurtis-TRI]

The core functionality expressed in this issue is that if nothing is said, a margin of zero should be used. That's the behavior implied by the statement.

From an implementation perspective, there are multiple ways to achieve this.

1. What is proposed above. That has the following implications:
   • Every proximity property (whether it has a hydro representation or not) picks up the ("hydroelastic", "margin") property.
   • Call sites would all assume that property's existence and error out on its absence.
   • The default value is clearly defined in the documentation of ProximityPropertyDefaults.
   • Simplifies the property hierarchy protocol. In other words, consumers of ProximityProperties will never have the need nor the ability to choose an arbitrary margin. It will always be defined.
2. Keep the code as it is, with the following implications:
   • The default value is implied by the absence of the property and only used where necessary.
   • Harder to keep the documentation of the default value and hard-coded default value in sync.
   • The call sites (limited to ProximityEngine) would own the default value.

(2) is what is in master now.

Reasons to prefer leaving master alone (i.e., leave (2) implemented):
- Marginally cheaper context allocation.

Reasons to prefer (1):
- Well documented default value.
- Aforementioned simplification of default value hierarchy.

The behaviors of (1) and (2) are basically identical but with one subtle distinction. In my mind, it all comes down to the question of "default" behavior. If we want the default value simple and unambiguous, we go with (1). If we want to maximize flexibility (in the name of admittedly strictly-speculative future functionality) we go with (2). The flexibility makes most sense for multiple consumers (e.g., PerceptionProperties). But we currently only have one consumer of ProximityProperties and anticipate things to remain so indefinitely.

The result of these consideration is that I second the motion that we go with (1).

@rpoyner-tri As the author of SceneGraphConfig you should probably weigh in. @jwnimmer-tri as an interested party in the creation of SceneGraphConfig, you should probably weigh in as well.

[jwnimmer-tri]

Therefore the use of std::optional for margin is overkill ...

Meh. If it ain't broken, don't fix it. I don't think an aesthetic judgement ("overkill") is a valid reason to make a breaking change.

Zap the TODO and call it a day.

The SceneGraph<T>::Hub is the single responsible party for assigning defaults. The two TODOs in question are wrong. If the Hub didn't set a margin, then there is no margin (i.e., it's effectively zero). The hard-coded 0.0 at those locations is correct and never need change.

[amcastro-tri]

I see your argument @jwnimmer-tri. In my mind, probably I was extrapolating my thoughts on other properties, see if you agree.

For instance, consider point_stiffness, which right now has a non-zero default of 1e6 N/m. I thinnk for this case, we'd like to zap the std::optional from DefaultProximityProperties so that there is no chance for anyone to hardcode a difficult to mantain non-zero default value. Would you agree?

Now, for margin I do see your point. Being zero, i.e. there is no margin, I think we could go both ways with either solution.

[jwnimmer-tri]

For instance, consider point_stiffness, which right now has a non-zero default of 1e6 N/m. I think for this case, we'd like to zap the std::optional from DefaultProximityProperties so that there is no chance for anyone to hardcode a difficult to maintain non-zero default value. Would you agree?

No. It's very easy to have the Hub notice the nulllopt and set a value anyway. If the hub wants to set 1e6 when the user says nullopt, it already has a constant it can use for that -- DefaultProximityProperties{}.point_stiffness.value().

[amcastro-tri]

okay, I like that. Do you think we should do that with margin (and other parameters) as well? (in a future PR).

[jwnimmer-tri]

The question is what we want to happen when the user says null. I don't see any reason to do it with margin. For others, I guess it depends on what we want to happen, but in any case this issue doesn't contain enough background or call to action to facilitate a decision.

[amcastro-tri]

It seems like we are converging to Sean's option 2. In #/22153 we use a hard-coded 0.0 and removed the TODO's.
If you both agree, I'll close this.