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 camera tracking #226

Merged
merged 32 commits into from
Jul 6, 2021
Merged

Added camera tracking #226

merged 32 commits into from
Jul 6, 2021

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented May 26, 2021

Signed-off-by: ahcorde [email protected]

🎉 New feature

Summary

This PR is part of the consolidation between the scene3d in ign-gui and ign-gazebo.

This plugin allows to follow and moveto the camera. This requires to change a small detail in ign-gui scene3D when we create the camera, instead of using the method createCamera(void) we should use createCamera(string). Then we will be able to fetch the camera from this plugin.

Related PRs:

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
Copy link
Contributor Author

ahcorde commented Jun 10, 2021

* Add sky tag to the new Scene3D

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

* Update docs

Signed-off-by: ahcorde <[email protected]>
@chapulina chapulina added the 🏯 fortress Ignition Fortress label Jun 17, 2021
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
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.

I know this is still WIP, but I'll leave a note to myself that this PR doesn't compile by itself:

/home/chapulina/dev_bionic/ws_fortress/src/ign-gui/src/plugins/camera_controller_manager/CameraControllerManager.cc:434:63: error: 'KeyReleaseToScene' in namespace 'ignition::gui::events' does not name a type
     events::KeyReleaseToScene *keyEvent = static_cast<events::KeyReleaseToScene*>(_event);

@ahcorde ahcorde requested a review from jennuine as a code owner June 28, 2021 16:15
@ahcorde ahcorde changed the title Added camera_controller_manager Added camera tracking Jun 28, 2021
Signed-off-by: ahcorde <[email protected]>
@ahcorde ahcorde requested a review from chapulina June 29, 2021 08:04
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.

Works for me! I pushed some changes in 5fc8a01:

  • Added to scene.config
  • Added manual testing instructions to scene_provider

I think this would be a good opportunity to add tests, what do you think? 😇

src/plugins/camera_tracking/CameraTracking.hh Outdated Show resolved Hide resolved
src/plugins/camera_tracking/CameraTracking.cc Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 1, 2021

Codecov Report

Merging #226 (9c4df01) into main (1190ed0) will increase coverage by 3.66%.
The diff coverage is 55.15%.

❗ Current head 9c4df01 differs from pull request most recent head 3a5ae75. Consider uploading reports for the commit 3a5ae75 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #226      +/-   ##
==========================================
+ Coverage   61.57%   65.24%   +3.66%     
==========================================
  Files          21       27       +6     
  Lines        2803     3979    +1176     
==========================================
+ Hits         1726     2596     +870     
- Misses       1077     1383     +306     
Impacted Files Coverage Δ
include/ignition/gui/GuiEvents.hh 100.00% <ø> (ø)
src/Conversions.cc 100.00% <ø> (ø)
src/plugins/image_display/ImageDisplay.cc 30.32% <0.00%> (+7.05%) ⬆️
src/Plugin.cc 57.44% <17.64%> (-2.83%) ⬇️
src/plugins/screenshot/Screenshot.cc 33.33% <33.33%> (ø)
src/plugins/minimal_scene/MinimalScene.cc 52.15% <52.15%> (ø)
...s/transport_scene_manager/TransportSceneManager.cc 52.29% <52.29%> (ø)
src/plugins/camera_tracking/CameraTracking.cc 68.45% <68.45%> (ø)
src/plugins/scene3d/Scene3D.cc 48.63% <71.51%> (+35.85%) ⬆️
src/Application.cc 84.89% <100.00%> (+0.20%) ⬆️
... and 10 more

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 6f50c9c...3a5ae75. Read the comment docs.

@ahcorde ahcorde requested a review from chapulina July 1, 2021 11:02
Base automatically changed from chapulina/6/new_scene to main July 1, 2021 18:23
ahcorde and others added 2 commits July 1, 2021 20:35
@chapulina
Copy link
Contributor

chapulina commented Jul 2, 2021

I added a test and some other final tweaks in 3a5ae75. I think this is ready! Update: almost ready!

/////////////////////////////////////////////////
void CameraTrackingPrivate::Initialize()
{
// Attach to the first camera we find
Copy link
Contributor

Choose a reason for hiding this comment

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

We have to sort this out to make sure we're not attaching to a plugin, see #231 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's get this in as is for now and handle choosing the camera in a future PR, added an item here: #137 (comment)

@chapulina chapulina merged commit bbad5be into main Jul 6, 2021
@chapulina chapulina deleted the ahcorde/camera_controller_manager branch July 6, 2021 22:29
@@ -44,7 +44,7 @@ set(IGN_MATH_VER ${ignition-math6_VERSION_MAJOR})

#--------------------------------------
# Find ignition-common
ign_find_package(ignition-common4 REQUIRED VERSION 4.1)
ign_find_package(ignition-common4 REQUIRED COMPONENTS profiler VERSION 4.1)
Copy link
Member

Choose a reason for hiding this comment

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

a nightly build failure is a good reminder to update the Debian metadata with this new dependency

CMake Error at /usr/share/cmake/ignition-cmake2/cmake2/IgnConfigureBuild.cmake:59 (message):
  -- 	Missing dependency [ignition-common4] (Components: profiler)

Copy link
Contributor

Choose a reason for hiding this comment

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

We should add these to the buildfarmer's dashboard 😏

Copy link
Member

Choose a reason for hiding this comment

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

I will be learning about that soon

Copy link
Member

Choose a reason for hiding this comment

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

we can start by adding them to our Jenkins views: gazebo-tooling/release-tools#491

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

Successfully merging this pull request may close these issues.

3 participants