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.

Sign up for GitHub

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

Jump to bottom

Fix more Animation memory leak #98

Merged
merged 3 commits into from
Sep 28, 2020
Merged

Fix more Animation memory leak #98

merged 3 commits into from
Sep 28, 2020

Conversation

iche033
Copy link
Contributor

@iche033 iche033 commented Sep 23, 2020

Continuing the effort from pull request #93, this PR fixes memory leaks in the animation classes by deleting objects that are new'd and using smart pointer where possible

Signed-off-by: Ian Chen ichen@osrfoundation.org

@iche033
fix animation mem leak
466d79e 
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
@iche033 iche033 requested a review from JShep1 September 23, 2020 05:20
@iche033 iche033 requested a review from mjcarroll as a code owner September 23, 2020 05:20
@github-actions github-actions bot added 🏰 citadel Ignition Citadel 📜 blueprint Ignition Blueprint 🔮 dome Ignition Dome labels Sep 23, 2020
@codecov
Copy link

codecov bot commented Sep 23, 2020 •
edited
Loading

Codecov Report

Merging #98 into ign-common3 will decrease coverage by 0.01%.
The diff coverage is 72.72%.

Impacted file tree graph

@@               Coverage Diff               @@
##           ign-common3      #98      +/-   ##
===============================================
- Coverage        73.95%   73.94%   -0.02%     
===============================================
  Files               69       69              
  Lines             9408     9408              
===============================================
- Hits              6958     6957       -1     
- Misses            2450     2451       +1
Impacted Files Coverage Δ
graphics/src/SkeletonAnimation.cc 43.83% <71.42%> (-1.50%) ⬇️
graphics/src/Animation.cc 91.83% <75.00%> (-0.35%) ⬇️

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 053083e...fc27ab5. Read the comment docs.

@iche033 iche033 mentioned this pull request Sep 23, 2020
Merged
mjcarroll
mjcarroll approved these changes Sep 23, 2020
View reviewed changes
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.

LGTM, but I think we should remove raw pointers when porting forward. I can do this separately if you would like.

graphics/src/Animation.cc
Comment on lines 263 to +266
delete this->positionSpline;
delete this->rotationSpline;
for (auto kf : this->keyFrames)
delete kf;
Copy link
Contributor

Choose a reason for hiding this comment

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

When bringing this patch forward to the main branch, let's change these to all use smart pointers. I don't think it's possible in a minor, because it would break ABI.

graphics/src/SkeletonAnimation.cc Outdated
Comment on lines 48 to 49
for (auto &it : this->data->animations)
delete it.second;
Copy link
Contributor

Choose a reason for hiding this comment

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

These could also be replaced by smart pointers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep done. c4c3a86

JShep1
JShep1 approved these changes Sep 23, 2020
View reviewed changes
Copy link

@JShep1 JShep1 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, I agree with Michael about the smart pointers.

@iche033
use smart pointer
c4c3a86 
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
@iche033
Copy link
Contributor Author

iche033 commented Sep 25, 2020

I think we should remove raw pointers when porting forward. I can do this separately if you would like.

yeah that'll be great. The remaining one is protected variable so I'm not sure if it's possible to deprecate it or if we should just change the type.

@iche033
Copy link
Contributor Author

iche033 commented Sep 28, 2020

ign-common3 is in Ignition Dome so I think we should hold off on merging this until after the release

@iche033
Copy link
Contributor Author

iche033 commented Sep 28, 2020

ok common3 is stable. Merging after last round of CI builds

@chapulina chapulina merged commit 335aa84 into ign-common3 Sep 28, 2020
@chapulina chapulina deleted the anim_mem_leak branch September 28, 2020 19:08
mjcarroll pushed a commit that referenced this pull request Oct 14, 2020
@iche033 @mjcarroll
Fix more Animation memory leak (#98)
c06c829 
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
mjcarroll pushed a commit that referenced this pull request Oct 19, 2020
@iche033 @mjcarroll
Fix more Animation memory leak (#98)
ac2c4b6 
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
Signed-off-by: Michael Carroll <michael@openrobotics.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JShep1 JShep1 JShep1 approved these changes

@mjcarroll mjcarroll mjcarroll approved these changes

Assignees
No one assigned
Labels
📜 blueprint Ignition Blueprint 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@iche033 @mjcarroll @JShep1 @chapulina