Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/multilevel crowdsim #279

Closed

Conversation

tianenchong
Copy link
Contributor

@tianenchong tianenchong commented Dec 29, 2020

This PR adds the following features:

  1. Independent multilevel crowdsim editing for building maps (added human properties for each vertex (only applicable to those in the human lanes), point-and-click feature at vertices to directly spawn humans, radiating dots at vertices to indicate valid human spawn points)
  2. Independent multilevel crowdsim building simulation plugin for ignition and gazebo, including support for use_crowdsim parameter at the launch script
  3. Add multilevel crowdsim plugin to world file generation, add non-static humans models to include tags, and also to toggle_floors gui plugin.
  4. Add independent multilevel crowdsim navmesh.nav, scene_file.xml and behavior_file.xml generation for each building level in their separate folders under config_resource folder.
  5. Redesigned unified profile and model type table in traffic editor gui.

Bug fix:

  1. ESC key for drawing lanes (no old vertex point retention).

Note:
This PR is not backward compatible with legacy crowd_sim tag outside of levels tag in building yaml file. Do ensure the legacy crowd_sim tag is absent before editing.

Chong Tian En added 26 commits November 4, 2020 11:52
…multilevel map (i.e. no segmentation fault), prohibit crowdsim on levels other than L1, add deletion function for human agent spawning for edges and vertices
@luca-della-vedova luca-della-vedova self-requested a review January 4, 2021 05:50
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.

Apart from the specific comments in the code chunks:

  • I see a lot of areas where we need to hack around when playing with agents because we use the same data structure for internal_agents and external_agents, hence there is a few areas where we need to be careful about assigning indexes and add a -1 (i.e. here), or places where we need to check in every iteration of agent groups whether they are external or not.
    I was wondering if it would be simpler to just keep the internal and external agents in separate data structures so you are guaranteed that whenever you need to loop through internal agents (most of the time) you don't have to worry about those edge cases all the time.

  • The const correctness of the code can be improved quite a bit, you should make all the functions that don't change the state const (for example here), const can also be used in local variables if you don't want to change them (changes becoming compile time errors can help to catch quite a few nasty bugs).

  • It would be good to compile some documentation (you can probably use @FloodShao 's original repo) and add it to the crowdsim folder, right now there is a lot of code and basically no documentation which makes it hard for other people to use it.

I have tried setting both levels to 0.01. They got faster. Apparently the 2nd floor is faster than the ground floor, which indicates some bug with the timestep.

Is this fixed?

I didn't have time to do thorough testing of the GUI yet but if the previous crashes have been fixed I think we should be getting there.

Comment on lines +40 to +47
namespace crowd_simulator {
class CrowdSimInterface : public CrowdSimInterfaceCommon
{
public:
gazebo::common::Time _gz_last_sim_time;
};
}

Copy link
Member

Choose a reason for hiding this comment

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

Instead of adding a layer of inheritance for a single variable can't the time just be stored as a member variable in the plugin class? Same for the Ignition plugin

traffic_editor/gui/building_level.cpp Outdated Show resolved Hide resolved
traffic_editor/gui/building_level.cpp Outdated Show resolved Hide resolved
Comment on lines 1612 to 1624
{
if (auto& impl = project.building.levels[level_idx].crowd_sim_impl)
{
auto& agent_groups = impl->get_agent_groups();
for (auto& agent_group:agent_groups)
{
auto p = agent_group.get_spawn_point();
if (p.second == v.y)
{
agent_group.set_spawn_point(p.first, stod(value));
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I'd try to refactor all those chunks into crowdsim and make the changes in editor.cpp as small as possible. They went from being one liners to being ~20 lines each and I think that affects readability quite a bit (this file is already very bulky).

traffic_editor/gui/multi_select_combo_box.h Outdated Show resolved Hide resolved
traffic_editor/include/traffic_editor/building_level.h Outdated Show resolved Hide resolved
codebot added 7 commits March 2, 2021 16:59
Signed-off-by: Morgan Quigley <[email protected]>
Signed-off-by: Morgan Quigley <[email protected]>
Signed-off-by: Morgan Quigley <[email protected]>
Remove project abstraction layer and built-in custom simulation plugin interface
@codebot
Copy link
Contributor

codebot commented Mar 3, 2021

the mega-cleanup PR #301 has introduced some merge conflicts. I'll fix those tomorrow.

@codebot
Copy link
Contributor

codebot commented Mar 3, 2021

@tianenchong can you give me write access to your fork of this repo? Then I'm happy to try to push some commits to it that fix the merge conflicts I just created. Thanks

@codebot
Copy link
Contributor

codebot commented Mar 4, 2021

OK, hopefully I did this correctly; I attempted to fix all the compilation errors and push onto the branch of this PR.

One thing that I am concerned about as-is, however, is that this branch requires people to install libqt5svg5-dev to render the human vertex SVG icons. However, so far as I understand, it's possible to render icons without bringing in this SVG dependency using the built-in Qt functions as happens here:
https://github.com/osrf/traffic_editor/blob/master/traffic_editor/gui/vertex.cpp#L137-L147

Can you paint the human vertex SVG's that way? I'm not sure if there is something I'm missing as to why QSvgWidget is needed.

@tianenchong
Copy link
Contributor Author

tianenchong commented Mar 5, 2021

Unfortunately, QIcon doesn’t seem to play well with animated svg, which is the primary reason why I resorted to using QSvgWidget. https://forum.qt.io/topic/32034/repaint-qicon-or-qiconengine

@codebot
Copy link
Contributor

codebot commented Mar 5, 2021

Does the SVG need to be animated? Can you attach a GIF of the animated? I tried to create a map where the SVG was animated but couldn't seem to do it.

@codebot
Copy link
Contributor

codebot commented Mar 5, 2021

It seems that the built-in Qt Animation framework might be a simple way to add animations, as well as having programmatic control over them, so that (for example) we could add checkboxes in the Preferences dialog for people to turn them on/off later: https://doc.qt.io/qt-5/animation-overview.html

I think it's probably nicer to generate these animations that way rather than having to hard-code their behavior in the SVG resource. As a side benefit, it seems that the Qt Animation framework doesn't require installing or linking anything other than what comes with the Qt5 default libraries.

@luca-della-vedova
Copy link
Member

PR is stale, closing

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

Successfully merging this pull request may close these issues.

5 participants