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

Visual cloning: check for null children and recursively delete cloned visuals if failure occurs #434

Merged
merged 1 commit into from
Sep 25, 2021

Conversation

adlarkin
Copy link
Contributor

Signed-off-by: Ashton Larkin ashton@openrobotics.org

🦟 Bug fix

Summary

Follow-up to #397

If cloning a visual fails, we should destroy the top-level cloned visual recursively in order to ensure that all cloned child visuals are also destroyed. I also added a check to make sure that the child visual to clone isn't nullptr.

Checklist

  • Signed all commits for DCO
  • Added tests
  • 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

… visuals if failure occurs

Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Sep 25, 2021
@osrf-triage osrf-triage added this to Inbox in Core development Sep 25, 2021
@adlarkin adlarkin added the beta Targeting beta release of upcoming collection label Sep 25, 2021
@codecov
Copy link

codecov bot commented Sep 25, 2021

Codecov Report

Merging #434 (2944bc9) into main (45a49f0) will decrease coverage by 0.01%.
The diff coverage is 25.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #434      +/-   ##
==========================================
- Coverage   55.12%   55.11%   -0.02%     
==========================================
  Files         192      192              
  Lines       19381    19383       +2     
==========================================
- Hits        10683    10682       -1     
- Misses       8698     8701       +3     
Impacted Files Coverage Δ
include/ignition/rendering/base/BaseVisual.hh 70.20% <25.00%> (-1.23%) ⬇️

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 45a49f0...2944bc9. Read the comment docs.

Core development automation moved this from Inbox to In review Sep 25, 2021
@chapulina chapulina merged commit 35087de into main Sep 25, 2021
@chapulina chapulina deleted the adlarkin/cleanup_failed_visual_clone branch September 25, 2021 01:22
Core development automation moved this from In review to Done Sep 25, 2021
@j-rivero j-rivero removed this from Done in Core development May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta Targeting beta release of upcoming collection 🏯 fortress Ignition Fortress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants