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
Preserve metadata when detaching objects #2814
Conversation
This ensures the meta-data will not be removed below.
00cdb53
to
e89a5b6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, much better than my PR, and equally preserves the color
Thanks!
Codecov Report
@@ Coverage Diff @@
## master #2814 +/- ##
==========================================
+ Coverage 60.42% 60.51% +0.09%
==========================================
Files 366 366
Lines 31673 31672 -1
==========================================
+ Hits 19136 19163 +27
+ Misses 12537 12509 -28
Continue to review full report at Codecov.
|
Right. When we attach the object, we remove it from the world without removing the metadata. This avoids deleting the metadata when detaching the object with the REMOVE keyword. I still think it would be much easier to understand the code with a DETACH keyword, and/or if the metadata was copied from the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@corot proposed to fix object color handling, but the handling itself does not seem to be broken the way they expect.
Instead, when trying to remove an attached object from the world, color/type was removed even when the object was not actually removed from the world (attached objects are not in the world, so their meta-data should stay around).
Additionally here is a test to verify handling of color and type works as expected when objects get attached/detached.
@corot The bugfix should help you find the culprit in your use case. Please have a look at the test as well.