-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add basic behaviour tree-based API #636
Conversation
import py_trees.composites | ||
import py_trees.common | ||
|
||
class Road(py_trees.composites.Sequence): |
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 would expect that the road will only contain cars, is that correct? I don't know if in a near future we could support "damageable" roads which could modify something else. So I think that roads could inherit from the parallel composite instead of sequence, using SuccessOnAll or SuccessOnOne policy. We should specify how a car could enter to a FAILURE state later on and go back to this composite
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.
That sounds pretty reasonable to me. I only started out with a Sequence
out of simplicity. @stonier thoughts?
8d28532
to
19c9962
Compare
python/delphyne/behaviours/agents.py
Outdated
self.nominal_speed = nominal_speed | ||
|
||
def setup(self, *, builder): | ||
road_index = self.parent.road_geometry.ById() |
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.
Playing around with the trees, I faced with one problem. This assumes that the parent is a road. I was adding a decorator to one of the cars and it crashed, since the parent now is a decorator, not a Road. I think that we should start specifying which variables should be exposed in a blackboard
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.
That's fair point. Feel free to update this PR with the changes necessary.
|
||
ROAD_GEOMETRY_KEY = 'road_geometry' | ||
SIMULATION_KEY = 'simulation' | ||
LANE_PROVIDER_KEY = 'lane_provider' |
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.
This should be removed
src/common/signal_guard.cc
Outdated
|
||
SignalGuard::~SignalGuard() { | ||
SignalGuard::allow_signal_handling = false; | ||
SignalGuard::signal_guards[signum_] = prev_signal_guard_; |
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.
Can we have a scenario where we construct one signal, and then we destroy another one so the atomic boolean turns to true and another thread tries to do something funny in the middle?
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.
This is not thread-safe, not by a long shot. But drake
isn't thread-safe either and a multithreaded simulation is not a deterministic one so I'd figured it wouldn't be a problem. We can document it accordingly though, I didn't :/
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.
If it's not thread safe, what's the point of using an atomic bool then? isn't pointless?
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.
Oops, I didn't reply. No, it's not pointless. Signals still arrive asynchronously and may be served concurrently.
Nevermind merge conflicts, we have an issue guys. Delphyne's |
72ee74f
to
7aa73ee
Compare
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.
LGTM
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
* Add blackboard helper and multilane road class * Add simulation as a key of the blackboard * Get the road geometry from the agent simulation builder * Add support for randomly placed cars in lanes (#639)
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
7aa73ee
to
144ff0b
Compare
Precisely what the title says. This is a first draft PR to support the bare minimum functionality necessary to start migrating existing demos.
Connected to #634.