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

Make Video recorder plugin work with MinimalScene #900

Merged
merged 15 commits into from
Sep 22, 2021

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Jul 2, 2021

Signed-off-by: ahcorde [email protected]

🎉 New feature

Summary

This PR is part of the consolidation between Scene 3D in ign-gazebo and ign-gui. This PR adds a plugin to record the user camera.

It builds on top of #813

Test it

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

@ahcorde ahcorde added the 🏯 fortress Ignition Fortress label Jul 2, 2021
@ahcorde ahcorde requested a review from chapulina July 2, 2021 08:01
@ahcorde ahcorde self-assigned this Jul 2, 2021
@ahcorde
Copy link
Contributor Author

ahcorde commented Jul 2, 2021

Require: gazebosim/gz-rendering#358

Signed-off-by: ahcorde <[email protected]>
@chapulina chapulina added needs upstream release Blocked by a release of an upstream library and removed needs upstream release Blocked by a release of an upstream library labels Jul 6, 2021
@ahcorde
Copy link
Contributor Author

ahcorde commented Jul 16, 2021

@osrf-jenkins retest this please

@ahcorde ahcorde marked this pull request as ready for review July 16, 2021 11:33
@ahcorde ahcorde marked this pull request as draft July 16, 2021 11:43
Base automatically changed from chapulina/6/scene_manager to main August 10, 2021 20:51
@ahcorde ahcorde marked this pull request as ready for review August 12, 2021 10:54
@ahcorde
Copy link
Contributor Author

ahcorde commented Aug 12, 2021

@chapulina open for review

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.

@ahcorde , for expediency, I pushed some suggestions directly to this branch in 7650dda instead of opening a PR against this branch. In summary:

  • Revert changes to gui.config
  • Merge VideoRecorderLogic into VideoRecorder like we're doing for TransformControl, with a legacy flag

Feel free to revert any of the changes I pushed as you see fit.

It seem to work for me with both MinimalScene and GzScene3D, but I'd appreciate it if @iche033 gives his ok when he's back. I'm mainly concerned about the extra camera update that we're doing in this plugin.

@chapulina chapulina changed the title Added Video recorder plugin Make Video recorder plugin work with MinimalScene Aug 13, 2021
@chapulina chapulina requested a review from iche033 August 13, 2021 23:05
@codecov
Copy link

codecov bot commented Aug 14, 2021

Codecov Report

Merging #900 (4e7964d) into main (c992f31) will not change coverage.
The diff coverage is n/a.

❗ Current head 4e7964d differs from pull request most recent head ab58eee. Consider uploading reports for the commit ab58eee to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##             main     #900   +/-   ##
=======================================
  Coverage   63.40%   63.40%           
=======================================
  Files         240      240           
  Lines       19548    19548           
=======================================
  Hits        12394    12394           
  Misses       7154     7154           

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 c992f31...ab58eee. Read the comment docs.

@iche033
Copy link
Contributor

iche033 commented Aug 16, 2021

It seem to work for me with both MinimalScene and GzScene3D, but I'd appreciate it if @iche033 gives his ok when he's back. I'm mainly concerned about the extra camera update that we're doing in this plugin.

I think we may be able to skip the camera update in this plugin. Given that the render event is emitted after the camera->Update() call in the scene 3d plugin, the image should be ready when the VideoEncoder calls AddFrame in this plugin

@chapulina
Copy link
Contributor

the render event is emitted after the camera->Update() call in the scene 3d plugin

Humm I think we may have a different issue then. Because the MinimalScene is not calling the render event in lockstep with simulation. This makes it hard for this plugin to record in lockstep...

@ahcorde
Copy link
Contributor Author

ahcorde commented Aug 17, 2021

@chapulina @iche033 what about adding a new camera in the same position and orientation as the original one ? then we can do whatever we want with this camera, isn't it ?

@chapulina
Copy link
Contributor

what about adding a new camera in the same position and orientation as the original one ?

That's an interesting idea 🤔 The new camera would need to track the original camera. And I imagine we'd have to protect against both cameras rendering at the same time, which I understand is not supported by ign-rendering / Ogre.

@iche033
Copy link
Contributor

iche033 commented Aug 24, 2021

what about adding a new camera in the same position and orientation as the original one ?

That's an interesting idea thinking The new camera would need to track the original camera. And I imagine we'd have to protect against both cameras rendering at the same time, which I understand is not supported by ign-rendering / Ogre.

The other option for lockstepping is to spawn the new camera on the server side and do the recording there (instead of the recording from the gui). Maybe that's what you mean? You can probably tweak the camera_video_recorder system with a new option to record from the gui camera instead of a camera sensor. If user indicates to record from the gui camera, the system just creates a new camera with same configuration as the gui camera and tracks the gui camera's pose.

@chapulina chapulina added the beta Targeting beta release of upcoming collection label Sep 8, 2021
@chapulina
Copy link
Contributor

The new approach of synchronizing Scene3D and the Sensors system that @mjcarroll is thinking of implementing could allow lockstepping the GUI camera when running with --same-process.

Signed-off-by: ahcorde <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
@chapulina chapulina merged commit 4be711c into main Sep 22, 2021
@chapulina chapulina deleted the ahcorde/plugin/video_recorder_logic branch September 22, 2021 00:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta Targeting beta release of upcoming collection 🏯 fortress Ignition Fortress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants