-
Notifications
You must be signed in to change notification settings - Fork 67
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
Feature/multilevel crowdsim #279
Conversation
… file and world file plugin generation.
…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
…awn_point current working level isolation
…lures allow parsing failures to not crash building_crowdsim
Signed-off-by: Morgan Quigley <[email protected]>
Signed-off-by: Morgan Quigley <[email protected]>
Signed-off-by: Morgan Quigley <[email protected]>
Signed-off-by: Morgan Quigley <[email protected]>
Signed-off-by: Morgan Quigley <[email protected]>
Signed-off-by: Morgan Quigley <[email protected]>
Signed-off-by: Morgan Quigley <[email protected]>
Signed-off-by: Morgan Quigley <[email protected]>
Signed-off-by: Morgan Quigley <[email protected]>
Add minimal CI test
There was a problem hiding this 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
andexternal_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.
namespace crowd_simulator { | ||
class CrowdSimInterface : public CrowdSimInterfaceCommon | ||
{ | ||
public: | ||
gazebo::common::Time _gz_last_sim_time; | ||
}; | ||
} | ||
|
There was a problem hiding this comment.
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/editor.cpp
Outdated
{ | ||
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)); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
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/include/traffic_editor/crowd_sim/crowd_sim_impl.h
Outdated
Show resolved
Hide resolved
traffic_editor/include/traffic_editor/crowd_sim/crowd_sim_impl.h
Outdated
Show resolved
Hide resolved
Signed-off-by: Morgan Quigley <[email protected]>
Signed-off-by: Morgan Quigley <[email protected]>
Signed-off-by: Morgan Quigley <[email protected]>
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
the mega-cleanup PR #301 has introduced some merge conflicts. I'll fix those tomorrow. |
@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 |
Signed-off-by: Morgan Quigley <[email protected]>
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 Can you paint the human vertex SVG's that way? I'm not sure if there is something I'm missing as to why |
Unfortunately, QIcon doesn’t seem to play well with animated svg, which is the primary reason why I resorted to using |
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. |
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. |
PR is stale, closing |
This PR adds the following features:
use_crowdsim
parameter at the launch scriptinclude
tags, and also totoggle_floors
gui plugin.navmesh.nav
,scene_file.xml
andbehavior_file.xml
generation for each building level in their separate folders underconfig_resource
folder.Bug fix:
Note:
This PR is not backward compatible with legacy
crowd_sim
tag outside oflevels
tag in building yaml file. Do ensure the legacycrowd_sim
tag is absent before editing.