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

Suggestions to #200 #276

Merged
merged 4 commits into from
Mar 20, 2021
Merged

Suggestions to #200 #276

merged 4 commits into from
Mar 20, 2021

Conversation

chapulina
Copy link
Contributor

  • Change Create to Update to reflect better what the function is used for
  • Made ogre1 and ogre2 code very similar - the only difference are the 2s
  • Changed the Update function so that the existing mesh is updated even when a new mesh doesn't need to be created (in case we add other properties that make capsuleDirty without altering the mesh's name)
  • Got the markers to work with ign-gazebo, opening a PR soon.

@chapulina
Copy link
Contributor Author

I recommend hiding whitespace changes when reviewing, there aren't as many changes as it seems.

Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

thank you for the clean up and fixes 👍

@iche033
Copy link
Contributor

iche033 commented Mar 20, 2021

I think the Capsule_TEST failed due to segfault on shutdown

Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
@chapulina
Copy link
Contributor Author

I think the Capsule_TEST failed due to segfault on shutdown

I fixed the test by removing the call to destroy from the destructor. I don't know if things are being correctly cleaned up though, we may have memory leaks. Something to revisit after the release.

@codecov
Copy link

codecov bot commented Mar 20, 2021

Codecov Report

Merging #276 (7043917) into ahcorde/geom/capsule (32c501e) will decrease coverage by 0.06%.
The diff coverage is 29.03%.

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

@@                   Coverage Diff                    @@
##           ahcorde/geom/capsule     #276      +/-   ##
========================================================
- Coverage                 57.60%   57.53%   -0.07%     
========================================================
  Files                       157      159       +2     
  Lines                     15592    15595       +3     
========================================================
- Hits                       8982     8973       -9     
- Misses                     6610     6622      +12     
Impacted Files Coverage Δ
include/ignition/rendering/Capsule.hh 100.00% <ø> (ø)
include/ignition/rendering/base/BaseCapsule.hh 100.00% <ø> (ø)
...gre/include/ignition/rendering/ogre/OgreCapsule.hh 0.00% <ø> (ø)
ogre/src/OgreCapsule.cc 0.00% <0.00%> (ø)
ogre/src/OgreMarker.cc 0.00% <0.00%> (ø)
...2/include/ignition/rendering/ogre2/Ogre2Capsule.hh 100.00% <ø> (ø)
ogre2/src/Ogre2Marker.cc 46.09% <46.66%> (-6.76%) ⬇️
ogre2/src/Ogre2Capsule.cc 61.01% <53.57%> (+4.76%) ⬆️
...e/ignition/rendering/base/BaseGaussianNoisePass.hh 96.66% <0.00%> (-3.34%) ⬇️
... and 3 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 32c501e...0cb4dfe. Read the comment docs.

@chapulina chapulina merged commit 223ed97 into ahcorde/geom/capsule Mar 20, 2021
@chapulina chapulina deleted the chapulina/5/capsule branch March 20, 2021 04:04
@chapulina chapulina mentioned this pull request Mar 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants