Remove drake/multibody/rigid_body_system1

[jwnimmer-tri]

_Edit: When originally filed, this title read "Remove rigid_body_plant_system1test". Since then, the scope has expanded; see downthread.

Original task: Remove rigid_body_plant_system1_test. Part of #3960. We will purge System 1.0 code, so we can't rely on it for this unit test. I guess we can keep this around until it's the last System 1.0 code remaining, but expect that within a month or so, we'll want to remove it. So if we need a replacement unit test, perhaps time to write it sooner than later.

Discovered programs and tests; we need to consider if we can delete these or not yet:

• testMassSpringDamper - no value; immediately remove
• lidarTest - no value; immediately remove
• testAccelerometer - no value; immediately remove
• testGyroscope - no value; immediately remove
• testMagnetometer - no value; immediately remove
• compareAcrobotURDFtoSDF -- tracked as #5408
• comparePriusURDFtoSDF - irrelevant due to #4074
• load_model_test - no value; immediately remove
• drake_joint_test - no value; immediately remove
• rigid_body_system_test - no value; immediately remove
• rigidBodyLCMNode - no value; immediately remove

[SeanCurtis-TRI]

Good to know; the RigidBodyPlant has relied on it to confirm a form of correctness. The test that currently tests against System1 should be moved and recast to compare against a different authority (probably a hard-coded value.) The test is related to contact but more fully the domain of dynamics. I'd recommend either me or @amcastro-tri as assignees; we can fight among ourselves.

[amcastro-tri]

Probably @mitiguy can provide analytical solutions to test against with?

[SeanCurtis-TRI]

At the very least, we can bake out the current answers that RBS produces and code them up. Not necessarily well principled, but very much preserves the exact functionality of the current test.

[jwnimmer-tri]

Under the hood, the RBS1 implementation is calling the same RBT code as the RBP2 implementation is calling now, so the possibility of divergence seems to be minimal. IIRC, this test code was added to confirm that things like the default value for gravity or something was correctly set in RBP2, or maybe that the order of positions in the output vector are correct or something?

In any case, it's manifestly not a dynamics check, but a basic sanity check on the rough contents of the output. It should be easy to replace, even without oracular benchmark results.

[SeanCurtis-TRI]

And yet, in my current PR this test fails in some builds....grrr...

Generally, this test is brittle anyways. If we change our contact model, the results of this test would change (because the model is currently hard-coded in RBP.) My self-serving voice says, "kill the test".... As part of finding out why it seems to intermittently dying, I may end up submitting a PR removing the System1 dependency.

[jwnimmer-tri]

That would be great! It's epsilon away from becoming the only remaining user of System 1.0.

[jwnimmer-tri]

... and now, it's the only code using System 1.0.

[jwnimmer-tri]

When working through #4553, it became clear that the entire RBS 1.0 needs a sanity sweep for what should be deleted, so I've broadened this title.

[david-german-tri]

FWIW, compareRigidBodySystems.cpp appears to use System1 RigidBodySystem to compare two RigidBodyTrees to each other, by evaluating their dynamics in a bunch of random configurations. It keeps the SDF and URDF versions of an Acrobot model and a Prius model in sync.

I think that's a good candidate for deletion, although it also wouldn't be so hard to port to System2/RigidBodyPlant. Better yet would be to replace it with some kind of equality check on the RigidBodyTree itself if possible...

@liangfok FYI

[jwnimmer-tri]

Yes. Before deleting more files, we should that ensure their test coverage is adequately captured elsewhere. I confirmed this for the stuff I deleted in #4553, but there's definitely more than just the RBP comparison test that this issue was originally filed as (thus the issue rename).

[david-german-tri]

Before deleting more files, we should that ensure their test coverage is adequately captured elsewhere.

Sure, I just think it's not obvious that we need coverage for prius.sdf == prius.urdf. The proposed alternative would be to delete one of those model files (presumably the URDF) as well as the test itself.

[jwnimmer-tri]

Note that #4074 will fix "Remove duplicated Prius model files", so that's one less thing for the System 1.0 regression tests to care about.

[liangfok]

Perhaps the best solution to replacing / removing compareRigidBodySystems.cpp is to implement comparison logic that allows us to execute rigid_body_tree_1.IsApproximatelyEqualTo(rigid_body_tree_2). Having such logic would enable us to ensure certain models remain in sync.

[jwnimmer-tri]

@amcastro-tri @SeanCurtis-tri re-ping. Any new thoughts on fixing this up?

[SeanCurtis-TRI]

I'm working on a PR that changes the contact model in RigidBodyPlant. As such, this test is rendered meaningless because System 1 and the system formerly known as System 2 no longer agree. It'll be killed in that PR.

[liangfok]

I'm going to re-open this issue because drake/multibody/rigid_body_system_1/ still exists in master.

[SeanCurtis-TRI]

Excellent point; in my mind, this issue became conflated with the dreaded rigid_body_plant_system1_test.cc. I hadn't paid enough attention. How much is it worth to us to finally kill this? As has been pointed out, we need to account for all the dependencies. We just have one less now.

[liangfok]

Completely removing drake/multibody/rigid_body_system_1/ is highly desirable but in my opinion low priority since it's not blocking higher priority work. For example, I'm slowly porting all of the sensors over to the new systems framework but only when higher priority tasks are blocked. I shall thus apply the "priority: low" label.

[jwnimmer-tri]

It would probably be worth at least another look soon, to enumerate the remaining work for clarity.

I would love to finally see this code disappear, but I would much rather see RBT disappear. The latter causes huge problems; the former is just a small bother.

[liangfok]

I updated the OP to include a list of items to port. It is likely incomplete -- I just put the things I'm aware of.

[liangfok]

I crossed out the porting of the parsers in the OP due to https://github.com/RobotLocomotion/drake/issues/4337.

[jwnimmer-tri]

The recent top-post description editing missed the point. We don't need to itemize System 1.0 features that haven't been ported yet. We need to itemize the impediments to removing System 1.0 from master, which is really just the System 1.0 tests that are not ported yet but are still relevant to System 2.0. I will endeavor to update with that new list.

[jwnimmer-tri]

The only possible test with remaining value is compareRigidBodySystems Acrobot.urdf Acrobot.sdf, which confirms that the two model files are in sync. I think tracking that with an issue (#5408) will be low enough risk to delete the code now, and then reimplement the test once we think we need it.