Python bound agents Part II #425
Conversation
stonier
force-pushed
the
stonier/other_cars
branch
4 times, most recently
from
30ea4b6
to
cd824b0
Compare
stonier
force-pushed
the
stonier/other_cars
branch
from
60a87b4
to
7ec7dda
Compare
|
Adding @hidmic for style review |
backend/automotive_simulator.h
Outdated
| * ready this agent for use in the simulation. | ||
| * | ||
| * @param agent | ||
| * @return |
There was a problem hiding this comment.
@stonier documentation is missing a few bits.
| /** | ||
| * @brief Adds an agent to the simulation. | ||
| * | ||
| * The user should have custom constructed this agent from |
There was a problem hiding this comment.
@stonier consider using Doxygen's @details command for this.
There was a problem hiding this comment.
Not needed if you have the blank line (as per the doxygen documentation). Do you have a persuasive argument for wanting to do so?
There was a problem hiding this comment.
Hmm no argument, I guess I'm just used to it. Dismiss it.
backend/automotive_simulator.h
Outdated
| @@ -249,8 +214,7 @@ class AutomotiveSimulator { | |||
| // with static id counters since they may run into threading problems) | |||
| int unique_system_id_{0}; | |||
|
|
|||
| // Maps an agent id to a pointer to the system that implements the agent. | |||
| std::map<int, std::unique_ptr<delphyne::AgentPluginBase<T>>> loadable_agents_; | |||
| // Maps an agent id to a the agents | |||
There was a problem hiding this comment.
@stonier consider Maps agent ids to agents. instead.
include/delphyne/agent_base.h
Outdated
| /// | ||
| /// Creators of scenarios need never to call this method. | ||
| /// | ||
| /// @param id: a unique id | ||
| /// @param road_geometry: the ground truth road information |
There was a problem hiding this comment.
@stonier missing trailing period on both sentences above.
include/delphyne/agent_base.h
Outdated
| /// | ||
| /// Creators of scenarios need never to call this method. | ||
| /// | ||
| /// @param id: a unique id |
There was a problem hiding this comment.
@stonier whose unique id? the agent's?
There was a problem hiding this comment.
Updated. Am trying to unwind this particular id as I don't think it's necessary. Will come in a future PR though.
| system_context->get_mutable_continuous_state().get_mutable_vector(); | ||
| drake::systems::BasicVector<double>* const state = | ||
| dynamic_cast<drake::systems::BasicVector<double>*>(&context_state); | ||
| DELPHYNE_ASSERT(state); |
There was a problem hiding this comment.
@stonier a comment stating why it is reasonable to make this assumption would be nice.
There was a problem hiding this comment.
Ugh, asserts. This was from the previous classes. I could second-guess the implications and write something here, but I'll unwind this entire method in Part III so that context retrieval and the subsequent assertion is not necessary.
There was a problem hiding this comment.
Done, across all the agents.
src/agents/mobil_car.h
Outdated
| */ | ||
| MobilCar(const std::string& name, const bool& direction_of_travel, | ||
| const double& x, const double& y, const double& heading, | ||
| const double& speed); |
There was a problem hiding this comment.
@stonier same as above regarding const references to POD types.
src/agents/rail_car.cc
Outdated
| std::make_unique<drake::automotive::PriusVis<double>>(id_, name)); | ||
|
|
||
| return 0; | ||
| if (initial_parameters_.lane.segment()->junction()->road_geometry()->id() != |
There was a problem hiding this comment.
@stonier we're making several assumptions regarding Lane state. We should have checks/assertions for each level of indirection, or state why that's not necessary.
| offset(offset), | ||
| speed(speed), | ||
| nominal_speed(nominal_speed) {} | ||
| } initial_parameters_; |
There was a problem hiding this comment.
@stonier what's the rationale/advantage of having this struct, as opposed to just defining those parameters as class fields?
There was a problem hiding this comment.
Communicates to the user that these parameters are only for initial construction of the object and not used thereafter. I also hope to drop in streaming operators for the agents and some infrastructure so that it's easy to see what the running agents have been configured with at runtime. This is a great means of debugging simple configuration mistakes.
There was a problem hiding this comment.
I haven't seen this pattern much, but I like it, especially if it will allow some sort of introspection in the future.
src/agents/rail_car.h
Outdated
| typedef drake::automotive::MaliputRailcarParams<double> | ||
| RailCarContextParameters; | ||
| typedef std::unique_ptr<RailCarContextParameters> RailCarContextParametersPtr; | ||
| typedef drake::automotive::MaliputRailcar2<double> RailCarSystem; |
There was a problem hiding this comment.
@stonier I'm fine with most typedefs, but obscuring pointers like this is not common in Drake. Not that I haven't seen it on other codebases, but I'd rather avoid it.
python/bindings/agents.cc
Outdated
| @@ -36,6 +38,15 @@ PYBIND11_MODULE(agents, m) { | |||
|
|
|||
| py::class_<delphyne::Agent>(m, "AgentBase"); | |||
|
|
|||
| py::class_<delphyne::MobilCar, delphyne::Agent>(m, "MobilCar") | |||
| .def(py::init<const std::string&, const bool&, const double&, | |||
| const double&, const double&, const double&>()); | |||
There was a problem hiding this comment.
Change const double& to double and similar. I think all the methods in this file use that notation.
python/delphyne/simulation_utils.py
Outdated
| x, # scene x-coordinate (m) | ||
| y, # scene y-coordinate (m) | ||
| heading, # heading (radians) | ||
| speed, # speed in the s-direction (m/s) |
There was a problem hiding this comment.
extra comma at the end. Plus I think pylint doesn't like the parenthesis alone in the bottom.
There was a problem hiding this comment.
Check, fixed for the others too.
| add_maliput_railcar(simulator, car_id, dragway, railcar_s, railcar_speed) | ||
| add_rail_car(simulator, | ||
| name=str(car_id), | ||
| lane = dragway.junction(0).segment(0).lane(1), |
| position=railcar_s, | ||
| offset=0.0, | ||
| speed=railcar_speed | ||
| ) |
There was a problem hiding this comment.
Same as above, I think that parenthesis should be in line 82
| y=0.0 + y_offset * (i % 3), | ||
| heading=0.0, | ||
| speed=velocity_base * i | ||
| ) |
| position=railcar_s, | ||
| offset=0.0, | ||
| speed=railcar_speed | ||
| ) |
| add_maliput_railcar(simulator, car_id, dragway, railcar_s, railcar_speed) | ||
| add_rail_car(simulator, | ||
| name=str(car_id), | ||
| lane = dragway.junction(0).segment(0).lane(1), |
| y=0.0 + y_offset * (i % 3), | ||
| heading=0.0, | ||
| speed=velocity_base * i | ||
| ) |
src/agents/rail_car.h
Outdated
| * @param direction_of_travel: designates whether the car will travel | ||
| * with or against the flow specified by the lane's rules | ||
| * @param longitudinal_position: initial position on the lane's track | ||
| * (maliput lane coordinate, 's' (m)) |
There was a problem hiding this comment.
+1 on the clarification in terms of maliput coordinate system, thanks!
| offset(offset), | ||
| speed(speed), | ||
| nominal_speed(nominal_speed) {} | ||
| } initial_parameters_; |
There was a problem hiding this comment.
I haven't seen this pattern much, but I like it, especially if it will allow some sort of introspection in the future.
include/delphyne/agent_base.h
Outdated
|
|
||
| /// @brief Prepare the backgroudn context(s) for the agent in this method. |
There was a problem hiding this comment.
@stonier typo in background.
include/delphyne/agent_base.h
Outdated
| /// Access the system, currently used internally by | ||
| /// @ref delphyne::AutomotiveSimulator "AutomotiveSimulator" to feed | ||
| /// |
There was a problem hiding this comment.
@stonier spurious extra line.
| * Systems | ||
| *********************/ | ||
| // driving | ||
| drake::automotive::MobilPlanner<double>* mobil_planner = |
There was a problem hiding this comment.
Fair point. Good that you've added then.
| system_context->get_mutable_continuous_state().get_mutable_vector(); | ||
| drake::systems::BasicVector<double>* const state = | ||
| dynamic_cast<drake::systems::BasicVector<double>*>(&context_state); | ||
| DELPHYNE_ASSERT(state); |
src/agents/rail_car.cc
Outdated
| *********************/ | ||
| // TODO(daniel.stonier): This is a very repeatable pattern for vehicle | ||
| // agents, reuse? | ||
| auto ports = aggregator->AddSinglePoseAndVelocityInput(name_, id); |
There was a problem hiding this comment.
@stonier should be specific about type here too.
There was a problem hiding this comment.
Done, across agents.
| EXPECT_LT(id2, 0); | ||
| // sim is using a different road geometry | ||
| simulator->SetRoadGeometry(std::move(dragway)); | ||
| auto different_dragway = CreateDragway("DifferentDragway", 2); |
There was a problem hiding this comment.
@stonier should be specific about type here too.
| 0.0, // offset | ||
| 0.0, // speed | ||
| 0.0 // nominal_speed | ||
| ); |
There was a problem hiding this comment.
@stonier here and below, missing units.
| @@ -578,7 +537,7 @@ TEST_F(AutomotiveSimulatorTest, TestLcmOutput) { | |||
| viewer_draw_callback); | |||
|
|
|||
| // Plus one to include the world. | |||
| const int expected_num_links = PriusVis<double>(0, "").num_poses() * 2 + 1; | |||
| const int expected_num_links = PriusVis<double>(0, "").num_poses() * 2 + 2; | |||
There was a problem hiding this comment.
Exercising the system with the road geometry added as well. Updated the comment to reflect that.
There definitely is some wierdness in the link count checks here and the test above though. That is out of the scope of this PR though.
| } | ||
|
|
||
| // Verifies that exceptions are thrown if a vehicle with a non-unique name is | ||
| // added to the simulation. | ||
| TEST_F(AutomotiveSimulatorTest, TestDuplicateVehicleNameException) { | ||
| auto simulator = std::make_unique<AutomotiveSimulator<double>>(); | ||
| auto road_geometry = |
There was a problem hiding this comment.
@stonier should be specific about type here too.
| simulator->AddLoadableAgent("rail-car", "decoy1", std::move(decoy_state1), | ||
| road, std::move(railcar_params1)); | ||
| auto decoy_1 = std::make_unique<delphyne::RailCar>( | ||
| "decoy1", *(road_geometry->junction(0)->segment(0)->lane(0)), |
There was a problem hiding this comment.
@stonier missing assert to ensure road_geometry is not nullptr.
There was a problem hiding this comment.
Also, I understand that we trust that these chained indirections will not cause a SIGSEGV because road_geometry is supposed to be a dragway. Still, I'd add a comment stating it.
There was a problem hiding this comment.
Again, getting out of scope of this actual PR and hitting alot of existing code. I think in this case we can say safe by construction (since it's explicit in a method at the top of the file and assumes such a Dragway construction is ok). That should and is tested elsewhere, so arguably we shouldn't need to repeat that here. If we did, I'd isolate that to a single test somewhere.
There was a problem hiding this comment.
Totally agree. That's why I suggested a comment.
There was a problem hiding this comment.
Aye, added in the last commit an hour ago, b65eb3d
Also added a convenience maliput library to play around with and for common patterns, push upstream. This means FindLane has disappeared from AutomotiveSimulator's interface. AgentBase added RoadGeometry to the Configure() call, necessary for checks against the road network, but in general, the principle is this is the point where the simulator passes to the agent the actual ground truth world entities.
stonier
force-pushed
the
stonier/other_cars
branch
from
f7b2cf1
to
bcbdbd4
Compare
|
More fixes in. @hidmic / @basicNew I know this is a relatively large PR, though technically it was merely a mechanical follow-on from #417 which I'd kept tightly scoped to address those technical concerns. Style's important, but in such a large PR, it's probably not the best place to address that in meticulous detail, especially given that so much of it is covering existing code and there was significant shuffling of code here. This is not a 'delphyne' style makeover PR. Note: starting to get difficult working through conflicts as other PR's go in. |
|
@stonier LGTM! I checked out your branch locally, tests are passing and demos are working fine. Sorry for the scope creeping, I had no way to tell what used to be there already and what not based on the PR diff only. |
|
Thanks lads for helping close the gaps! |
Additionally:
Configure()timeReadme.md)FindLanefromAutomotiveSimulatorto a maliput utilities library so agents can use it (seeReadme.mdthere)AutomotiveSimulator::road_⇨road_geometry_, there are other road components coming soon