Fix ArrayBuffer in handle_special_geometry and loading some DAE files

[ManifoldFR]

This PR:

• changes the return signature of merge_geometries to return an array containing the merged geometry and material (instead of adding the material to result)
• fixes handling of multiple materials per group in merge_geometries
• parses the right buffer object when loading DAE files

This PR fixes an issue we encountered with loading DAE geometry files such as these files for the UR5 robot.

Here's the before:

And after:

Here's a minimum example reproducing the downstream problem:

import example_robot_data as erd
import pinocchio as pin
from pinocchio.visualize import MeshcatVisualizer


robot = erd.load("ur5")
model = robot.model
q0 = pin.neutral(model)

colmodel = robot.collision_model
vismodel = robot.visual_model

viz = MeshcatVisualizer(model, colmodel, vismodel)
viz.initViewer(open=True, loadModel=True)

viz.display(q0)

edit: replaced "STL" by "DAE". sorry for the confusion on my end

[jwnimmer-tri]

Thanks for this!

I will take a look at the logic changes & test it later today or tomorrow.

One surface things I noticed on a quick skim, though -- some of the new code here uses abbreviations like mts or bo. Broadly, we try to eschew abbreviation because it makes the code unnecessarily difficult to read. (The thought process is explained a bit more here: https://drake.mit.edu/styleguide/cppguide.html#General_Naming_Rules.) Before this merges, we'll need to ask you to change it to use clearer variable names.

[jwnimmer-tri]

Please also have a look at #138 and see if/how it relates.

[ManifoldFR]

Please also have a look at #138 and see if/how it relates.

I think #138 does make part of this PR redundant (the part where I access the buffer, get the offset and length of the desired slice). The reasoning about the change to msgpack would explain what happened.

[jwnimmer-tri]

I filed #172 to fix the msgpack stuff. I think the best plan is to land that first, and then return here to revisit the question of geometry merging.

[ManifoldFR]

I filed #172 to fix the msgpack stuff. I think the best plan is to land that first, and then return here to revisit the question of geometry merging.

Sounds good to me, I'll keep an eye out. Thanks!

[jwnimmer-tri]

Okay, #172 is merged. I'll wait for a new push here before I test & review the changes.

[ManifoldFR]

Okay, #172 is merged. I'll wait for a new push here before I test & review the changes.

I just rebased my changes. Only thing left is the geometry merging.

[jwnimmer-tri]

If I'm reading the code correctly, the only remaining bug is with DAE file textures, nothing to do with STL files -- is that right? There are a lot of return-value respelling changes here, but the only place I see a functional change is in DAE loading.

I'm going to try to put together a test case that just uses meshcat, without pinocchio & etc. Something like loading one of the UR5 DAE files, but using the standalone-file mechanism of https://github.com/meshcat-dev/meshcat/blob/master/test/meshfile_object_dae.html.

---

So far, in some less-than-rigorous testing, I suspect I'm going to say the same thing here that I did in #139:

When loading with _meshfile_object we don't really need to call ExtensibleObjectLoader and merge_geometries. Instead, we can do what the *.gltf loader already does -- load the *.dae file as a scene and just plop the whole scene tree into three.js, without any geometry merging. In that case, threejs would just show the loaded scene however it got loaded; meshcat doesn't need to get in the way.

I did a quick prototype of that approach, and it successfully loaded some sample DAE files (from the threejs test corpus). It's only like 5 lines of code, something alone the lines of:

--- a/src/index.js
+++ b/src/index.js
@@ -1350,6 +1350,19 @@ class Viewer {
                     configure_obj(scene);
                 }
             });
+        } else if (object_json.object.type == "_meshfile_object" && object_json.object.format == "dae") {
+            let manager = new THREE.LoadingManager();
+            if (object_json.object.resources !== undefined) {
+                manager.setURLModifier(url => {
+                    if (object_json.object.resources[url] !== undefined) {
+                        return json.resources[url];
+                    }
+                    return url;
+                });
+            }
+            let loader = new ColladaLoader(manager);
+            let dae = loader.parse(object_json.object.data, "");
+            configure_obj(dae.scene);
         } else {
             let loader = new ExtensibleObjectLoader();
             loader.onTextureLoad = () => { this.set_dirty(); }

[jwnimmer-tri]

Could you test if #174 solves your problem?

[ManifoldFR]

You're right about the remaining changes, it does something with DAE files.
I think I forgot because this PR comes from rebasing and squashing some commits from our fork. I also got mixed up between UR5 (which uses DAE files) and Solo-12 (which uses STL) in the MWE, sorry about that!

#172 did fix STL and Solo-12.

For #174 I'm getting this on the UR5:

The mesh looks good I think, except for the pose

[ManifoldFR]

Hi @jwnimmer-tri, any news on this PR or #174 ? At this point I think we could go for #174 (after figuring out the pose issue) plus my fix for the handling of multiple materials

[jwnimmer-tri]

The #174 will be the right approach, so that's the one I'd like to aim for.

Either way, we need a test case that fails before the fix, and passes afterwards. (Something in the test folder.) I was hoping to write that myself, but I haven't found the time. If you have time to make a simple test case, that would help move things along.

[ManifoldFR]

The #174 will be the right approach, so that's the one I'd like to aim for.

Either way, we need a test case that fails before the fix, and passes afterwards. (Something in the test folder.) I was hoping to write that myself, but I haven't found the time. If you have time to make a simple test case, that would help move things along.

No problem, I can help out. What would you require from the test, just being able to load a file successfully?

[jwnimmer-tri]

The tests are basically in the style of "open it up in a browser and see if the shape looks okay". If there's something special the human needs to look for in the scene, we can have a comment about that inside the file. Something like "you should see three different colors" or whatever.

We already have meshfile_object_dae.html so possibly you could improve on that case. Or else you could make meshfile_object_dae2.html if it requires something different enough to merit a second case.

The very most important thing is that the test fails with the index.js code on master and passes once the loading bugs are fixed.

(If you like, free to copy the code from #174 into here if that's easier for you to push here.)

[ManifoldFR]

Hi @jwnimmer-tri, I've added a complex Collada test file in #181. It looks good on the current master/HEAD here.

Edit Nevermind, I found a broken Collada file. I'm gonna update the PR #181 accordingly.

[jwnimmer-tri]

If you haven't seen it yet, please read over these links, and check if the #181 test is hitting this particular console warning:

• ColladaLoader object position/center swapped values mrdoob/three.js#24289 (comment)
• ColladaLoader.js#L4093

Perhaps #174 was losing the scene.transform somehow, and we can go back and patch that in correctly.

My general preference remains to try to use three.js (and the meshcat protocol) as intended by their authors, along the lines of #174 -- with _meshfile_object as the message used for textured geometry, and feeding that file data directly to the loader with no reprocessing / merging of geometries after the fact. The merge_geometries will always be fighting an uphill battle to get it right, compared to just using the vanilla loader tools provided by upstream.

[ManifoldFR]

If you haven't seen it yet, please read over these links, and check if the #181 test is hitting this particular console warning:

• ColladaLoader object position/center swapped values mrdoob/three.js#24289 (comment)
• ColladaLoader.js#L4093

Perhaps #174 was losing the scene.transform somehow, and we can go back and patch that in correctly.

I am indeed getting the warning about the Z-UP axis in the console when using #174. The orientation of the meshes is consistent with an error between Y and Z up coordinates I think. I can't figure out how to fix this though. I've tried applying the dae.scene.matrix to the geometries but nothing has worked so far.

My general preference remains to try to use three.js (and the meshcat protocol) as intended by their authors, along the lines of #174 -- with _meshfile_object as the message used for textured geometry, and feeding that file data directly to the loader with no reprocessing / merging of geometries after the fact. The merge_geometries will always be fighting an uphill battle to get it right, compared to just using the vanilla loader tools provided by upstream.

I completely understand. Better to do things "correctly".