Skip to content
This repository has been archived by the owner on Jul 22, 2021. It is now read-only.

Feature/add multilevel crowdsim #174

Open
wants to merge 29 commits into
base: master
Choose a base branch
from

Conversation

tianenchong
Copy link

@tianenchong tianenchong commented Dec 29, 2020

Added a lean (minimum working version to avoid performance degradation) crowdsim map for office (1 level) and clinic (2 levels)

Merge prerequisite: multilevel crowdsim traffic editor pr

Chong Tian En added 18 commits October 16, 2020 17:34
…unch.xml to office_crowd.launch.xml

and added building_crowdsim build task for office world in CMakeLists.txt
…om ignition fuel online models to src before building. Original MaleVisitorPhone model is removed from rmf_demo_assets src to reduce bulkiness.
…odified office_crowd demo building.yaml with new yaml format.
@luca-della-vedova luca-della-vedova self-requested a review January 11, 2021 01:58
Copy link
Member

@luca-della-vedova luca-della-vedova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! Before I try it out had a bunch of high level comments

@@ -0,0 +1,51 @@
<?xml version='1.0' ?>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we avoid creating a whole new file and just having the crowd sim be a launch parameter? Similar to what is done with ignition through the use_ignition launch parameter.

Copy link
Author

@tianenchong tianenchong Jan 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure about creating a whole new file, as there could be some legacy old files left over due to merging my old version with current rmf_demos. use use_crowdsim launch parameter can't be used here because even if we remove the crowdsim env variables in launch xml file to disable the plugin, the plugin section in sdf can still prompt the activation of plugin, and not being able to find the env variables can crash the demo.

Copy link
Member

@luca-della-vedova luca-della-vedova Jan 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds strange, we use the trick of "having a plugin in the world file but not in the path" to allow models to work with both ignition and gazebo so not finding a plugin should print an error but not crash the simulation, for example we do it for differential drive robots like the Caddy.
I could reproduce the crash when the menge environment variable is not present but it seems to be related to the plugin's code rather than gazebo / ignition itself.
Also from my understanding the environment variable is only for menge files, the plugin will still be loaded. What you could do is a check to make sure the environment variable exists / the files exist before trying to initialize menge to avoid raising the exception.

@@ -42,10 +45,14 @@
name="IGN_GAZEBO_SYSTEM_PLUGIN_PATH"
value="$(var plugin_path)"/>

<env
<env
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, remove this change?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pls go ahead to remove it. Apparently, the automatic style formatting does not apply to xml.

Comment on lines 0 to 1
{"dispensers": {"coke_dispenser": {"icons": {"coke_dispenser": ""}, "location": {"x": 16.85, "y": -4.8, "yaw": 0, "level_name": "L1"}}, "coke_ingestor": {"icons": {"coke_ingestor": "/dispensers/coke_ingestor/humanIcon.png"}, "location": {"x": 20.95, "y": -6.95, "yaw": 0, "level_name": "L1"}}}, "robots": {"tinyRobot": {"icons": {"tinyRobot": "/robots/tinyRobot/tinyRobot.png"}, "places": {"supplies": [], "tinyRobot2_charger": [], "coe": [], "tinyRobot1_charger": [], "hardware_2": ["coke_ingestor"], "cubicle_2": [], "pantry": ["coke_dispenser"], "station_1": [], "lounge": [], "cubicle_1": [], "hardware_1": [], "station_2": []}}, "deliveryRobot": {"icons": {"deliveryRobot": "/robots/deliveryRobot/deliveryRobot.png"}, "places": {}}}} No newline at end of file
{"dispensers": {"coke_ingestor": {"icons": {"coke_ingestor": "/dispensers/coke_ingestor/humanIcon.png"}, "location": {"x": 20.95, "y": -6.95, "yaw": 0, "level_name": "L1"}}, "coke_dispenser": {"icons": {"coke_dispenser": ""}, "location": {"x": 16.85, "y": -4.8, "yaw": 0, "level_name": "L1"}}}, "robots": {"deliveryRobot": {"icons": {"deliveryRobot": "/robots/deliveryRobot/deliveryRobot.png"}, "places": {}}, "tinyRobot": {"icons": {"tinyRobot": "/robots/tinyRobot/tinyRobot.png"}, "places": {"supplies": [], "tinyRobot2_charger": [], "coe": [], "tinyRobot1_charger": [], "hardware_2": ["coke_ingestor"], "cubicle_2": [], "pantry": ["coke_dispenser"], "station_1": [], "lounge": [], "cubicle_1": [], "hardware_1": [], "station_2": []}}}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change necessary or can it be reverted?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be no change in this file, as the changes are due to the old remnants.

rmf_demo_maps/maps/office/log.css Outdated Show resolved Hide resolved
Comment on lines 25 to 27
- [31, 13, {motion_axis: [1, start], motion_degrees: [3, 90], motion_direction: [2, 1], name: [1, main_door], plugin: [1, normal], type: [1, double_hinged]}]
- [35, 37, {motion_axis: [1, start], motion_degrees: [3, 90], motion_direction: [2, 1], name: [1, coe_door], plugin: [1, normal], type: [1, hinged]}]
- [18, 17, {motion_axis: [1, start], motion_degrees: [3, 90], motion_direction: [2, 1], name: [1, hardware_door], plugin: [1, normal], type: [1, hinged]}]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change also seems unnecessary?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change is auto generated upon save in traffic editor, essentially just some random reordering, which is deemed unnecessary.

@luca-della-vedova
Copy link
Member

A few high level comments, other than removing the changes in the few remaining files that still have a diff:

I tried to run the office demo and I noticed that sometimes the actors don't update their yaw and this results in the following behavior, did you notice this before?

actor_yaw

I also noticed quite a few crashes at startup for ignition but it's been quite some time since I ran the office demo there so they might be unrelated.

Also apart from making humans configurable at runtime I'd probably recommend reducing the number of humans in the office environment and making the ones in the clinic level 1 move around as well.
Ideally they should be "a bit" of an obstacle for robots (hence not fully static like the ones in the clinic level 1) but not "too much" (hence not crowded like the office).
You can probably try to run a few robot loops to have an idea of how much delays the humans cause to the robots.

@tianenchong
Copy link
Author

tianenchong commented Jan 11, 2021

I did noticed the yaw glitch. I have not been paying much attention to the glitch as majority of the human are not suffering from it. I have yet to find out what causes it but I do suspect the glitch happen as a result of narrow lane width from the start. Btw, narrow lane width also plays apart in traffic congestion, blocking robot's way with human standing around it using mobile phone. I will make adjustments to both maps tomorrow.

@luca-della-vedova
Copy link
Member

I didn't foresee the issue that when the plugin is disabled the humans will be just walking in place. I'll update the model on fuel so they have an Idle animation instead but we should still make sure that the simulations work correctly when the humans are not spawned (specifically that humans are spawned far away from robot lanes so they will not be on the way of robots if they are not walking).

@tianenchong
Copy link
Author

tianenchong commented Jan 13, 2021

In office demo, when the crowdsim is disabled, all humans will overlap at origin (0,0,0), away from office at top left corner, so they will not get in the way of robot for this map. The humans only move into their designated position in sdf file when the gazebo clock starts. I am not sure if this is a quirk for actor/non-static model in gazebo. In other maps where the origin is somewhere in the building, this could be a problem. Nevertheless, all actors are overlapped into one.

@tianenchong
Copy link
Author

Ignition and gazebo config resource path are different with "_ign" extension for the ignition map.

@luca-della-vedova
Copy link
Member

luca-della-vedova commented Jan 13, 2021

In office demo, when the crowdsim is disabled, all humans will overlap at origin (0,0,0), away from office at top left corner, so they will not get in the way of robot for this map. The humans only move into their designated position in sdf file when the gazebo clock starts. I am not sure if this is a quirk for actor/non-static model in gazebo. In other maps where the origin is somewhere in the building, this could be a problem. Nevertheless, all actors are overlapped into one.

I noticed that gazebo and ignition have very different behavior here. In ignition the actor is in the right position but playing the wrong animation (walking animation in the correct spawn point) while in gazebo it is in the wrong position but with the right animation (idle, all in the origin).
The best of both worlds would be to have actors spawn in the right position with the idle animation in both simulators. This is a change for traffic_editor simulation plugins though, the only part to make sure in this PR is that the spawning point is not in the middle of a robot lane (as is the case in both the maps now).

Ignition and gazebo config resource path are different with "_ign" extension for the ignition map.

True I didn't notice, duplicated is good then.

@luca-della-vedova
Copy link
Member

There have been quite a lot of changes since the dispatcher_demo PR was merged so these maps don't run anymore on the latest rmf_core.
Can you either rebase or merge the changes from the current main branch?

@tianenchong
Copy link
Author

Sure.

@tianenchong
Copy link
Author

tianenchong commented Jan 18, 2021

I have resolved the clinic map differences but haven't tried it. For office map, unfortunately, the vertices have changed too much and hence affects their reference indices that I have to remake the map.

I can't merge it today because I still need the old map and old setup to figure out the origin of the yaw glitch.

@luca-della-vedova
Copy link
Member

I have resolved the clinic map differences but haven't tried it. For office map, unfortunately, the vertices have changed too much and hence affects their reference indices that I have to remake the map.

I can't merge it today because I still need the old map and old setup to figure out the origin of the yaw glitch.

Any update on this? I am unable to test the demos since they don't run anymore with the latest rmf_core

@luca-della-vedova
Copy link
Member

It seems that by default humans are spawned in robot lanes, hence if crowdsim were to be disabled it would not be possible to run demo scenarios (example screenshot for office demo attached, but the same is true for the clinic demo).

image

We should be very careful about not breaking any existing behavior since I expect most people would want to run the demos without crowd simulation.

@tianenchong
Copy link
Author

For this demo and the clinic one, some part of human lane and robot lanes do overlap. Some human lanes (not all, because some part of the map can be quite narrow) vertices are positioned only slightly off the robot lane to avoid head-on obstruction.
If I understand correctly, what u meant is to spawn human at vertices of human lanes that are at completely different position from those of robot lanes?

@luca-della-vedova
Copy link
Member

luca-della-vedova commented Feb 1, 2021

what u meant is to spawn human at vertices of human lanes that are at completely different position from those of robot lanes?

Correct.
What I mean is that if the crowdsim was to be disabled the humans would be stuck in the middle of traffic lanes and it would not be possible to run the demos, hence their spawn point should not be in the middle of a robot's path (the screenshot I attached is such behavior, with the human there not moving and the robot unable to proceed).

@luca-della-vedova
Copy link
Member

Because of the upcoming migration would it be possible to split this into a PR with only the changes to the launch packages (that can be merged straight away) and a "future work" change for rmf_demo_maps?

@tianenchong
Copy link
Author

I think that should be possible as the launch script has crowdsim plugin off by default. The demo maps can be treated as future work since there isn’t much add on, apart from crowdsim tag for each level. Edwin is working with crowdsim branch btw.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants