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

Add PerformerDetector, a system for detecting when performers enter a specified region #125

Merged
merged 5 commits into from
May 29, 2020

Conversation

azeey
Copy link
Contributor

@azeey azeey commented May 5, 2020

The PerformerDetector system publishes a message when a performer enters or leaves its region. This can be used as a trigger in another system to do something in response to a performer approaching an area.

The system does not assume that levels are enabled, but it does need performers to be specified.
I needed to change the code in LevelManager.cc to read in performer info even when levers are not enabled.

I'll keep this as a draft while I work on a system that spawns objects by using the PerformerDetector as a trigger.

… specified region

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@chapulina chapulina added the 📜 blueprint Ignition Blueprint label May 12, 2020
azeey added 2 commits May 26, 2020 14:38
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey azeey requested a review from nkoenig May 26, 2020 19:41
@azeey azeey marked this pull request as ready for review May 26, 2020 19:41
@azeey azeey requested a review from chapulina as a code owner May 26, 2020 19:41
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

This plugin is very similar to Gazebo-classic's ContainPlugin, did you get a chance to look at that? Some differences being:

  • The usage of performers
  • The usage of AxisAlignedBox instead of OrientedBox (if I remember correctly, this was a helpful difference during the SRC)
  • The use of Intersects instead of Contains
  • The message being published
  • The "enabled" feature

It would be nice if we could say that the contain plugin has been migrated to Ignition. Do you think we could at least make the change to oriented box?

examples/worlds/performer_detector.sdf Show resolved Hide resolved
src/systems/performer_detector/PerformerDetector.cc Outdated Show resolved Hide resolved
src/systems/performer_detector/PerformerDetector.cc Outdated Show resolved Hide resolved
@nkoenig
Copy link
Contributor

nkoenig commented May 28, 2020

This plugin is very similar to Gazebo-classic's ContainPlugin, did you get a chance to look at that? Some differences being:

  • The usage of performers
  • The usage of AxisAlignedBox instead of OrientedBox (if I remember correctly, this was a helpful difference during the SRC)
  • The use of Intersects instead of Contains
  • The message being published
  • The "enabled" feature

It would be nice if we could say that the contain plugin has been migrated to Ignition. Do you think we could at least make the change to oriented box?

Those are good suggestions. The problem is timing. We need this out the door asap in order to meet a deadline on Wednesday next week. Can these features be saved for future improvements, maybe captured in an issue and moved on the project board?

Copy link
Contributor

@nkoenig nkoenig left a comment

Choose a reason for hiding this comment

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

Looks good, with the addition of documentation to the system.

@chapulina
Copy link
Contributor

Can these features be saved for future improvements, maybe captured in an issue and moved on the project board?

Sounds good, if @azeey doesn't get to it I can add it to my list of issues to ticket. I think the missing features can be added in a backwards-compatible way.

@azeey azeey self-assigned this May 28, 2020
@azeey
Copy link
Contributor Author

azeey commented May 28, 2020

This plugin is very similar to Gazebo-classic's ContainPlugin, did you get a chance to look at that? Some differences being:

Yes, @iche033 mentioned that in our design discussion. I didn't know that the ContainPlugin existed and I had already written this system by that point. I agree we can add the missing features in a backward-compatible way. The only issue I see is the name. I could rename this to EntityDetector, but that would give a false impression that this would work with any entity. Maybe we can rename the system later and use ign-plugin name aliases?

@iche033
Copy link
Contributor

iche033 commented May 28, 2020

yeah there are also a couple of other plugins in gazebo classic do something similar so it would be nice to make this generic. But if we don't have any plans / time to commit to this in the near future, we probably just do what @azeey suggetesd and rename and create an alias later when the time comes.

@chapulina
Copy link
Contributor

Sure, I don't think the name is a big deal, we've renamed other things as we ported them. We just need a migration guide for each plugin 🙂

EntityDetector, but that would give a false impression that this would work with any entity.

Given that the entity needs a geometry, and to be movable, I think limiting the plugin to performers is reasonable.

azeey added 2 commits May 28, 2020 18:01
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey azeey merged commit 3431635 into gazebosim:ign-gazebo2 May 29, 2020
@azeey azeey deleted the performer_detector branch May 29, 2020 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📜 blueprint Ignition Blueprint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants