Fred/building_crowdsim for generating the navmesh file and required configuration files for menge

[FloodShao]

This sub-package is developed for generating the navmesh file and required configuration files from the building.yaml output by traffic-editor GUI.
This package is integrated within the building_map_tools package, and can be called using ros2 run command.
Before using building_crowdsim, the building.yaml files with "human_lanes" and "crowd_sim" tag. An example building.yaml for office scenario can be found here: https://github.com/FloodShao/traffic_editor/tree/fred/building_crowdsim_test/building_crowdsim_test_files

ros2 run command, after package building:

1. for generating the navmesh using "human_lanes"
   ros2 run building_map_tools building_navmesh navmesh [input_building_yaml] [output_dir]
2. for generating the configfiles using "crowd_sim"
   ros2 run building_map_tools building_navmesh configfile [input_building_yaml] [output_dir] [world.sdf_to_be_inserted_with_plugin] [gazebo_or_ign_tag]

[luca-della-vedova]

I see a bit of test code scattered around the code base, specifically in

• building_navmesh.py
• navmesh/test.py

I guess it could either be moved to actual unit / integration test files or deleted, what do you think?

[luca-della-vedova]

Another "high level" comment is that I would try to follow the conventions in the rest of the code base, from a quick glance (similar to the plugins PR):

• snake_case for function names and general style consistency (I see some snake case, some camelCase, some UpperCamelCase for function names scattered around). IIRC we generally follow PEP8 for Python.
• getters / setters are not typically used in Python and attributes are usually accessed directly, this should reduce the code size by quite a lot in simpler files, such as most of the smaller files under navmesh

[aaronchongth]

Thanks for putting these features together, @FloodShao!

I am hesitant to review this fully at the moment, as it requires a lot of workarounds to test it with the other branches, namely, #218 and #225. I will suggest we merge #218 first, rebase this branch, properly integrate the changes so that we can test the whole build pipeline.

@luca-della-vedova and I discussed a bit regarding maintainability, and would like your opinion on this. Since we are not writing APIs, but rather declaring a lot of different data structures, do you think it will be a better idea to keep the code base small by accessing and manipulating the data structures via their public members, instead of getters and setters?

Only when further conditions are needed to access data from data structures (eg. checks, calculations, etc), and that it is widely used across the code base, then we should consider defining getters for that.

The source files are currently missing their respective licenses, and the style should also be checked.

[FloodShao]

Agreed. We can merge #218 first, and I'm working on #225 and #224. Thanks for all that comments and advice on the code, that helps a lot.
Yes, I will try to avoid the getters and setters in python class (I should have checked with @luca-della-vedova first). and that definitely will make the code small.
I will modify the code style and licenses as well. Thanks!

[FloodShao]

Hi, @aaronchongth and @luca-della-vedova, I have reworked the class structure, removed unnecessary setter and getter functions, and modified the code style. I have rebased this pr last THU, so this PR includes the crowd_simulation_plugin now.
As for the test, I created a test branch base on this pr, in which a CMake base package in added to run the pipeline. https://github.com/FloodShao/traffic_editor/tree/fred/building_crowdsim_navmesh_test/building_crowdsim_pr_test.
Now you can follow the steps to test it:

1. use followsing repose:

repositories:
  rmf/rmf_core:
    type: git
    url: https://github.com/osrf/rmf_core.git
    version: master
  rmf/rmf_schedule_visualizer:
    type: git
    url: https://github.com/osrf/rmf_schedule_visualizer.git
    version: master
  rmf/traffic_editor:
    type: git
    url: https://github.com/FloodShao/traffic_editor.git
    version: fred/building_crowdsim_navmesh_test
  rmf/rmf_demos:
    type: git
    url: https://github.com/osrf/rmf_demos.git
    version: master
  meng_core:
    type: git
    url: https://github.com/FloodShao/menge_core.git
    version: master

2. colcon build the whole rmf workspace.
3. find the launch file in source folder (because I didn't install the launch file in the package share folder).
   for office scenario:
   https://github.com/FloodShao/traffic_editor/blob/fred/building_crowdsim_navmesh_test/building_crowdsim_pr_test/maps/office/office.crowd_simulation.launch.xml
   for airport scenario:
   https://github.com/FloodShao/traffic_editor/blob/fred/building_crowdsim_navmesh_test/building_crowdsim_pr_test/maps/airport_terminal/airport_terminal.launch.xml
4. launch the command:
   for office scenario: ros2 launch office.crowd_simulation.launch.xml
   for airport_scenario: ros2 launch airport_terminal.launch.xml gazebo_version:=9 if you only have gazebo9 now.

I think these steps should be able to test the pipeline after getting the "building.yaml".

Thank you for your time and patient for reviewing this PR.

[luca-della-vedova]

I'm looking at the generation CMakeLists and trying to understand the steps and if they can be simplified or documented better.

From what I understand there is need to add three build items to the CMakeLists if anyone was to add crowd simulation to their world:

1. The navmesh generation, which is the format that Menge expects to move the actors around.
2. The configfile generation, Another menge specific set of xml files (behavior_file and scene_file that define respectively the behavior of agents and their number / category / spawning points).
3. The nav graphs generation that I'm guessing is used by the simulation plugin to control the actors (?).

I wonder if there is any merging that can be done for those, I think the only reason we would want them separate would be if there is a use case for generating only a subset of them and it seems that anyone who wants to integrate crowd simulation will always need to generate all the three sets of files?
The idea behind the merging would be to make it easier for people to integrate, right now it seems they would need to add three custom commands with the right parameters or it would just not work.

Also I don't really understand the phony folder, what is its purpose?

[FloodShao]

Hi @luca-della-vedova , only https://github.com/FloodShao/traffic_editor/blob/fred/building_crowdsim_navmesh_test/building_crowdsim_pr_test/CMakeLists.txt#L44-#L75 is related to the crowd simulation setup. Only these block is inserted compared to the https://github.com/osrf/rmf_demos/blob/master/rmf_demo_maps/CMakeLists.txt
step 1 and step2 you are right. the nav_garphs generation is for the robot delivery rviz.

Yep, I think I can merge the two commands together into one if that's necessary. Because step 1 and step 2 are processing the same building.yaml, except step 2 requires a generated world.sdf as input.

[luca-della-vedova]

OK! Had some time to go through the code and give some more detailed feedback, for the CMakeLists I think we should try to integrate them in a single command so there is less risk for the downstream users to make mistakes (if for example they only call one of the two commands).

Also I'd recommend structuring tests so that their results are checked (i.e. make sure that a generated file is equal to a template), and integrating them with colcon so they are ran when doing colcon test

[FloodShao]

Current PR changes:
build and tested under focal/foxy/gazebo11
"plugin" part:

1. Modify the ign CMakeLists to change the ign plugin name the same as gazebo
2. Modify the gazebo actor spawn method to to make sure the plugin parameter the same as ign. Now the simulation only support gazebo11!
   "python" part:
3. Make the ModelType class platform independent.
4. Use one line command ros2 run building_map_tools building_crowdsim [.yaml] [output_dir] [world.sdf] to generate all the required files for menge
5. Add tests to package setup. compare the navmesh test with the standard result. Now the tests can be conducted under colcon test and checking the test result under colcon test-result
   (Thanks to Brandon's Issues with colcon test --python-testing pytest colcon/colcon-ros#105)

Test:
In branch https://github.com/FloodShao/traffic_editor/tree/fred/building_crowdsim_pr_test.
Both gazebo and ign launch file for office scenario are provided. Airport scenario only provided with gazebo launch file.
Environment:

1. Must be under gazebo 11! Although I think eloquent + gazebo11 should also work.
2. Please make sure you have model://MaleVisitorPhone in your ~/.gazebo/models (cannot directly use the walk.dae anymore after changes the actor spawn method)
   Steps:
3. Use the above branch for "traffic_editor"
4. colcon build
5. Go to src/rmf/traffic_editor/building_crowdsim_pr_test/maps/office.
   use ros2 launch office.launch.xml or ros2 launch office_ign.launch.xml for office scenario

An example for MaleVisitorPhone actor.
MaleVisitorPhone.tar.gz

[luca-della-vedova]

Running colcon test I found a test failure in the package:

</system-out></testcase><testcase classname="building_map_tools.test.building_crowdsim.test_build_configfile" file="test/building_crowdsim/test_build_configfile.py" line="83" name="test_plugin_from_yaml" time="0.181"><failure message="TypeError: __init__() takes 1 positional argument but 2 were given">test/building_crowdsim/test_build_configfile.py:89: in test_plugin_from_yaml
    plugin_file = Plugin(&apos;gazebo&apos;)

[FloodShao]

Running colcon test I found a test failure in the package:

</system-out></testcase><testcase classname="building_map_tools.test.building_crowdsim.test_build_configfile" file="test/building_crowdsim/test_build_configfile.py" line="83" name="test_plugin_from_yaml" time="0.181"><failure message="TypeError: __init__() takes 1 positional argument but 2 were given">test/building_crowdsim/test_build_configfile.py:89: in test_plugin_from_yaml
    plugin_file = Plugin(&apos;gazebo&apos;)

Apologies for that. This was caused by the changing Plugin constructor. The previous Plugin constructor requires one parameter for the platform. And I forgot to changed that line in the test file.

This problem is fixed now.

[luca-della-vedova]

Not 100% a fan of generating files in the source folder but I guess we can accept it for now, maybe just change the logic a bit so the file is deleted also when the test fails? (right now if I interpret it correctly the generated file will be added to the source folder if the assert fails)

[FloodShao]

Just fixed the deleting file problem and the test failure problem (it turns out the I forgot to update the test yaml file to be consistent with the current code as well.)
Now I think nothing is broken, and all tests pass.

[luca-della-vedova]

Awesome, thanks for all the changes!