Automatically update CollisionGroups #1112
Conversation
|
As of right now this does not address #197, but we can fix that pretty easily by "versioning" the Shape objects. |
|
I'm attempting to add tests for Bullet and ODE collision detectors, but the Factory approach doesn't seem to be working. It looks like each library is supposed to register its collision detector with the static Factory when the library is loaded, but if an executable doesn't use any symbols from the library, then the linker will not bother having the executable link to the library (even though I'm instructing it to link using @jslee02 : Do you know a way to force a test to load a library without calling its symbols? In this case, I'm referring to test_CollisionGroups.cpp specifically. I'm attempting to link the libraries here. |
|
You're right. It seems there is no way at least with the current implementation. I've genuinely wondered if there is a nice solution for this. I guess it's related to #1079 and #1040 (#1040 (comment)). |
|
I agree that #1079 is related, but I don't think #1040 is related. The issue (as far as I can tell) is not about the visibility of symbols, but rather it's about whether the linker decides to link the executable to the library, as requested by the build system. On this topic, I've noticed something interesting. When I build using gcc/g++, here's what I get when I run You might notice that On the other hand, when I build using clang/clang++, I get this: Notice that this does have So it seems to just boil down to the discretion of the program that's used for linking, which is a deeply unsatisfying conclusion. |
|
One approach I can think of that would guarantee that the library gets linked is to use the dlfcn library (or on Windows, dlfcn-win32). This wouldn't be as convenient for users, because it requires you to know the external library location at runtime, but we could at least use it to ensure that our tests are able to link against all the collision libraries, regardless of which compiler is used to build them. |
Codecov Report
@@ Coverage Diff @@
## release-6.7 #1112 +/- ##
===============================================
+ Coverage 55.87% 56.34% +0.47%
===============================================
Files 343 344 +1
Lines 25192 25461 +269
===============================================
+ Hits 14076 14347 +271
+ Misses 11116 11114 -2
|
mxgrey
changed the title
|
I'm removing The implementation wound up being significantly more complicated than what I was originally hoping for, but I believe I've gotten it to be air-tight with the smallest possible run-time overhead. |
|
@mxgrey Thanks for this great changes! Overall looks good to me. Let me take a look at the details soon. |
There was a problem hiding this comment.
Automatic update control
The current implementation allows us to programmatically turn off the automatic update of the collision group even when a body node or skeleton is subscribed. I'm okay with that, but just wonder if we would have a use case of turning off the automatic update for the subscribing objects.
Supporting Objects
Currently, this API only supports BodyNode and Skeleton. I see the reason MetaSkeleton isn't supported yet. Are we going to support other types like ShapeFrame and CollisionGroup?
dart/collision/CollisionGroup.hpp
Outdated
|
|
||
| }; | ||
|
|
||
| /// \private Implementation of addShapeFrame. The source argument tells us |
There was a problem hiding this comment.
Nit: Good to learn \private Doxygen keyword! But, according to the documentation, it's only necessary for a language that doesn't have the concept of protection level natively like C++. Is there a reason we want to use this keyword explicitly?
There was a problem hiding this comment.
Ah, I got into a habit of applying it to everything that's private, because it's needed to hide things like private implementation classes. I didn't realize that private functions will automatically be hidden. I'll remove this.
dart/collision/CollisionGroup.hpp
Outdated
| /// \private Implementation of addShapeFrame. The source argument tells us | ||
| /// whether this ShapeFrame is being requested explicitly by the user or | ||
| /// implicitly through a BodyNode, Skeleton, or other CollisionGroup. | ||
| ObjectInfo* _addShapeFrameImpl( |
There was a problem hiding this comment.
Nit: I see the point you used a leading underscore for private member functions, but I don't think that's our convention. Could you remove it?
There was a problem hiding this comment.
I think the convention varies a bit throughout the source code, but that's probably just because I have a long-time habit of decorating private things with underscores (I like to make it obvious from the function name that a function isn't part of the public API). I can remove it if it's bothersome.
| const Others&... others) | ||
| { | ||
| const auto collisionShapeNodes = | ||
| bodyNode->getShapeNodesWith<dynamics::CollisionAspect>(); |
There was a problem hiding this comment.
This can be moved into the below if-statement to avoid unnecessary computation.
dart/collision/CollisionGroup.hpp
Outdated
| const dynamics::BodyNode*, BodyNodeSource>; | ||
|
|
||
| /// \private Internal function called to update a Skeleton source | ||
| bool _updateSkeletonSource(SkeletonSources::value_type& entry); |
There was a problem hiding this comment.
Nit: It seems this function returns true if an update is necessary. Could you add a documentation about it? Ditto for the below two functions.
|
The codacy complaints are false negatives, and the Appveyor failure seems to be a mysterious ICE unrelated to this PR which we can track under issue #1180. I'll go ahead and merge this now. |
|
Yeah, some codacy complaints are false negatives. Let's fix that we think necessary. Thanks for merging this! |
This PR adds features to allow
CollisionGroupobjects to subscribe toSkeletonandBodyNodeobjects, so that any changes to the properties of either of those structures will get reflected in the collision results.The implementation here might be a bit crude, but I tried my best to have it require as few overhead operations as possible, especially when no changes are being made to the Skeletons and BodyNodes.
I still plan on doing some more cleanup and writing more tests (so far I've only written one test, which shows a World getting automatically updated), so I'm marking this PR as a WIP for now. But I wanted to open it so that feedback is possible while I finish it up.
Before creating a pull request
clang-formatBefore merging a pull request
CHANGELOG.md