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

Dart integration v2 #29

Merged
merged 2 commits into from Feb 26, 2018

Conversation

Projects
3 participants
@costashatz
Contributor

costashatz commented Feb 9, 2018

This PR improves the DartIntegration introduced in #28 .. It is still work in progress, but at a quite good state:

  • Require DART version >=6.0.0; I am highly unsure if the code works with older versions of DART
  • Check for mesh updates from DART
  • Support soft bodies
  • Incorporate all the DART flags for rendering
  • Support multiple meshes and materials per DartIntegration::Object
  • Provide default behavior (i.e, remove it from scene) when an Object is deleted from the DART world; maybe we do not need to add this
  • Fix normals in DART SoftMeshShape; for now we are rendering them without normals
  • For MeshShapes use DART flags for rendering properly
  • Code optimization
    • Properly edit already created meshes (e.g., do not re-create them from scratch when just scaling needs to be changed)
    • Check the usage of std::shared_ptr; maybe replace it with std::unique_ptr? It could not be a good idea to have unique_ptrs as you may need to use the pointers to check whether a mesh is already created/or not
    • Check all the pointers in the code and see if they should/could be replaced by smart pointers

For an example usage, you can have a look at this branch, which I am updating to match the updates on this integration.

Let me know what you think!

@mosra

Hi, wow, didn't expect this so soon! :)

Did just a quick glance over the new stuff and made some minor course correction comments, hope that's okay :) Will dig deeper and discuss the overall design with you later -- I understand lots of this is still in a WIP stage.

@endcode
or
@code{.cpp}
dart::dynamics::ShapeNode* node = getShapeNodeFromDart();
SceneGraph::Object<SceneGraph::MatrixTransformation3D> object;
DartObject* obj = new DartObject{&object, node};
Object* obj = new Object{&object, node};

This comment has been minimized.

@mosra

mosra Feb 9, 2018

Owner

After this rename I would prefer to use the explicit namespace in the documentation snippets (DartIntegration::Object), similarly to what's done with SceneGraph APIs.

This comment has been minimized.

@costashatz

costashatz Feb 9, 2018

Contributor

After this rename I would prefer to use the explicit namespace in the documentation snippets (DartIntegration::Object), similarly to what's done with SceneGraph APIs.

Good point!

class Mesh;
class Buffer;
template<UnsignedInt dimensions> class Texture;

This comment has been minimized.

@mosra

mosra Feb 9, 2018

Owner

Please use one of the forward declaration headers instead of specifying the forward declarations by hand. It gets very messy very fast once the templates get involved :)

In this case it would be #include <Magnum/Magnum.h>.

This comment has been minimized.

@costashatz

costashatz Feb 9, 2018

Contributor

Please use one of the forward declaration headers instead of specifying the forward declarations by hand. It gets very messy very fast once the templates get involved :)

In this case it would be #include <Magnum/Magnum.h>.

I should at some point read in more detail the docs! 😄

Trade::PhongMaterialData material;
/** @brief Textures */
Containers::Array<Texture<2>*> textures;

This comment has been minimized.

@mosra

mosra Feb 9, 2018

Owner

Continuing with the forward declaration header from above, you could then simply use Texture2D here.

- `MultiSphereConvexHullShape`
- `PlaneShape` (this is an infinite plane with normal)
*/
bool _convertShapeNode();

This comment has been minimized.

@mosra

mosra Feb 9, 2018

Owner

This is a private function and so the documentation will get completely ignored. Can you put that to the class docs instead?

Also, please don't prefix functions with underscore. The func is private already, that's enough :)

This comment has been minimized.

@costashatz

costashatz Feb 9, 2018

Contributor

This is a private function and so the documentation will get completely ignored. Can you put that to the class docs instead?

Of course...

@@ -42,9 +43,9 @@ endif()
configure_file(${CMAKE_CURRENT_SOURCE_DIR}/configure.h.cmake
${CMAKE_CURRENT_BINARY_DIR}/configure.h)
corrade_add_test(DartIntegrationDartSkeletonTest DartSkeletonTest.cpp LIBRARIES MagnumDartIntegration ${DART_LIBRARIES})
target_include_directories(DartIntegrationDartSkeletonTest PUBLIC ${DART_INCLUDE_DIRS})
corrade_add_test(DartIntegrationDartWorldTest DartWorldTest.cpp LIBRARIES Magnum::OpenGLTester MagnumDartIntegration ${DART_LIBRARIES})

This comment has been minimized.

@mosra

mosra Feb 9, 2018

Owner

Because the test needs GL now, in order to keep things running properly on particular systems and the CIs (which don't have access to GL at the moment) there needs to be:

  • a new BUILD_GL_TESTS CMake option (see here for how's that done elsewhere -- search for BUILD_GL_TESTS in that file)
  • this test named DartIntegrationWorldGLTest (so it can be easily black/whitelisted during ctest run)
  • this test added only if(BUILD_GL_TESTS) (see e.g. here for an example), don't forget to split out the set_target_properties() command below as well
  • you need to enable the OpenGLTester lib on the CIs to avoid build failures, see e.g. mosra/magnum-extras@b27d0e2 for inspiration.

Sorry that most of the support code for GL tests is not here yet -- it was not needed (or not cared for) until now :)

This comment has been minimized.

@costashatz

costashatz Feb 9, 2018

Contributor

Sorry that most of the support code for GL tests is not here yet -- it was not needed (or not cared for) until now :)

No problem! Should I also do the -E GLTest as here?

This comment has been minimized.

@mosra

mosra Feb 10, 2018

Owner

Yes, please. I thought at least that is done already, but apparently just partially -- in particular this line should say -R Dart -E GLTest.

corrade_add_test(DartIntegrationDartSkeletonTest DartSkeletonTest.cpp LIBRARIES MagnumDartIntegration ${DART_LIBRARIES})
target_include_directories(DartIntegrationDartSkeletonTest PUBLIC ${DART_INCLUDE_DIRS})
corrade_add_test(DartIntegrationDartWorldTest DartWorldTest.cpp LIBRARIES Magnum::OpenGLTester MagnumDartIntegration ${DART_LIBRARIES})
target_include_directories(DartIntegrationDartWorldTest PUBLIC ${DART_INCLUDE_DIRS})

This comment has been minimized.

@mosra

mosra Feb 9, 2018

Owner

Now, thinking about this, isn't adding DART include dirs done implicitly through the transitive dependency on DART?

This comment has been minimized.

@costashatz

costashatz Feb 9, 2018

Contributor

Now, thinking about this, isn't adding DART include dirs done implicitly through the transitive dependency on DART?

Good catch! Thanks!

@codecov-io

This comment has been minimized.

codecov-io commented Feb 13, 2018

Codecov Report

Merging #29 into master will decrease coverage by 8.79%.
The diff coverage is 45.42%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master      #29     +/-   ##
=========================================
- Coverage   55.33%   46.53%   -8.8%     
=========================================
  Files          12       12             
  Lines         253      462    +209     
=========================================
+ Hits          140      215     +75     
- Misses        113      247    +134
Impacted Files Coverage Δ
src/Magnum/DartIntegration/Object.h 0% <0%> (ø)
src/Magnum/DartIntegration/World.h 0% <0%> (ø)
src/Magnum/DartIntegration/Object.cpp 1.14% <1.14%> (ø)
src/Magnum/DartIntegration/World.cpp 1.31% <1.31%> (ø)
src/Magnum/DartIntegration/ConvertShapeNode.h 100% <100%> (ø) ⬆️
src/Magnum/DartIntegration/ConvertShapeNode.cpp 92.85% <92.54%> (+49.81%) ⬆️
src/Magnum/BulletIntegration/ConvertShape.cpp 35.71% <0%> (-0.65%) ⬇️
src/Magnum/BulletIntegration/MotionState.cpp 100% <0%> (ø) ⬆️
... 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 e9b193d...f0dc765. Read the comment docs.

@costashatz costashatz changed the title from [WIP]: Dart integration v2 to Dart integration v2 Feb 13, 2018

@costashatz

This comment has been minimized.

Contributor

costashatz commented Feb 13, 2018

@mosra I think it's time that you can check more thoroughly what I've written and give me your thoughts/comments on the code and the 2 last bullets remaining (about the pointers).. Of course I will work on the documentation more.. 😄

Thanks!

@mosra

Sorry for the delays with this.

I made just a general comment regarding the design, with the goal to make this whole thing more testable -- currently Travis can't run the code at all due to requirement of the GL context, which is quite bad for numerous reasons, code coverage being one of them. After I can see what's being covered by tests and what not, I can comment further.

I'm not saying this often enough, sorry: thank you for the continued effort with this :)

@@ -30,7 +30,8 @@ else()
set(DARTINTEGRATION_TEST_DIR "${CMAKE_CURRENT_SOURCE_DIR}")
endif()
find_package(DART CONFIG REQUIRED OPTIONAL_COMPONENTS io-urdf utils-urdf)
find_package(Magnum REQUIRED OpenGLTester)

This comment has been minimized.

@mosra

mosra Feb 18, 2018

Owner

This line should be in the top-level CMakeLists and only if BUILD_GL_TESTS is enabled, like here for example.

@@ -42,9 +43,8 @@ endif()
configure_file(${CMAKE_CURRENT_SOURCE_DIR}/configure.h.cmake
${CMAKE_CURRENT_BINARY_DIR}/configure.h)
corrade_add_test(DartIntegrationDartSkeletonTest DartSkeletonTest.cpp LIBRARIES MagnumDartIntegration ${DART_LIBRARIES})
target_include_directories(DartIntegrationDartSkeletonTest PUBLIC ${DART_INCLUDE_DIRS})
corrade_add_test(DartIntegrationWorldGLTest WorldGLTest.cpp LIBRARIES Magnum::OpenGLTester MagnumDartIntegration ${DART_LIBRARIES})

This comment has been minimized.

@mosra

mosra Feb 18, 2018

Owner

Similarly to not needing to add DART include dirs, I think it's also not needed to link DART libs here ;)

This comment has been minimized.

@costashatz

costashatz Feb 18, 2018

Contributor

Here we need to link to DART libs, because for the test we require more (i.e., either utils-urdf or io-urdf) libs from DART than the rest of the integration and as such cannot link properly..

for(std::size_t i=0; i<normals.size(); ++i)
normals[i].normalize();
}

This comment has been minimized.

@mosra

mosra Feb 18, 2018

Owner

This whole thing could be replaced by MeshTools::generateFlatNormals(), I think. Correct me if I'm wrong, of course ;)

This comment has been minimized.

@costashatz

costashatz Feb 19, 2018

Contributor

This whole thing could be replaced by MeshTools::generateFlatNormals(), I think. Correct me if I'm wrong, of course ;)

Initially I was using the MeshTools::generateFlatNormals() functionality, but then I realized that their SoftMeshes are ''special''. Long story short, the triangles generated in the mesh do not all follow the same orientation (i.e., either clockwise or anti-clockwise) and as such I found this code in their own GUI implementations in order to try to fix this issue. The result is still not the best:

image

Nevertheless, it is much better than generating the normals assuming that they follow some orientation; in that case, we get wrong lighting (e.g., triangles completely black etc)..

As a side note, I found this nice library that has a nice facet orientation fixing algorithm. I might at some point re-implement their algorithm for Magnum (it might be a nice feature) and thus we could use it here to get much nicer normals..

This comment has been minimized.

@mosra

mosra Feb 19, 2018

Owner

Right, sorry, I remember we were talking about this.

I looked at the algorithm only briefly at first and thought it's generating flat normals (didn't see the += at the end). Now I see that it's generating smooth normals: averaging the normal over neighboring faces, doing some weighting on top (I'm not sure if completely correctly, as I see some weird artifacts on the screenshot). That fixes the face winding issue only by accident, I think -- only if there's not many faces with wrong winding.

Compared to the flat normals the result is quite ugly, though :/

Most importantly, why I'm commenting: this code snippet is never going to be tested well enough in the context of the DartIntegration library and since it doesn't really solve the problem in a nice way, I don't know if it's worth to invest further time in this direction (I originally wanted to suggest extracting this code out into MeshTools and test it there).

But I got two different ideas:

  1. what if you would, in case of those "special SmoothMeshes", add every face twice, once in the original winding and once in the reverse? That, together with enabling face culling and generating flat normals afterwards, could solve the issue in a nice-looking (yet ugly) way. This would need to be explicitly mentioned in the docs, as forgetting to enable face culling may have disastrous results, heh :)
  2. what if you would use functionality from Assimp itself to generate the normals? That would remove the need for the above code, avoid extra testing effort and we could blame a third-party lib for producing ugly artefacts 😆 The relevant option is here. But I give no guarantees that this approach would also accidentally circumvent the face winding issue (and it will still look ugly). So maybe solution 1 is still the best among the worst :D

By the way, did you report this "speciality" to DART? Fixing it there would be the best solution in my opinion.

This comment has been minimized.

@mosra

mosra Feb 19, 2018

Owner

I did a bit of research, opened mosra/magnum#229 so I don't forget all the details

This comment has been minimized.

@costashatz

costashatz Feb 19, 2018

Contributor

what if you would, in case of those "special SmoothMeshes", add every face twice, once in the original winding and once in the reverse? That, together with enabling face culling and generating flat normals afterwards, could solve the issue in a nice-looking (yet ugly) way. This would need to be explicitly mentioned in the docs, as forgetting to enable face culling may have disastrous results, heh :)

I think I like this idea more..! Let me finish the convertShapeNode() first and then I'll do this.. Thanks for the suggestions..

This comment has been minimized.

@costashatz

costashatz Feb 19, 2018

Contributor

By the way, did you report this "speciality" to DART? Fixing it there would be the best solution in my opinion.

They already know it! 😛 I can make a more explicit issue with possible solutions (or even fix it).. Putting it on my TO-DO list..!

This comment has been minimized.

@mosra

mosra Feb 19, 2018

Owner

I tried to search, but didn't find any issue related to this on their main repo.

Adding a link to the issue to the documentation about "why is everything twice and why one needs to enable face culling" would be great, so somebody can look it up later and discard the whole workaround once they fix it :)

This comment has been minimized.

@costashatz

costashatz Feb 19, 2018

Contributor

Adding a link to the issue to the documentation about "why is everything twice and why one needs to enable face culling" would be great, so somebody can look it up later and discard the whole workaround once they fix it :)

The thing is that they do not really care about the graphics part of the library. They have it (and update it) only when they need something (or someone does it for them). That's why I said it's better if I implement it myself for them.. 😛

if(!_node)
trans = &_body->getRelativeTransform();
else
trans = &_node->getRelativeTransform();

This comment has been minimized.

@mosra

mosra Feb 18, 2018

Owner

is getRelativeTransform() really returning a reference? I would just copy the thing by value here.

This comment has been minimized.

@costashatz

costashatz Feb 18, 2018

Contributor

Yes it is returning a const reference (see here) and I measured it and we gain a few μs that might be crucial (Eigen::Isometry3d is not the simplest structure).

This comment has been minimized.

@mosra

mosra Feb 19, 2018

Owner

Okay then :)

I really need to dig deeper into Eigen :D I tried to understand what Isometry3d actually is a few times, but always failed :)

This comment has been minimized.

@costashatz

costashatz Feb 19, 2018

Contributor

I tried to understand what Isometry3d actually is a few times, but always failed :)

Eigen::Isometry3d is an alias for Eigen::Transform that allows only isometric transformations.

This comment has been minimized.

@mosra

mosra Feb 19, 2018

Owner

I mean, like the internal representation, if it's a quat + translation, or a matrix or what exactly. But, nevermind :)

This comment has been minimized.

@costashatz

costashatz Feb 19, 2018

Contributor

Never really wondered about it, but it seems that it is a matrix :D..

std::reference_wrapper<ShapeData> Object::shapeData() {
return std::ref(*_shapeData);
}

This comment has been minimized.

@mosra

mosra Feb 18, 2018

Owner

Why not just returning ShapeData&? std::reference_wrapper exists only so references can be stored inside std::vector or std::initializer_list, it is nothing more than an annoyance outside of these cases.

This comment has been minimized.

@costashatz

costashatz Feb 18, 2018

Contributor

Why not just returning ShapeData&? std::reference_wrapper exists only so references can be stored inside std::vector or std::initializer_list, it is nothing more than an annoyance outside of these cases.

You're right!

Containers::Array<Mesh*> meshes(meshesCount);
Containers::Array<Buffer*> vertexBuffers(meshesCount);
Containers::Array<Buffer*> indexBuffers(meshesCount);
Containers::Array<Containers::Optional<Trade::PhongMaterialData>> materials(meshesCount);

This comment has been minimized.

@mosra

mosra Feb 18, 2018

Owner

(Just for the record, as you told me on gitter: you're aware this leaks)

I think the arrays could contain the instances directly here (possibly wrapped in Containers::Optional, if that's needed). The object is the single owner of the ShapeData structure and it will provide references to it to whoever needs it for rendering, so no need for shared_ptr or anything.

not the other way around. To get the latest DART transformation, you should
update the object with @ref update().
@brief convertShapeNode

This comment has been minimized.

@mosra

mosra Feb 18, 2018

Owner

Um. This line doesn't quite make sense here. I assume it's a leftover when copypasting the docs from their original location? ;)

Object& update();
/** @brief Get whether Object was updated */
bool used();

This comment has been minimized.

@mosra

mosra Feb 18, 2018

Owner

This could be inline in the header, as the function is really small; similarly for the updatedMesh() function below and maybe also clearUsed().

Regarding naming: why is this named "used" and the one below is named "updatedMesh"? Can't both be "updated", especially when the function documentation is saying that?

Naming style: functions returning bool are usually named isSomething or hasSomething, could you change the naming to reflect that? Thank you :)

private:
explicit Object(SceneGraph::AbstractBasicObject3D<Float>& object, SceneGraph::AbstractBasicTranslationRotation3D<Float>& transformation, dart::dynamics::ShapeNode* node, dart::dynamics::BodyNode* body);
bool convertShapeNode();

This comment has been minimized.

@mosra

mosra Feb 18, 2018

Owner

So, this comment is why the review took so long (sorry about that): I tried to come up with a solution to make this whole thing more testable because otherwise it's not really possible to verify that everything is working correctly -- all you get now are some opaque Buffer and Mesh structures. I think I have it now. So, what about this:

Previously, there was a convertShapeNode() function that didn't depend on GL context being present and which just returned the populated ShapeData. This function was very testable, so I'd vote for restoring it (together with the header it was), but with an updated API:

  • it takes the ShapeNode&, as previously, but has no overload for DartIntegration::Object, as this class handles it internally
  • it takes an EnumSet to describe which data should be extracted and which not so the Object can update only what's needed
  • it takes an existing instance of plugin manager instead of creating an instance of it inside

The Object would call convertShapeNode() inside, asking for extraction of only the data that changed, and then it calls MeshTools::compile() to populate its own internal mesh, buffer etc. instances. (So, yes, the ShapeData would contain just the data again.)

Instead of having the plugin manager as static variable inside a function, the instance would be part of the World instance and every Object would have access to it. (Reason: static variables inside functions are very evil -- they cause the compiler to generate thread-safe wrapper around these even if you aren't involve with any threading, the destruction of the whole variable is at some unspecified point after exiting from main(), etc. etc.)

This way the majority of the code could be properly tested again (maxing out the code coverage, verifying how the imported mesh data actually look etc.). Besides that, this would make this library less tightly integrated with SceneGraph, allowing users to implement the transformation handling in their own way, if they want to.

I hope I didn't miss anything that would make this impossible. As always, I'm open for further discussion :)

This comment has been minimized.

@costashatz

costashatz Feb 19, 2018

Contributor

Previously, there was a convertShapeNode() function that didn't depend on GL context being present and which just returned the populated ShapeData. This function was very testable, so I'd vote for restoring it (together with the header it was), but with an updated API:

it takes the ShapeNode&, as previously, but has no overload for DartIntegration::Object, as this class handles it internally
it takes an EnumSet to describe which data should be extracted and which not so the Object can update only what's needed
it takes an existing instance of plugin manager instead of creating an instance of it inside
The Object would call convertShapeNode() inside, asking for extraction of only the data that changed, and then it calls MeshTools::compile() to populate its own internal mesh, buffer etc. instances. (So, yes, the ShapeData would contain just the data again.)

Thanks for the comments! I am working on this and I should have something by tomorrow. I do not see any reason for now why this would not work...

void texture();
void bench_simple();
void bench_soft();

This comment has been minimized.

@mosra

mosra Feb 18, 2018

Owner

Coding style: use camelCase instead of underscore_case, please. Also no need to prefix with test or bench, it's clear enough from the test output :)

@mosra

I did a thorough review now. I like how simple it turned out in the end and how testable it is now :)

@@ -45,6 +47,10 @@ namespace dart { namespace dynamics {
class ShapeNode;
}}
namespace Magnum { namespace Trade{
class AbstractImporter;
}}

This comment has been minimized.

@mosra

mosra Feb 20, 2018

Owner

There's #include <Magnum/Trade/Trade.h> for this ;)

Similarly in Object.h.

/* get diffuse color from Mesh color */
Color4 meshColor = meshData->colors(0)[meshShape->getColorIndex()];
new(&materials[j]) Trade::PhongMaterialData{Trade::PhongMaterialData::Flags{}, 80.f};
materials[j].diffuseColor() = Color3(meshColor[0], meshColor[1], meshColor[2]);

This comment has been minimized.

@mosra

mosra Feb 20, 2018

Owner

(Sorry, there's a bunch of TODOs on my side related to this piece of code already 😅)

You could use just meshColor.rgb() here.

}
/* fallback mesh data to by-pass seg-faults */
else
new(&meshes[j]) Trade::MeshData3D{MeshPrimitive::Triangles, std::vector<UnsignedInt>(), std::vector<std::vector<Vector3>>(1, std::vector<Vector3>()), std::vector<std::vector<Vector3>>(), std::vector<std::vector<Vector2>>(), std::vector<std::vector<Color4>>()};

This comment has been minimized.

@mosra

mosra Feb 20, 2018

Owner

As we discussed before -- wouldn't Containers::Array<Containers::Optional<Trade::MeshData3D> (and similar for materials) be a better / safer solution here, to avoid such suspiciously-looking workarounds?

One more argument for Optional: if someone is using the ShapeData directly, they currently wouldn't have a intuitive way to know if a particular mesh is valid or not. With Optional this would be obvious.

Now I'm thinking -- how many imported objects are non-meshes? If it's like one of ten, then I would just drop all code that calculates meshesCount and use the full importer->object3DCount() instead.

This comment has been minimized.

@costashatz

costashatz Feb 20, 2018

Contributor

One more argument for Optional: if someone is using the ShapeData directly, they currently wouldn't have a intuitive way to know if a particular mesh is valid or not. With Optional this would be obvious.

If a mesh is asked to be loaded and cannot be loaded, we return Containers::NullOpt. Otherwise the size of the ShapeData::meshes gives us the number of loaded meshes (Trade::MeshData3D) and all of them are always valid..

As we discussed before -- wouldn't Containers::Array<Containers::OptionalTrade::MeshData3D (and similar for materials) be a better / safer solution here, to avoid such suspiciously-looking workarounds?

To this I mostly agree, so I am also inclining to change things to Containers::Optional..

UnsignedInt j = 0;
for(UnsignedInt i = 0; i < importer->object3DCount(); i++) {
auto meshData3D = dynamic_cast<Trade::MeshObjectData3D*>(importer->object3D(i).release());
if(meshData3D) {

This comment has been minimized.

@mosra

mosra Feb 20, 2018

Owner

🤔 What about this instead:

auto objectData = importer->object3D(i);
if(objectData->instanceType() != Trade::ObjectInstanceType3D::Mesh) continue;

Trade::MeshObjectData3D& meshObjectData = static_cast<Trade::MeshObjectData3D&>(*objectData);
  • it avoids dynamic_cast (which is ... 😭)
  • it avoids the ugly release() / delete pair, which can leak if you are not extremely careful
  • the continue avoids one level of indentation of the code below, which is always good :)
  • naming (I got confused here) could it be meshObjectData instead of meshData3D? ;)

Basically the same would be in the meshesCount-calculating loop above.

};
typedef Containers::EnumSet<ShapeLoadType> ShapeLoadTypes;
CORRADE_ENUMSET_OPERATORS(ShapeLoadTypes)

This comment has been minimized.

@mosra

mosra Feb 20, 2018

Owner

What about naming this ConvertShapeType and ConvertShapeTypes to suggest more that this belongs to the convertShapeNode() function?

Also, some docs for the enum, values and typedef would be nice ;)

std::shared_ptr<dart::simulation::World> _dartWorld;
std::unordered_map<dart::dynamics::Frame*, std::shared_ptr<Object>> _dartToMagnum;
std::vector<std::shared_ptr<Object>> _toRemove;
std::unordered_set<std::shared_ptr<Object>> _updatedShapeObjects;

This comment has been minimized.

@mosra

mosra Feb 20, 2018

Owner

Re shared pointers and ownership:

  • The _dartWorld could be simply passed in and stored as dart::simulation::World& -- as far as I understand, it needs to stay alive for the whole lifetime of the World instance anyway, so there's no doubt about ownership
  • I would make one of the arrays/maps/sets an owner of the Object instances -- I think that would be the _dartToMagnum member? Then this one would contain std::unique_ptr<Object> and all the others would be just std::reference_wrapper<Object>
  • All the public accessor functions should return just simple dart::simulation::World& or std::vector<std::reference_wrapper<Object>>, never a std::shared_ptr -- it's not an intention to share the ownership with users of the World object, anyway
{
auto it = _dartToMagnum.find(frame);
_toRemove.emplace_back(it->second);
/* @todo: Manage removal from scene */

This comment has been minimized.

@mosra

mosra Feb 20, 2018

Owner

Not sure if I remember the original discussion -- this is the responsibility of the user, right? I would maybe extend the documentation to explicitly say what's the user expected to do with the unusedObjects() -- I see there's now just a call to a deleteObjectsFromScene() function in the snippet, which is maybe not clear enough.

Maybe I overlooked something -- kick me in that case ;)

auto soft = dart::dynamics::Skeleton::create("soft");
auto bn = addSoftBody<dart::dynamics::WeldJoint>(soft, "soft box");
for(auto& shapeNode : bn->getShapeNodesWith<dart::dynamics::VisualAspect>()) {

This comment has been minimized.

@mosra

mosra Feb 20, 2018

Owner

Minor coding style nitpick: there's never a space before :, so it should be for(auto& shapeNode: bn->.... Similarly in other cases in other files -- you can even safely do find&replace and replace all <space>: with :, I think.

Why I'm saying this -- your adherence to Magnum coding style is now almost totally absolutely perfect (👍) so I won't need to do basically any cleanup afterwards, this is the only remaining thing ;)

refresh();
}

This comment has been minimized.

@mosra

mosra Feb 20, 2018

Owner

The class has some heavy members, I would add an explicit destructor here and = default it in the *.cpp file to speed up the compilation.

Similarly for the constructor -- delegate most of the initialization to a private non-templated constructor that has a definition in the *.cpp file.

namespace Magnum { namespace DartIntegration {
Containers::Optional<ShapeData> convertShapeNode(dart::dynamics::ShapeNode& shapeNode) {
Containers::Optional<ShapeData> convertShapeNode(dart::dynamics::ShapeNode& shapeNode, ShapeLoadTypes loadType, Trade::AbstractImporter* importer) {

This comment has been minimized.

@mosra

mosra Feb 20, 2018

Owner

The importer could be just a reference, not a pointer. Similarly in all functions that call convertShapeNode() -- Object::update() etc.

Update after a discussion (allow calling this w/o using any importer at all):

  • I would update the documentation to say that it's possible to pass in a nullptr importer (and explain the implications of it)
  • I would print some warning on error in case we're loading a Mesh ShapeNode and the importer is not present, instead of only returning NullOpt, so it's clear what happened
@mosra

Last round, hopefully :)

There's still the constructor/destructor stuff and some leftover shared_ptrs that i pointed out in the previous review, though ;)

bool getPrimitive = shape->checkDataVariance(dart::dynamics::Shape::DataVariance::DYNAMIC_PRIMITIVE);
bool getMesh = shape->checkDataVariance(dart::dynamics::Shape::DataVariance::DYNAMIC_VERTICES)
|| shape->checkDataVariance(dart::dynamics::Shape::DataVariance::DYNAMIC_ELEMENTS)
|| shape->checkDataVariance(dart::dynamics::Shape::DataVariance::DYNAMIC);

This comment has been minimized.

@mosra

mosra Feb 22, 2018

Owner

Almost there: these three bools could be moved directly inside the if(get...) conditions below, as they are not used anywhere else

/* If we got here, everything went OK;
* update flag for rendering */
_updatedMesh = firstTime || getMaterial || getPrimitive || getMesh;

This comment has been minimized.

@mosra

mosra Feb 22, 2018

Owner

this could be simply _updatedMesh = !!loadType

std::reference_wrapper<Object> World::objectFromDartFrame(dart::dynamics::Frame* frame) {
return std::ref(*_dartToMagnum[frame]);
}

This comment has been minimized.

@mosra

mosra Feb 22, 2018

Owner

Sorry to sound like a broken record here: you can return just Object& here, no need for the wrapper ;)

Also: since this is a public API at the moment, think about what happens when you ask for a frame that's not in the _dartToMagnum structure: it will insert a new element with such key, default-construct it (so you get nullptr value) and then you dereference it ... 💥

Suggestion:

  • I would use at() instead of operator[], as that does not insert (but rather throw) when not found
  • I would explicitly mention the behavior in the docs
}
} else if(shape->getType() == dart::dynamics::MeshShape::getStaticType()) {
if (!importer) {
Error{} << Debug::boldColor(Debug::Color::Red) << "DartIntegration::convertShapeNode(): AssimpImporter is not available and you are trying to load a dart::dynamics::MeshShape" << Debug::resetColor;

This comment has been minimized.

@mosra

mosra Feb 22, 2018

Owner

Unconditional output coloring will make problems when redirecting output to a text file (or on CIs that don't understand color codes). So I would rather have the boring uncolored output here instead.

@costashatz costashatz force-pushed the costashatz:dart-integration-v2 branch from 48279f2 to f0dc765 Feb 24, 2018

@mosra mosra added this to TODO in Physics via automation Feb 25, 2018

@mosra mosra added this to the 2018.0b milestone Feb 25, 2018

@mosra

mosra approved these changes Feb 25, 2018

@mosra mosra merged commit f0dc765 into mosra:master Feb 26, 2018

2 of 4 checks passed

codecov/patch 45.42% of diff hit (target 55.33%)
Details
codecov/project 46.53% (-8.8%) compared to e9b193d
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Physics automation moved this from TODO to Done Feb 26, 2018

@mosra

This comment has been minimized.

Owner

mosra commented Feb 26, 2018

Merged, thanks a lot. Code coverage including GL tests says 92%, you're doing amazing work 👍 🎉

I made a few small changes to the public API that might break your code, see e3efe6d, 3c3d554 and 7676fdb. In 016c252 I removed a public function that I think was superfluous and never used in any code, I hope that's okay.

Looking forward to the example now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment