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

Suggestions to #148 #150

Merged

Conversation

chapulina
Copy link
Contributor

  • Address codechecker warnings
  • Added tests
  • Pass and return const &
  • Changed the snap API to Position / Rotation / Scale, to better match APIs of other Ignition libraries, like math, rendering, common...
  • I think the "preview" concept is very specific to Gazebo and its server-client architecture. Other applications may be interested in using this event to just spawn anything into a scene. I think we'll only really know once we start making use of that event in Scene3D.

Signed-off-by: Louise Poubel <[email protected]>
@chapulina chapulina added the 🏰 citadel Ignition Citadel label Dec 10, 2020
Signed-off-by: Louise Poubel <[email protected]>
@chapulina chapulina mentioned this pull request Dec 10, 2020
@codecov
Copy link

codecov bot commented Dec 10, 2020

Codecov Report

❗ No coverage uploaded for pull request base (jshep1/add_gazebo_gui_events@be5a482). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@                       Coverage Diff                       @@
##             jshep1/add_gazebo_gui_events     #150   +/-   ##
===============================================================
  Coverage                                ?   60.16%           
===============================================================
  Files                                   ?       20           
  Lines                                   ?     2500           
  Branches                                ?        0           
===============================================================
  Hits                                    ?     1504           
  Misses                                  ?      996           
  Partials                                ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be5a482...8b4d158. Read the comment docs.

Copy link

@JShep1 JShep1 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the tweaks

@JShep1 JShep1 merged commit e4e6012 into jshep1/add_gazebo_gui_events Dec 10, 2020
@JShep1 JShep1 deleted the chapulina/jshep1/add_gazebo_gui_events branch December 10, 2020 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants