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

Destroy root node and clean up sensor resources on exit #617

Merged
merged 3 commits into from
May 9, 2022
Merged

Destroy root node and clean up sensor resources on exit #617

merged 3 commits into from
May 9, 2022

Conversation

iche033
Copy link
Contributor

@iche033 iche033 commented May 3, 2022

Signed-off-by: Ian Chen [email protected]

🦟 Bug fix

Fixes #615

Summary

When the scene was destroyed, only non-root nodes were destroyed. This PR makes sure to destroy the root node as well. As soon as that is done, I noticed that destructors of sensors are now called but tests are segfaulting. Fixed by cleaning up more resources in sensors' Destroy function.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • 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 and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

@iche033
destroy root node
293faea 
Signed-off-by: Ian Chen <[email protected]>
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label May 3, 2022
@iche033 iche033 mentioned this pull request May 3, 2022
Closed
@codecov
Copy link

codecov bot commented May 3, 2022 •
edited
Loading

Codecov Report

Merging #617 (80f7f87) into ign-rendering6 (abbe5d2) will not change coverage.
The diff coverage is n/a.

❗ Current head 80f7f87 differs from pull request most recent head db722be. Consider uploading reports for the commit db722be to get more accurate results

@@               Coverage Diff               @@
##           ign-rendering6     #617   +/-   ##
===============================================
  Coverage           80.00%   80.00%           
===============================================
  Files                   1        1           
  Lines                  15       15           
===============================================
  Hits                   12       12           
  Misses                  3        3

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 abbe5d2...db722be. Read the comment docs.

ahcorde
ahcorde requested changes May 4, 2022
View reviewed changes
ogre2/src/Ogre2GpuRays.cc Outdated Show resolved Hide resolved
ogre2/src/Ogre2GpuRays.cc Outdated Show resolved Hide resolved
@iche033 iche033 requested a review from ahcorde May 5, 2022 18:18
ahcorde
ahcorde approved these changes May 6, 2022
View reviewed changes
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.

I believe there are more places where we can use kCubeCameraCount. But I will create a follow up PR

@iche033
Copy link
Contributor Author

iche033 commented May 6, 2022

I believe there are more places where we can use kCubeCameraCount. But I will create a follow up PR

sounds good, thanks

@iche033
Copy link
Contributor Author

iche033 commented May 6, 2022

hmm a few windows warnings started showing up on CI, not sure why. They are not introduced by this PR. I''ll look into fixing them in a separate PR.

@codecov
Copy link

codecov bot commented May 6, 2022 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (ign-rendering6@abbe5d2). Click here to learn what that means.
The diff coverage is n/a.

@@                Coverage Diff                @@
##             ign-rendering6     #617   +/-   ##
=================================================
  Coverage                  ?   80.00%           
=================================================
  Files                     ?        1           
  Lines                     ?       15           
  Branches                  ?        0           
=================================================
  Hits                      ?       12           
  Misses                    ?        3           
  Partials                  ?        0

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 abbe5d2...d7f1b55. Read the comment docs.

@iche033
Copy link
Contributor Author

iche033 commented May 7, 2022

hmm a few windows warnings started showing up on CI, not sure why. They are not introduced by this PR. I''ll look into fixing them in a separate PR.

#622

@iche033
Fix Windows warnings (#622)
d7f1b55 
* fixing windows warnings

Signed-off-by: Ian Chen <[email protected]>

* testing msc_ver

Signed-off-by: Ian Chen <[email protected]>

* test explicity disabling warnings

Signed-off-by: Ian Chen <[email protected]>

* fixes more warnings

Signed-off-by: Ian Chen <[email protected]>

* fixes more warnings

Signed-off-by: Ian Chen <[email protected]>
@iche033 iche033 merged commit e4c9157 into ign-rendering6 May 9, 2022
@iche033 iche033 deleted the scene_destroy_root branch May 9, 2022 18:12
@iche033 iche033 mentioned this pull request May 9, 2022
Merged
8 tasks
iche033 added a commit that referenced this pull request May 10, 2022
@iche033
Cleanup depth camera resources on destroy (#623)
bb57d85 
A follow up to #617 - now that we are actually destroying sensors properly, I noticed a crash when running the depth camera integration test in ign-sensors. This should fix the crash (fix is similar to the changes done in #617)

Signed-off-by: Ian Chen <[email protected]>
@Blast545 Blast545 mentioned this pull request Jun 23, 2022
Open
mjcarroll added a commit that referenced this pull request Jul 29, 2022
@mjcarroll
Remove logic added in #617
44f1a1a 
Signed-off-by: Michael Carroll <[email protected]>
@mjcarroll mjcarroll mentioned this pull request Jul 29, 2022
Merged
mjcarroll added a commit that referenced this pull request Jul 29, 2022
@mjcarroll
Revert "Remove logic added in #617"
0a6c1b3 
This reverts commit 44f1a1a.
@chapulina chapulina mentioned this pull request Jul 29, 2022
Closed
@Crola1702 Crola1702 mentioned this pull request Aug 16, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahcorde ahcorde ahcorde approved these changes

Assignees
No one assigned
Labels
🏯 fortress Ignition Fortress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Leaks from Shaders on shutdown
2 participants
@iche033 @ahcorde