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

Modelpropshop gazebo plugin port #1331

Merged
merged 23 commits into from
Mar 31, 2022
Merged

Modelpropshop gazebo plugin port #1331

merged 23 commits into from
Mar 31, 2022

Conversation

marcoag
Copy link
Member

@marcoag marcoag commented Feb 10, 2022

This is still work in progress.

Opening a PR here to track progress instead of marcoag/ign-modelphotoshoot#1.

This is the current progress on the different steps needed to achieve a functional plugin:

  • Load model into ign-gazebo (done through the sdf config, couldn't find a way to receive command line params in a plugin).
  • Create camera.
  • Create lights.
  • Change background color.
  • Render and take picture with camera.
  • Center model in picture.
  • Add different picture positions.
  • Make model joints move to random positions.
  • Add save-to-file option for the poses.
  • Shutdown server (it seems that this feature still not available: /server_control stop does nothing #1237), using --iterations 50 for now.

Currently working on: Make the model joints move to random positions.

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

🎉 New feature

As part of the work towards EricCousineau-TRI/repro#73.

Summary

Porting of the gazebo modelpropshop plugin.

Test it

Compile an run ign gazebo -s ./install/share/ignition/ignition-gazebo5/worlds/model_photo_shoot.sdf.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

Copy link
Contributor

@azeey azeey left a comment

Choose a reason for hiding this comment

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

It's great to see the PropShop plugin ported to ignition! I have a high level comment about how models are loaded.

return;
}

if (!sdf::readFile(this->modelLocation, modelSdf)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would think it would be easier and more consistent with the rest of Gazebo for Gazebo itself to load the model directly and then this plugin can just be attached to the model. This would eliminate the whole model loading logic here.

So instead of

    <plugin
      filename="ignition-gazebo-model-photo-shoot-system"
      name="ignition::gazebo::systems::ModelPhotoShoot">
      <model_uri>https://fuel.ignitionrobotics.org/1.0/OpenRobotics/models/barcs_qav500_sensor_config_1</model_uri>
      <translation_data_file>poses.txt</translation_data_file>
    </plugin>

It could be

<include>
  <uri>https://fuel.ignitionrobotics.org/1.0/OpenRobotics/models/barcs_qav500_sensor_config_1</uri>
  <plugin
      filename="ignition-gazebo-model-photo-shoot-system"
      name="ignition::gazebo::systems::ModelPhotoShoot">
      <translation_data_file>poses.txt</translation_data_file>
  </plugin>
</include>

Copy link
Member Author

Choose a reason for hiding this comment

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

The previous plugin would allow the user to specify the model location from the command line, I couldn't find any example on how to parse the arguments from the plugin side. Do we really want to add the command line loading option or is it enough with using the sdf file for that matter?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we have a way for plugins to parse arguments from the command line in ign-gazebo, so we'll have to leave that as a TODO for future work.

Signed-off-by: Marco A. Gutierrez <[email protected]>
@codecov
Copy link

codecov bot commented Mar 1, 2022

Codecov Report

Merging #1331 (a9dee08) into ign-gazebo5 (adeb304) will increase coverage by 0.33%.
The diff coverage is 91.33%.

@@               Coverage Diff               @@
##           ign-gazebo5    #1331      +/-   ##
===============================================
+ Coverage        66.95%   67.28%   +0.33%     
===============================================
  Files              279      281       +2     
  Lines            21302    21429     +127     
===============================================
+ Hits             14262    14419     +157     
+ Misses            7040     7010      -30     
Impacted Files Coverage Δ
src/systems/model_photo_shoot/ModelPhotoShoot.cc 91.26% <91.26%> (ø)
src/systems/model_photo_shoot/ModelPhotoShoot.hh 100.00% <100.00%> (ø)
src/SdfEntityCreator.cc 85.93% <0.00%> (+0.52%) ⬆️
src/rendering/RenderUtil.cc 46.21% <0.00%> (+0.73%) ⬆️
src/rendering/SceneManager.cc 37.12% <0.00%> (+2.47%) ⬆️
src/systems/sensors/Sensors.cc 78.19% <0.00%> (+4.26%) ⬆️

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 adeb304...a9dee08. Read the comment docs.

src/systems/model_photo_shoot/ModelPhotoShoot.cc Outdated Show resolved Hide resolved
src/systems/model_photo_shoot/ModelPhotoShoot.cc Outdated Show resolved Hide resolved
src/systems/model_photo_shoot/ModelPhotoShoot.cc Outdated Show resolved Hide resolved
src/systems/model_photo_shoot/ModelPhotoShoot.cc Outdated Show resolved Hide resolved
src/systems/model_photo_shoot/ModelPhotoShoot.cc Outdated Show resolved Hide resolved
src/systems/model_photo_shoot/ModelPhotoShoot.cc Outdated Show resolved Hide resolved
src/systems/model_photo_shoot/ModelPhotoShoot.cc Outdated Show resolved Hide resolved
src/systems/model_photo_shoot/ModelPhotoShoot.cc Outdated Show resolved Hide resolved
src/systems/model_photo_shoot/ModelPhotoShoot.cc Outdated Show resolved Hide resolved
@marcoag
Copy link
Member Author

marcoag commented Mar 3, 2022

It seems like accepting the change proposal through the gihub gui created a commit without signing it off making the DCO fail now. Shall I fix it and force push?

@azeey
Copy link
Contributor

azeey commented Mar 3, 2022

Yes, I don't know if there's something I haven't configured or what, but yeah, github doesn't sign the DCO when accepting suggested changes. Go ahead and force push with the fix.

@marcoag
Copy link
Member Author

marcoag commented Mar 4, 2022

I wanted to add links to the demo world on the tutorial but the sdf file is neither part of the ignition-edifice nor the main branch yet. Shall I add (temporarily broken) links to where I assume they will be or is it better to wait and add them later?

marcoag and others added 2 commits March 4, 2022 06:24
Co-authored-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Marco A. Gutierrez <[email protected]>
Signed-off-by: Marco A. Gutierrez <[email protected]>
Marco A. Gutierrez and others added 3 commits March 4, 2022 08:31
Signed-off-by: Marco A. Gutierrez <[email protected]>
Co-authored-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Marco A. Gutierrez <[email protected]>
Co-authored-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Marco A. Gutierrez <[email protected]>
@azeey
Copy link
Contributor

azeey commented Mar 4, 2022

I think either is fine. My preference would be to just put the path: examples/worlds/model_photo_shoot.sdf. Links to github are okay, but they can get outdated.

}
else
{
igndbg << "No jointAxisComp found, ignoring joint: " <<
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, that reminds me about fixed joints. We should probably check the joint type and ignore it if it's fixed. It would also be good to emit an error if the joint type is not Revolute or Prismatic.


A `camera sensor` must be added as it will be used by the plugin to take the
pictures. This allows plugin users to set the different parameters of the
camera to their desired values.
Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome tutorial! Thanks.

{
std::uniform_real_distribution<double> distribution(
jointAxisComp->Data().Lower(), jointAxisComp->Data().Upper());
float jointPose = distribution(generator);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be a double.

examples/worlds/model_photo_shoot.sdf Outdated Show resolved Hide resolved
Co-authored-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Marco A. Gutierrez <[email protected]>
Marco A. Gutierrez and others added 5 commits March 7, 2022 07:22
Signed-off-by: Marco A. Gutierrez <[email protected]>
Signed-off-by: Marco A. Gutierrez <[email protected]>
Signed-off-by: Marco A. Gutierrez <[email protected]>
Signed-off-by: Marco A. Gutierrez <[email protected]>
@chapulina
Copy link
Contributor

@marcoag @azeey , heads up that Edifice (ign-gazebo5) will EOL next week. Do you think this will get merged before then?

test/integration/model_photo_shoot.cc Outdated Show resolved Hide resolved
test/integration/model_photo_shoot.cc Outdated Show resolved Hide resolved
test/integration/model_photo_shoot.cc Outdated Show resolved Hide resolved
@marcoag
Copy link
Member Author

marcoag commented Mar 29, 2022

@marcoag @azeey , heads up that Edifice (ign-gazebo5) will EOL next week. Do you think this will get merged before then?

The PR should be ready between today and tomorrow, so hopefully we can squeeze it in.

Signed-off-by: Marco A. Gutierrez <[email protected]>
Marco A. Gutierrez and others added 6 commits March 30, 2022 09:33
Copy link
Contributor

@azeey azeey left a comment

Choose a reason for hiding this comment

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

LGTM! I made some small changes:

  • Enable only on Linux because I don't think rendering tests work on macOS and Windows yet.
  • Add include guard
  • Add unique directory for test so it doesn't interfere with other tests.

@azeey azeey merged commit 3458318 into ign-gazebo5 Mar 31, 2022
@azeey azeey deleted the marcoag/model_propshop branch March 31, 2022 13:50
chapulina added a commit that referenced this pull request Apr 13, 2022
* GzSceneManager: Prevent crash 💥 when inserted from menu (#1371)

Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: ahcorde <[email protected]>

Co-authored-by: ahcorde <[email protected]>

* Prepare version 6.7.0 (#1373)

* Prepare version 6.7.0

Signed-off-by: Jose Luis Rivero <[email protected]>

* Populate GUI plugins that are empty (#1375)

Signed-off-by: Louise Poubel <[email protected]>

* Fix visualization python tutorial (#1377)

Signed-off-by: ahcorde <[email protected]>

* Add xyz and rpy offset to published odometry pose (#1341)

* Added xyz and rpy offset to published pose

Signed-off-by: Aditya <[email protected]>

* Added headless rendering tutorial (#1386)

* Added headless rendering tutorial

Signed-off-by: Nate Koenig <[email protected]>

* Update tutorials/headless_rendering.md

Co-authored-by: Louise Poubel <[email protected]>
Signed-off-by: Nate Koenig <[email protected]>

* Update tutorials/headless_rendering.md

Co-authored-by: Louise Poubel <[email protected]>
Signed-off-by: Nate Koenig <[email protected]>

* Mention ogre2

Signed-off-by: Nate Koenig <[email protected]>

* Added note about software rendering

Signed-off-by: Nate Koenig <[email protected]>

* add 'on linux systems'

Signed-off-by: Nate Koenig <[email protected]>

* Add xyz and rpy offset to published odometry pose (#1341)

* Added xyz and rpy offset to published pose

Signed-off-by: Aditya <[email protected]>
Signed-off-by: Nate Koenig <[email protected]>

Co-authored-by: Nate Koenig <[email protected]>
Co-authored-by: Louise Poubel <[email protected]>
Co-authored-by: Aditya Pande <[email protected]>

* Allow to turn on/off lights (#1343)

Signed-off-by: ahcorde <[email protected]>
Co-authored-by: Louise Poubel <[email protected]>

* Add gazebo Entity id to rendering sensor's user data (#1381)

Adds the gazebo::Entity id to a rendering::Sensor's UserData object. The main use case is so that we can get back the corresponding gazebo Entity in the rendering thread when processing sensors.

Signed-off-by: Ian Chen <[email protected]>

Co-authored-by: Nate Koenig <[email protected]>
Co-authored-by: Alejandro Hernández Cordero <[email protected]>

* Don't mark entities with a ComponentState::NoChange component as modified (#1391)

Signed-off-by: Ashton Larkin <[email protected]>

* Disable ModelCommandAPI_TEST.RgbdCameraSensor on macOS (#1397)

Disabling test since it's very flaky.

Signed-off-by: Addisu Z. Taddese <[email protected]>

* Disable PeerTracker.PeerTrackerStale on macOS (#1398)

Signed-off-by: Addisu Z. Taddese <[email protected]>

* Toggle Light visuals (#1387)

Signed-off-by: ahcorde <[email protected]>

* Prevent hanging when world has only non-world plugins (#1383)

Signed-off-by: Louise Poubel <[email protected]>

Co-authored-by: Alejandro Hernández Cordero <[email protected]>

* Component inspector: refactor Pose3d C++ code into a separate class (#1400)

Signed-off-by: Louise Poubel <[email protected]>

* add initial_position param to joint controller system (#1406)

Similar to the <initial_position> parameter in the JointTrajectoryController system, this PR adds an <initial_position> parameter to the JointPositionController to let users specifies the initial target pos for a joint.

Signed-off-by: Ian Chen <[email protected]>

* Fix JointStatePublisher topic name for nested models (#1405)

The joint-state-publisher system currently assumes it's always attached to the top level model and hence generates an incorrect topic name for nested models. This PR updates it to generate the correct topic name.

Signed-off-by: Ian Chen <[email protected]>

* Added user command to set multiple entities (#1394)

Signed-off-by: ahcorde <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>

Co-authored-by: Louise Poubel <[email protected]>

* Disable tests that are expected to fail on Windows (#1408)

Signed-off-by: Louise Poubel <[email protected]>

* Fortress: Install Ogre 2.2, simplify docker (#1395)

Signed-off-by: Louise Poubel <[email protected]>

* SceneBroadcaster: only send changed state information for change events (#1392)

Signed-off-by: Ashton Larkin <[email protected]>

Co-authored-by: Louise Poubel <[email protected]>

* Add wheel slip user command (#1241)

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

* Distortion camera integration test (#1374)

Signed-off-by: William Lew <[email protected]>
Co-authored-by: Alejandro Hernández Cordero <[email protected]>
Co-authored-by: Louise Poubel <[email protected]>

* Add the Model Photo Shoot system, port of Modelpropshop plugin from Gazebo classic (#1331)

This system can be used to generate thumbnails of models. In comparison to the Modelpropshop plugin in Gazebo classic, this adds the ability to generate thumbnails after randomizing the joint positions of the model.

Signed-off-by: Marco A. Gutierrez <[email protected]>
Signed-off-by: Addisu Z. Taddese <[email protected]>

* Referring to Fuel assets within a heightmap (#1419)

Signed-off-by: Louise Poubel <[email protected]>

* Disable sensors in sensors system when battery is drained (#1385)

Added a new parameter <disable_on_drained_battery> to the sensors system. If set to true, sensors will be disabled if the model is out of battery.

It listens to the battery state of charge from the battery plugin, and if the charge reaches zero, all child sensors and nested sensors are set to be inactive.

Signed-off-by: Ian Chen <[email protected]>

* 🏁🎈 5.4.0 (#1420)

Signed-off-by: Louise Poubel <[email protected]>

* ServerConfig accepts an sdf::Root DOM object (#1333)

Signed-off-by: Nate Koenig <[email protected]>

Co-authored-by: Nate Koenig <[email protected]>
Co-authored-by: Michael Carroll <[email protected]>
Co-authored-by: Alejandro Hernández Cordero <[email protected]>

* Preparing 6.8.0 release (#1425)

* Prepareing 6.8.0 release

Signed-off-by: Jose Luis Rivero <[email protected]>

* Update changelog after merging forward port

Signed-off-by: Jose Luis Rivero <[email protected]>

* Add Gaussian noise to Odometry Publisher (#1393)

* Added gaussian noise and odometry with covariance publisher

Signed-off-by: Aditya <[email protected]>

Co-authored-by: Louise Poubel <[email protected]>

* Fix deprecation warnings for ModelPhotoShoot (#1437)

Signed-off-by: Louise Poubel <[email protected]>

Co-authored-by: ahcorde <[email protected]>
Co-authored-by: Jose Luis Rivero <[email protected]>
Co-authored-by: Aditya Pande <[email protected]>
Co-authored-by: Nate Koenig <[email protected]>
Co-authored-by: Nate Koenig <[email protected]>
Co-authored-by: Ian Chen <[email protected]>
Co-authored-by: Ashton Larkin <[email protected]>
Co-authored-by: Addisu Z. Taddese <[email protected]>
Co-authored-by: Ivan Santiago Paunovic <[email protected]>
Co-authored-by: William Lew <[email protected]>
Co-authored-by: Marco A. Gutiérrez <[email protected]>
Co-authored-by: Michael Carroll <[email protected]>
@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-04-13-fortress-edifice/1367/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏢 edifice Ignition Edifice
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants