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

Include MoveTo Helper class to ign-rendering #311

Merged
merged 7 commits into from
May 25, 2021

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Apr 29, 2021

Signed-off-by: ahcorde [email protected]

🎉 New feature

This PR is part of the consolidation of Scene3D with ign-gazebo's GzScene3D gazebosim/gz-gui#137

Summary

This class is inside the ign-gazebo plugin which contains a TODO to move this hidden class to ign-rendering.

Test it

Checklist

Note to maintainers: Remember to use Squash-Merge

@chapulina
Copy link
Contributor

I like the idea of having this functionality inside ign-rendering. I'm not so sure about keeping it in a separate class though. I think it could be part of the camera API, together with all the Track and Follow functions:

https://github.com/ignitionrobotics/ign-rendering/blob/07c4ca96c924db4004ccab9e0b7a1e6ac5b1b033/include/ignition/rendering/Camera.hh#L196-L273

What do you think, @iche033 ?

@iche033
Copy link
Contributor

iche033 commented May 7, 2021

that was the intention before - putting it somewhere like in BaseCamera class. There is also an advantage of keeping this functionality in a separate class though. We can pimpl'ize it to avoid all the ABI compatibility headaches that we are facing with ign-rendering when making changes to templated classes in header files.

@chapulina
Copy link
Contributor

There is also an advantage of keeping this functionality in a separate class though.

Yeah that's true, the current implementation could be added to ign-rendering3 even 👍

@ahcorde ahcorde force-pushed the ahcorde/AddMoveToHelper branch from da9d145 to 1783087 Compare May 10, 2021 11:31
@ahcorde ahcorde changed the base branch from main to ign-rendering3 May 10, 2021 11:31
@ahcorde
Copy link
Contributor Author

ahcorde commented May 10, 2021

Added pimpl and retargeted to ign-rendering3

@ahcorde ahcorde marked this pull request as ready for review May 10, 2021 11:32
@ahcorde ahcorde requested a review from iche033 as a code owner May 10, 2021 11:32
Signed-off-by: ahcorde <[email protected]>
@iche033
Copy link
Contributor

iche033 commented May 11, 2021

@osrf-jenkins run tests please

Copy link
Contributor

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

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

Some small style updates

src/MoveToHelper.cc Show resolved Hide resolved
src/MoveToHelper.cc Show resolved Hide resolved
include/ignition/rendering/MoveToHelper.hh Outdated Show resolved Hide resolved
Signed-off-by: ahcorde <[email protected]>
@ahcorde ahcorde requested a review from mjcarroll May 12, 2021 14:25
Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

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

looks good to me.

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.

Should we add some tests?

@ahcorde ahcorde requested a review from iche033 May 19, 2021 08:47
@ahcorde
Copy link
Contributor Author

ahcorde commented May 19, 2021

@iche033 do you mind to have a look to the new tests? 8e98a13

@codecov
Copy link

codecov bot commented May 19, 2021

Codecov Report

Merging #311 (f111938) into ign-rendering3 (d3eb5ca) will increase coverage by 0.26%.
The diff coverage is 90.58%.

Impacted file tree graph

@@                Coverage Diff                 @@
##           ign-rendering3     #311      +/-   ##
==================================================
+ Coverage           52.94%   53.21%   +0.26%     
==================================================
  Files                 129      131       +2     
  Lines               11917    12002      +85     
==================================================
+ Hits                 6310     6387      +77     
- Misses               5607     5615       +8     
Impacted Files Coverage Δ
src/MoveToHelper.cc 90.47% <90.47%> (ø)
include/ignition/rendering/MoveToHelper.hh 100.00% <100.00%> (ø)

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 d3eb5ca...f111938. Read the comment docs.

Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

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

Just very small nitpicks. This is good to go as is

src/MoveToHelper_TEST.cc Outdated Show resolved Hide resolved
src/MoveToHelper_TEST.cc Outdated Show resolved Hide resolved
ahcorde added 2 commits May 25, 2021 08:06
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
@ahcorde ahcorde merged commit e14d708 into ign-rendering3 May 25, 2021
@ahcorde ahcorde deleted the ahcorde/AddMoveToHelper branch May 25, 2021 08:01
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.

4 participants