IntegratorBase::StepResult seems superfluous

[david-german-tri]

StepResult is the enum return type from IntegratorBase<T>::StepOnceAtMost to the Simulator<T>::StepTo loop. The return value is computed as follows:

• The dt constraints (next publish time, next update time, and max step size) are sorted by floating-point comparison.
• The smallest is passed to DoStepOnceAtMost as a hard limit on step size.
• If the integrator declines to step that far, we return kTimeHasAdvanced.
• If the integrator steps the full dt, we return kReachedPublishTime, kReachedUpdateTime, or kTimeHasAdvanced depending on which constraint was active.

(#4331 will make the logic a little more complex by "stretching" the max step size, but that's not germane to this issue.)

Then, StepTo uses the return type to determine whether to invoke the publish or update handlers on the system being simulated.

I believe we could stop returning the enum, and simply check in StepTo whether a publish or update time has passed:

publish_hit = (context_->get_time() >= next_publish_time);
update_hit = (context_->get_time() >= next_update_time);

Reasons I believe this enum-less approach is equally correct:

1. It does not change the actual step size taken, under any circumstances.
2. In particular, it does not affect the guarantee that StepOnceAtMost will never step past the next discrete event time.
3. Systems under simulation compute the next discrete event time based on context_->get_time(), so this phrasing makes very clear the Simulator is honoring that request.
4. StepTo already does a comparison with get_time() to cope with simultaneous discrete events. If it's legitimate there it's legitimate more generally.

(3) in particular is noteworthy, because a direct check of get_time() would have prevented a Simulator bug that skipped System-requested discrete updates in #4325. Admittedly, it would have done so at the cost of forcing some epsilon-sized integrator steps, so the #4331 fix is both sufficient and superior - but I claim we can have a belt and suspenders, and delete some lines of code to boot!

The dynamics team assures me that actually these enums are vital, but it takes days of painstaking reasoning by a team of Ph.Ds to understand why. They regret not fully capturing this reasoning in comments for the benefit of hapless readers like me. So, I am opening an issue as requested by @SeanCurtis-TRI and @edrumwri, requesting documentation and/or cleanup.

[sherm1]

The fundamental motivation for the discrete return values is to avoid making critical simulation decisions based on unreliable floating point math. This will get much worse with witness functions included since the theory requires a "zero crossing" which must be isolated and in fact never actually gets to zero. The goal is to isolate the floating point nastiness to as small a piece of code as possible, make a discrete decision and then stick with it after that.

The requisite educational requirement is not a PhD -- it's a diploma from the School of Hard Knocks for numerical computation!

[david-german-tri]

Yes - my point in brief is that we're making decisions based on floating point comparison regardless in LeafSystem<T>::DoCalcNextUpdateTimeImpl (self-explanatory) and IntegratorBase<T>::StepOnceAtMost (the sort).

[edrumwri]

Closing this issue as it seems to no longer have an advocate. Feel free to reopen if desired.