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 #170 #171

Merged
merged 3 commits into from
Feb 2, 2021
Merged

Suggestions to #170 #171

merged 3 commits into from
Feb 2, 2021

Conversation

chapulina
Copy link
Contributor

Refactoring suggestions so we keep all the screnshot logic within the Screenshot plugin. This way:

  • There's no need to add any code to the scene, so no screenshot functionality is loaded unless the user wants to use it
  • The same screenshot plugin is usable with ign-gui and ign-gazebo (needs Also use Ignition GUI render event gz-sim#598)

The tutorials introduced in gazebosim/gz-sim#596 provide more context on this refactoring.

Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
@codecov
Copy link

codecov bot commented Jan 29, 2021

Codecov Report

Merging #171 (3d9aa76) into jennuine/screenshot (af13052) will increase coverage by 0.43%.
The diff coverage is n/a.

Impacted file tree graph

@@                   Coverage Diff                   @@
##           jennuine/screenshot     #171      +/-   ##
=======================================================
+ Coverage                59.88%   60.31%   +0.43%     
=======================================================
  Files                       20       20              
  Lines                     2535     2510      -25     
=======================================================
- Hits                      1518     1514       -4     
+ Misses                    1017      996      -21     
Impacted Files Coverage Δ
src/plugins/scene3d/Scene3D.cc 12.78% <ø> (-0.13%) ⬇️

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 af13052...3d9aa76. Read the comment docs.

@chapulina chapulina mentioned this pull request Jan 29, 2021
Copy link
Contributor

@jennuine jennuine left a comment

Choose a reason for hiding this comment

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

Works and LGTM! Thank you @chapulina!

@jennuine jennuine merged commit 385223b into jennuine/screenshot Feb 2, 2021
@jennuine jennuine deleted the chapulina/screenshot branch February 2, 2021 00:33
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.

2 participants