Simulator agnostic agents #481

stonier · 2018-06-22T16:48:29Z

In part, for #474.

Broadly:

No more Configure
When writing an agent you no longer need to be aware of the simulator bits and pieces

Things to look at specifically:

Header library for agent developers - delphyne/mi6 (couldn't call it 'agents', but could not come up with anything better in a short time frame). agent_base.h has been moved here.
New agent_diagram_builder.h and diagram_bundle.h to compartmentalise what an agent diagram should look like

                         -----------
                        |   Agent   | --> State
      Traffic Poses --> |  Diagram  | --> Pose
                        |           | --> Velocity
                         -----------

No more id in AgentBase (unused right now)
Agent helpers moved to the backend since it's entirely contained therein right now

Did not take into consideration:

Agents being anything other than vehicles and vehicles being anything other than Prius cars. We'll have to deal with this eventually, but that day is not today.

Things to follow on with (punted to the next PR - this PR achieves the initial objective and is already too big):

Currently initial_world_pose is being computed at construction time, would probably be better to have that as the default value sitting on the pose output.
Work out if we should have any simulator specific code in AgentBase (there are some things like geometry_ids, is_source_of which the author of an agent does not need to be aware of, but is useful for the simulator to have wrapped up in the AgentBase container.

stonier · 2018-06-22T20:23:00Z

Ready for human consumption

clalancette

This large PR makes sense to me, and local testing showed things working well. All of my comments are fairly minor changes; once those are done, I'm happy with this. Note that I pushed two commits of my own that did header cleanup and some small comment cleanups.

clalancette · 2018-06-25T12:28:19Z

include/delphyne/CMakeLists.txt

@@ -50,3 +49,4 @@ install(TARGETS public_headers EXPORT ${PROJECT_NAME}-targets)
 ##############################################################################

 add_subdirectory(maliput)
+add_subdirectory(mi6)


I like it :).

clalancette · 2018-06-25T12:32:22Z

include/delphyne/mi6/agent_base.h

+/// This is the abstract class that all delphyne agents must inherit from.
+/// Concrete implementations must implement both the 'Configure' method and the
+/// 'Initialize' method; see the documentation for those methods for more
+/// information.


The above comment is now woefully out-of-date, since both Configure and Initialize are gone. Probably revamp it.

clalancette · 2018-06-25T12:40:28Z

include/delphyne/mi6/agent_base.h

+class AgentBase {
+ public:
+  using DiagramBundle = delphyne::DiagramBundle<T>;
+  using DiagramBuilder = AgentDiagramBuilder<T>;


This DiagramBuilder isn't used anywhere in this header file. Typically I'd remove it and push it down to the pieces that need it (in the individual agents), but since this is a base class you may have intended for this to be here. Thoughts?

Trying to keep client agent code simple. In this case, saving them from having to worry about templates and finding the tools they are supposed to use. This puts the toolbox directly in the class itself. Can't miss it.

This is not so uncommon, for example, it underpins the way traits are usually setup.

Added some documentation for them.

clalancette · 2018-06-25T12:40:56Z

include/delphyne/mi6/agent_diagram_builder.h

@@ -0,0 +1,127 @@
+// Copyright 2017 Toyota Research Institute


Sheesh, that went fast.

clalancette · 2018-06-25T12:44:43Z

include/delphyne/mi6/diagram_bundle.h

@@ -0,0 +1,52 @@
+// Copyright 2017 Toyota Research Institute


clalancette · 2018-06-25T12:48:47Z

python/delphyne/utilities.py

    """Adds a lane changing (MOBIL) car to the simulation."""
    agent = agents.MobilCar(
        name=name,                 # unique name
        direction_of_travel=True,  # with or against the lane s-direction
        x=scene_x,                 # scene x-coordinate (m)
        y=scene_y,                 # scene y-coordinate (m)
        heading=heading,           # heading (radians)
-        speed=speed)               # the s-direction (m/s)
+        speed=speed,               # the s-direction (m/s)
+        road_geometry=road_geometry)


It's kind of obvious, but for consistency, add a comment at the end of the line?

clalancette · 2018-06-25T12:48:52Z

python/delphyne/utilities.py

@@ -136,7 +138,8 @@ def add_rail_car(simulator, name, lane, position, offset, speed):
        longitudinal_position=position,  # lane s-coordinate (m)
        lateral_offset=offset,           # lane r-coordinate (m)
        speed=speed,                     # initial speed in s-direction (m/s)
-        nominal_speed=5.0)               # nominal_speed (m/s)
+        nominal_speed=5.0,               # nominal_speed (m/s)
+        road_geometry=road_geometry)


clalancette · 2018-06-25T12:53:27Z

src/agents/rail_car.cc

      std::make_unique<RailCarSystem>(lane_direction, context_continuous_state,
                                      context_numeric_parameters));
-  rail_car_system->set_name(name_);
+  rail_car_system->set_name(name_ + "_system");


I'm usually wary of adding to names like this, as it can be surprising to the user. What if they added a RailCar named "foo", then added a SimpleCar named "foo_system"? From their perspective they did nothing wrong, but they will get an error about duplicate names (I think). I'd stick with just "name_" here, and do this at the application level.

Ah, not immediately obvious I guess. Earlier in BuildDiagram() there is:

DiagramBuilder builder(name_);

which steals name_ for the Diagram name. Since that too is a system, which gets added to AutomotiveSimulator's diagram, it has to be unique.

clalancette · 2018-06-25T12:58:45Z

src/backend/automotive_simulator.cc

+  /*********************
+   * Agent Visuals
+   *********************/
+  // TODO(daniel.stonier) this just enforces ... 'everything is a prius'.


This is the reason that I originally pushed this down into the individual agents; it seemed to me that they should be responsible for their own visuals. In that sense, I think this is a regression, but I'm OK with it as long as we have a follow up task to fix this again.

I fully agree with @clalancette. That's actually why I added the collision geometries where I did. But I must say that I do see the major code simplification and de-duplication this new approach brings. Maybe we can find a middle ground in the next PR e.g. decorating agents.

Posting thoughts in the main thread.

clalancette · 2018-06-25T13:01:17Z

src/agents/simple_car.cc

  simple_car_system->set_name(name_);

  /*********************
   * Teleop Systems
   *********************/
-  std::string command_channel = "teleop/" + std::to_string(id);
+  std::string command_channel = "teleop/" + this->name();


Fantastic :).

hidmic

@stonier I left a couple minor comments. PTAL.

hidmic · 2018-06-25T13:39:55Z

include/delphyne/mi6/agent_base.h

+ private:
+  // TODO(daniel.stonier) dubious whether we should have
+  // simulator specific machinery here
+  std::set<drake::geometry::GeometryId> geometry_ids_{};


@stonier FYI when I first added this, the rationale was to keep collision geometries an agent implementation detail.

hidmic · 2018-06-25T13:40:46Z

include/delphyne/mi6/agent_base.h

+  }
+
+  /// @brief Mutable accessor to the geometry ids
+  std::set<drake::geometry::GeometryId>& mutable_geometry_ids() {


@stonier it is a standard practice in Drake to use pointers instead of mutable references.

I'd like to kick back on that. I really, really, really don't want to be coerced into thinking I must check for null every time I call this method. Perhaps I'm missing some logic for why they do this?

Well, I can't do mind control :). But if you're worried about pointer integrity, I'd perform the necessary checks within the accessor if it applies (just like you'd do before returning a reference) and then all user code needs not worry about it (as long as the documentation clearly states that you don't have to and a nullptr won't ever happen).

Having said that, I've seen mutable references being returned in some parts of Drake. So, in any case, I would advocate for consistency.

It's not in the style guide and I don't think we should introduce an arbitrary relaxation of the function contract just to follow some pattern that may have merely become habit without good reason elsewhere. Happy to be swayed if there is a good reason.

Having said that, not so happy with using mutable values to set a variable here. Would rather make use of a setter since this is not an expensive, high frequency operation, but it wasn't written that way originally and would likely be revisited when working out how to incorporate geometries more appropriately in agents.

hidmic · 2018-06-25T13:56:43Z

test/regression/cpp/automotive_simulator_test.cc

-  std::unique_ptr<const drake::maliput::dragway::RoadGeometry> dragway =
-      CreateDragway("TestDragway", 1);
+
+  auto dragway =


@stonier I'd be explicit about the type.

hidmic · 2018-06-25T14:01:54Z

src/backend/automotive_simulator.cc

+  /*********************
+   * Agent Visuals
+   *********************/
+  // TODO(daniel.stonier) this just enforces ... 'everything is a prius'.


I fully agree with @clalancette. That's actually why I added the collision geometries where I did. But I must say that I do see the major code simplification and de-duplication this new approach brings. Maybe we can find a middle ground in the next PR e.g. decorating agents.

hidmic · 2018-06-25T14:10:59Z

include/delphyne/mi6/agent_diagram_builder.h

+  /// @brief Export the specified velocity output port.
+  void ExportVelocityOutput(const drake::systems::OutputPort<T>& output) {
+    outputs_["velocity"] = this->ExportOutput(output);
+  }


@stonier consider asserting that exporting any one of the output port types more than once is a logic error (and updating documentation and tests accordingly).

Think I'm missing the point of this comment, care to unwind for me?

That's because I had a brainfart and wrote half what I wanted to :). Review comment edited.

TL;DR assert in each export method that it is called once and only once.

Good catch. Done in e5c77a6

hidmic · 2018-06-25T14:40:17Z

include/delphyne/mi6/agent_diagram_builder.h

+    bundle.diagram->set_name(name_);
+    bundle.outputs = outputs_;
+    bundle.inputs = inputs_;
+    return bundle;


@stonier FYI it seems this works because of copy elision. I wonder if it wouldn't be better to stay consistent with Drake and return a std::unique_ptr<DiagramBundle<T>> instance instead (and completely deleting copy and move constructors of the bundle).

Ah, if elision is not happening, then DiagramBundle breaks because it is trying to copy a unique pointer?

Updated in e68b77b.

Interesting tidbit, the c++ standard doesn't define a move constructor if you have defined (or deleted) the copy constructor.

9 If the definition of a class X does not explicitly declare a move constructor, one will be implicitly declared as defaulted if and only if — X does not have a user-declared copy constructor, 123) This implies that the reference parameter of the implicitly-declared copy constructor cannot bind to a volatile lvalue; see C.1.9. § 12.8 269 c ISO/IEC N3690 — X does not have a user-declared copy assignment operator, — X does not have a user-declared move assignment operator, and — X does not have a user-declared destructor.

hidmic · 2018-06-25T17:57:31Z

Hmm, @stonier isn't this PR effectively removing all agents' entity? I just realized that, with this implementation, "agents" no longer directly relate to an entity within the simulation (e.g. a car) but become more like "builders" instead. No reference to the built diagram is kept, and actually, there's nothing in the type system that prevents one from building multiple diagrams out of a single "agent". How does that play with simulation introspection? Up until now, I thought that queries e.g. position or velocity would eventually go through agents. Specially those coming from Python.

stonier · 2018-06-26T04:08:02Z

@hidmic All good points. For the most part, there was so much to do here, that I tried to keep this PR focused on keeping the simulator out of the agent and keeping the agent code as minimal as possible. This keeps it simple for the agent developer, and we'll probably reap the benefits of a clean abstraction layer later.

As for the rest, agree on all fronts, though the way forward is still not entirely clear to me yet. I'd like to get both of your opinions on design laid out - and despite wanting to delay it, maybe sooner rather than later is better.

Thought bullets:

As a builder only? Yes, like that now, but will change as soon as @clalancette starts using an instance (a pointer to?) of this class from the python layer to modify the agent's behaviour from scriptlets (python callbacks) in the runtime (so yes, queries e.g. position or velocity would eventually go through agents)
The above means the python layer needs to get some handle to the instance when AddAgent is called. @clalancette will try to make this as pythonic as possible.
The above also probably requires a pointer to the diagram needs to be kept around. I think a bundle.diagram.get() at the end of BuildDiagram() will suffice.
The simulator can't actually instantiate two of the same agent right now (name uniqueness), so ... indirectly, no need to protect BuildDiagram() from being built twice. Probably some checking/management of this comes in if we can both add, remove or event re-build agents at some point in time.
Geometries and Visuals, should absolutely be in the agents at some point.

This is where the path forward is not yet crystal clear. An agent could probably be neatly modularised into Perception, Planner, Controller, Geometry, Visuals. Perception, planner and Controller could probably be reasonably bundled together (Brain, Geometry & Visuals?), but geometry and visuals should be separate to allow for combinatorial options that will minimise the number of agents that we need to create. This is in scope for M3.

Another interesting path forward would be one where we could swap the controller out on the fly mid-scenario. e.g. going from TrajectoryFollower -> RailFollower. This is out of scope for M3 though.

I'd also like to keep a focus on minimising the complexity required to build an agent (or one of its constituent parts). This means shifting complexity out to helper methods/classes in mi6, to other methods in AgentBase (bit ugly) or to the simulator itself whenever possible.

Next step: design doc we can hack together on?

stonier · 2018-06-26T04:31:21Z

@clalancette what do you use for include checking/pruning?

hidmic · 2018-06-26T20:33:02Z

Perfect. Thanks for the clarification @stonier !

stonier · 2018-06-28T04:43:57Z

bump

clalancette · 2018-06-28T12:55:55Z

This is where the path forward is not yet crystal clear. An agent could probably be neatly modularised into Perception, Planner, Controller, Geometry, Visuals. Perception, planner and Controller could probably be reasonably bundled together (Brain, Geometry & Visuals?), but geometry and visuals should be separate to allow for combinatorial options that will minimise the number of agents that we need to create. This is in scope for M3.

I like the idea of having different modules that can be combined to make an agent, along the lines you outlined above. However, that also seems to me to be a different step. That is, we can easily combine all of the above into "monolithic" agents for the time being, and then split those out later as it makes sense (this is along the lines of the refactoring you did to make agents has-a instead of is-a). From that perspective, I think it makes sense to keep things like the visuals and other agent-specific details down in the agents, and work on refactoring that as a separate item.

Another interesting path forward would be one where we could swap the controller out on the fly mid-scenario. e.g. going from TrajectoryFollower -> RailFollower. This is out of scope for M3 though.

Off the top of my head I'm not exactly sure how we would do this, but since it is out of scope I won't worry about it too much right now :).

I'd also like to keep a focus on minimising the complexity required to build an agent (or one of its constituent parts). This means shifting complexity out to helper methods/classes in mi6, to other methods in AgentBase (bit ugly) or to the simulator itself whenever possible.

I agree that it should be simpler to write an agent, but there is inevitable detail in doing so. I think having the "gadgets" as you outlined above should help with that, without impacting the layering between the simulator infrastructure and the agents themselves.

Next step: design doc we can hack together on?

Yeah, I'll take a look at the document you started.

@clalancette what do you use for include checking/pruning?

I generally do it by hand. I don't know of a tool, but would be happy to be pointed to one.

stonier · 2018-06-28T14:02:08Z

@clalancette I plan to work on handling configurable geometry and visuals immediately in the following PR's as we'll need the variation for pedestrians (not everything is a prius). I can't do that so easily here without either dragging the simulator in, or establishing new diagram interfaces to be flipped over, or stuffing it into the current diagram, which I'd rather not do since I'm trying to establish that as the modular boundary between SPC blocks and the rest of the system. Geometry and Visuals will have to be elsewhere since they are orthogonally configurable, that elsewhere right now is easiest in the simulator.

In short, we're aiming for the same thing, the path to get there is not a critical/showstopping problem and this is the path that I've identified as the least effort to get there. Are we bikeshedding?

clalancette · 2018-06-28T15:07:47Z

@clalancette I plan to work on handling configurable geometry and visuals immediately in the following PR's as we'll need the variation for pedestrians (not everything is a prius). I can't do that so easily here without either dragging the simulator in, or establishing new diagram interfaces to be flipped over, or stuffing it into the current diagram, which I'd rather not do since I'm trying to establish that as the modular boundary between SPC blocks and the rest of the system. Geometry and Visuals will have to be elsewhere since they are orthogonally configurable, that elsewhere right now is easiest in the simulator.

I understand the problem now.

In short, we're aiming for the same thing, the path to get there is not a critical/showstopping problem and this is the path that I've identified as the least effort to get there. Are we bikeshedding?

I don't particularly think so; this is an important design decision.

I guess there are two boundaries we want:

Keep the simulator out of the agents.
Have the agents configure everything about themselves.

The code in master has the second, at the expense of the first. This PR has the first, at the expense of the second. Personally I would have kept the existing second boundary and incrementally worked towards the first, but since we already have this code doing the first one, I'm fine with going forward with this.

With that in mind, I'll do another round of review here.

clalancette

One more question, then it looks like it needs to be rebased, then I'll approve it.

clalancette · 2018-06-28T15:33:47Z

src/agents/mobil_car.cc

-      std::make_unique<drake::automotive::PriusVis<double>>(id, name_));
-
-  // Computes the initial world to car transform X_WC0.
-  const drake::Isometry3<double> X_WC0 =


I see this Isometry3 is being removed here (and in the other agents), but not added back anywhere else. I'm not particularly familiar with what this was being used for; is this just no longer needed?

It was moved to the constructor.

D'oh, of course. For some reason it didn't come up in my search before. OK, thanks!

stonier added the type: enhancement label Jun 22, 2018

stonier added this to the Milestone #3 milestone Jun 22, 2018

stonier requested review from clalancette, basicNew and hidmic June 22, 2018 16:48

stonier force-pushed the stonier/agent_diagrams branch 2 times, most recently from 0896716 to f1a89e1 Compare June 22, 2018 20:04

stonier mentioned this pull request Jun 23, 2018

Helper libraries for agent development #477

Closed

clalancette reviewed Jun 25, 2018

View reviewed changes

hidmic reviewed Jun 25, 2018

View reviewed changes

hidmic mentioned this pull request Jun 25, 2018

Adds 'on collision' callback support #483

Merged

stonier force-pushed the stonier/agent_diagrams branch from 77d33ff to 6b7ac2c Compare June 26, 2018 03:10

stonier force-pushed the stonier/agent_diagrams branch from b8d1ba3 to e68b77b Compare June 26, 2018 04:45

stonier mentioned this pull request Jun 27, 2018

Daniel's Agent Evolution ScratchPad #438

Closed

14 tasks

stonier force-pushed the stonier/agent_diagrams branch from e5c77a6 to b5d9ad2 Compare June 27, 2018 18:25

clalancette reviewed Jun 28, 2018

View reviewed changes

clalancette approved these changes Jun 28, 2018

View reviewed changes

stonier force-pushed the stonier/agent_diagrams branch 2 times, most recently from dfb5174 to 2c73e95 Compare July 2, 2018 04:47

[agents] diagram building and simulator agnostic

7c554bf

stonier force-pushed the stonier/agent_diagrams branch from 2c73e95 to 7c554bf Compare July 2, 2018 04:59

stonier merged commit a1864dd into master Jul 2, 2018

stonier deleted the stonier/agent_diagrams branch July 2, 2018 05:12

stonier mentioned this pull request Jul 2, 2018

Add a SimpleCarStatePublisher system that feeds from the PoseAggregator #485

Closed

apojomovsky pushed a commit that referenced this pull request Jul 10, 2018

[agents] diagram building and simulator agnostic (#481)

bce1156

This was referenced Sep 18, 2018

Agent Bookkeeping #459

Closed

Architectural Improvements & Bugfixes #453

Closed

		@@ -0,0 +1,127 @@
		// Copyright 2017 Toyota Research Institute

Simulator agnostic agents #481

Simulator agnostic agents #481

Conversation

stonier commented Jun 22, 2018

stonier commented Jun 22, 2018

clalancette left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stonier Jun 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stonier Jun 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stonier Jun 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic Jun 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic commented Jun 25, 2018

stonier commented Jun 26, 2018

stonier commented Jun 26, 2018

hidmic commented Jun 26, 2018

stonier commented Jun 28, 2018

clalancette commented Jun 28, 2018

stonier commented Jun 28, 2018

clalancette commented Jun 28, 2018

clalancette left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stonier Jun 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stonier Jun 26, 2018 •

edited

Loading

stonier Jun 26, 2018 •

edited

Loading

stonier Jun 26, 2018 •

edited

Loading

hidmic Jun 25, 2018 •

edited

Loading

stonier Jun 28, 2018 •

edited

Loading