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

Cleanup and add Implementation pointer to Animation classes #160

Merged
merged 9 commits into from
Mar 12, 2021

Conversation

luca-della-vedova
Copy link
Member

Note! This PR introduces a dependency on ign-utils1 for the implementation pointer.

There was a static map hack added in ign-common3 to add a member variable to the Animation class that didn't have an implementation pointer.
This PR removes the hack, as well as adding an implementation pointer to all the classes in the file and doing some scattered cleanups mainly:

  • Templating the CreateKeyFrame function. Since both PoseAnimation and NumericAnimation had the same code with the only difference being the type I thought it might be good to remove the duplicated code and create an Animation templated function.
  • Change from raw to shared pointers for splines, just to be able to remove the custom destructor and make sure there is no issues in case of objects being copied and deleted. Tthere was no copy operator in either PoseAnimation or Animation and they all had raw pointer member variables that were deleted on destruction. If there happened to be a copy created and destructed, the memory would be freed and the original wouldn't be able to access it anymore.

Also I removed a few const from functions that were changing the state like BuildInterpolationSplines(). They were able to "get away with it" since there is a lot of mutable member variables but I can revert that if it's not a great idea.

Finally, I'm tempted to remove all the unit tests for copy / copy assignment / move operators for TrajectoryInfo since they are now just the default ones and they should be tested by the ImplPtr tests.

@luca-della-vedova luca-della-vedova added the 🏢 edifice Ignition Edifice label Jan 29, 2021
@codecov
Copy link

codecov bot commented Jan 29, 2021

Codecov Report

Merging #160 (cf18eb0) into main (1123a41) will decrease coverage by 0.03%.
The diff coverage is 94.73%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #160      +/-   ##
==========================================
- Coverage   75.74%   75.71%   -0.04%     
==========================================
  Files          73       73              
  Lines       10474    10428      -46     
==========================================
- Hits         7934     7896      -38     
+ Misses       2540     2532       -8     
Impacted Files Coverage Δ
graphics/src/Animation.cc 94.02% <94.73%> (+2.12%) ⬆️

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 1123a41...cf18eb0. Read the comment docs.

@chapulina chapulina mentioned this pull request Jan 29, 2021
5 tasks
@chapulina
Copy link
Contributor

This PR introduces a dependency on ign-utils1

I ticketed #161 to keep track of that.


CI is all green right now because it's skipping the graphics component:

   -- 	Cannot build component [graphics] - Missing: ignition-utils1

@chapulina
Copy link
Contributor

@osrf-jenkins run tests again, thanks!

@mjcarroll
Copy link
Contributor

The Focal action is a performance test, so I think we're okay there.

The Windows failures are AV related, which I also believe are expected

The MacOS failure is a bit suspicious, reinterpet_pointer_cast is added in 17, which should be available on MacOS. @scpeters do you have any insight here?

Signed-off-by: Michael Carroll <[email protected]>
@mjcarroll mjcarroll force-pushed the luca/refactor_animation branch from 248f4d7 to 2685e79 Compare March 2, 2021 18:54
@mjcarroll
Copy link
Contributor

@osrf-jenkins retest this please

@mjcarroll mjcarroll merged commit fda53ac into main Mar 12, 2021
@mjcarroll mjcarroll deleted the luca/refactor_animation branch March 12, 2021 14:49
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