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

Added screenshot to toolbar #588

Merged
merged 3 commits into from
Mar 11, 2021
Merged

Added screenshot to toolbar #588

merged 3 commits into from
Mar 11, 2021

Conversation

jennuine
Copy link
Contributor

@jennuine jennuine commented Jan 28, 2021

Closes gazebosim/gz-gui#95
Added a screenshot service which saves an image of the current 3D scene from the user camera.
This depends on the ign-gui screenshot plugin gazebosim/gz-gui#170

Depends on gazebosim/gz-gui#170

Updated gui.config toolbar layout to use x / y positions instead of anchors and to contain the screenshot plugin.

image

@github-actions github-actions bot added the 🏰 citadel Ignition Citadel label Jan 28, 2021
@jennuine jennuine marked this pull request as ready for review January 28, 2021 02:22
@jennuine jennuine requested a review from chapulina as a code owner January 28, 2021 02:22
@codecov
Copy link

codecov bot commented Jan 28, 2021

Codecov Report

Merging #588 (575bca4) into ign-gazebo3 (c9f815a) will increase coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           ign-gazebo3     #588      +/-   ##
===============================================
+ Coverage        77.86%   77.90%   +0.03%     
===============================================
  Files              212      210       -2     
  Lines            11719    11648      -71     
===============================================
- Hits              9125     9074      -51     
+ Misses            2594     2574      -20     
Impacted Files Coverage Δ
src/gui/Gui.cc 66.40% <0.00%> (-0.53%) ⬇️
src/Util.cc 95.15% <0.00%> (-0.03%) ⬇️
src/network/NetworkManagerPrimary.cc 78.89% <0.00%> (ø)
...ems/kinetic_energy_monitor/KineticEnergyMonitor.hh
...ems/kinetic_energy_monitor/KineticEnergyMonitor.cc
src/Server.cc 83.43% <0.00%> (+0.63%) ⬆️
.../gui/plugins/transform_control/TransformControl.cc 15.92% <0.00%> (+0.88%) ⬆️
src/gui/plugins/entity_tree/EntityTree.cc 13.73% <0.00%> (+3.90%) ⬆️

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 c9f815a...efdab2a. Read the comment docs.

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.

With the suggestions on gazebosim/gz-gui#171 and #598, the ign-gui plugin can be used with both ign-gazebo and ign-gui, so we don't need to duplicate the logic here.

If you think those changes are ok, I'd suggest we update this pull request to:

  • Remove all the changes to Scene3D
  • Add the screenshot plugin from ign-gui to gui.config next to the shapes on the grey toolbar.

Signed-off-by: Jenn Nguyen <[email protected]>
@jennuine jennuine force-pushed the jennuine/screenshot branch from 850f5ef to 575bca4 Compare March 4, 2021 01:03
@jennuine jennuine changed the title Screenshot service Added screenshot to toolbar Mar 4, 2021
@chapulina chapulina added the needs upstream release Blocked by a release of an upstream library label Mar 10, 2021
@chapulina chapulina mentioned this pull request Mar 10, 2021
7 tasks
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.

LGTM! We just need an ign-gui release before merging: gazebosim/gz-gui#188

@chapulina chapulina removed the needs upstream release Blocked by a release of an upstream library label Mar 11, 2021
@chapulina
Copy link
Contributor

chapulina commented Mar 11, 2021

All the test failures seem to be due to a problem on Ignition Fuel's server, it's timing out:

$ ign fuel download -v 4 -u https://fuel.ignitionrobotics.org/1.0/OpenRobotics/models/Backpack
Downloading model: 
  Name: backpack
  Owner: openrobotics
  Server:
    URL: https://fuel.ignitionrobotics.org
    Version: 1.0

[Msg] Downloading model [https://fuel.ignitionrobotics.org/openrobotics/models/backpack]
[Err] [FuelClient.cc:596] Failed to download model.
  Server: https://fuel.ignitionrobotics.org
  Route: openrobotics/models/backpack/tip/backpack.zip
  REST response code: 504
Download failed.

I think we can ignore them and merge this.

@chapulina chapulina merged commit b47e7e5 into ign-gazebo3 Mar 11, 2021
@chapulina chapulina deleted the jennuine/screenshot branch March 11, 2021 19:36
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