gltfpack does not preserve node hierarchy

[demike]

Is there some option to keep the "body" nodes in the image below:

because running gltfpack without parameters results in a flat node list:

this makes it impossible to move the bodies

[zeux]

There's currently no option to preserve the node hierarchy; I can add one. In your file, is it the case that the file itself doesn't contain animations, but you'd like to move the nodes programmatically?

[demike]

yes that' right it's a robot with 3 axis and therfore has 3 "bodies" I move programmatically

[Shimmen]

Somewhat related, I have the problem that meshes are merged, which means that the node hierarchy is not preserved. I need meshes to not merge, since you are supposed to be able to individually change materials for every object from within the application.

I tried changing the canMerge function to always return false, but this consistently causes the application to crash in the destructor of std::vector when leaving reindexMesh.

[zeux]

@Shimmen Yeah this is a related issue; I'm planning to introduce two separate flags, for keeping mesh identity (which would disable mesh merging and keep all meshes in their original place in the hierarchy), and a separate flag for keeping node hierarchy just in case other nodes are relevant.

On an unrelated note, changing canMerge to return false should be safe. Can you attach a mesh that has this issue?

[Shimmen]

That's good to hear!

The meshes I have tested with now are proprietary, so I can't share any of them. I will try to reproduce it on some non-proprietary mesh and get back to you.

[demike]

@zeux it should actually be possible to merge unnamed nodes / meshes. In the first example picture all the child nodes are unnamed and could be merged ( If they have the same material)

[Shimmen]

@zeux I can't manage to reproduce it in any other files, and importing then exporting the "broken" files through Blender results in files that do work. So clearly there is something unique about my files, despite them being valid according to the official Khronos glTF validator.

I had an idea about fuzzing & anonymizing the data and sending it to you, but since I can't even export it without fixing the issue I'm not sure how to proceed.

[zeux]

@Shimmen I've added a couple of assertions to reindexMesh, it would help if you run gltfpack on problematic files in debug mode to make sure no assertions are triggered anywhere.

edit oh actually, upon closer inspection, I suspect 0ccd614 will fix your problem.

[zeux]

The PR should resolve this issue - note that currently it should be sufficient to use km flag because it will keep meshes in their original place in the hierarchy which will also lead to the ancestry nodes being preserved. In the future if gltfpack performs more aggressive node transformations kn flag may become necessary for this as well. Please let me know if this works for you so that I can merge this.

[demike]

Hello the PR works but there is a huge performance hit.
I found a solution that does not have this performance impact but keeps the structure and all named nodes

//	if ( !settings.keep_meshes) 
	{
		for (size_t i = 0; i < meshes.size(); ++i)
		{
			Mesh& mesh = meshes[i];

			if (mesh.node)
			{
				NodeInfo& ni = nodes[mesh.node - data->nodes];

				// we transform all non-skinned non-animated meshes to world space
				// this makes sure that quantization doesn't introduce gaps if the original scene was watertight
				if (!ni.animated && !mesh.skin && mesh.targets == 0 && !settings.keep_meshes)
				{
					transformMesh(mesh, mesh.node);
					mesh.node = 0;
				}

				// skinned and animated meshes will be anchored to the same node that they used to be in
				// for animated meshes, this is important since they need to be transformed by the same animation
				// for skinned meshes, in theory this isn't important since the transform of the skinned node doesn't matter; in practice this affects generated bounding box in three.js
			}
		}

		mergeMeshes(meshes);
//	}

void mergeMeshes(std::vector<Mesh>& meshes)
{
	for (size_t i = 0; i < meshes.size(); ++i)
	{
		Mesh& mesh = meshes[i];
		if (mesh.node && mesh.node->name) {
			continue;
		}

		for (size_t j = 0; j < i; ++j)
		{
			Mesh& target = meshes[j];
			if (target.node && target.node->name) {
				continue;
			}

			if (target.indices.size() && canMerge(mesh, target))
			{
				mergeMeshes(target, mesh);

				mesh.streams.clear();
				mesh.indices.clear();

				break;
			}
		}
	}
}

But with this change it is possible to merge all meshes without unnamed nodes and the same parent
--> Huge performance gain

Original glb file size ~ 5.8MB
keep meshes (your pull request): 3.8MB
only merge unnamed (my modification): 1.3MB
merge all; 1.05MB

[zeux]

Right, I didn't want to use the fact that the nodes are named or not, but it helps to know that in your case the penalty is significant - this means we need to treat only named nodes as unmergeable. The issue is that for the usecase that @Shimmen has merging based on the node name may be dangerous.

@Shimmen, in your case is the application free to change materials on any meshes? Or do they also have to be discoverable through named nodes?

[zeux]

@demike By the way, are you able to share the model that has these results? It looks like models I have now mostly don't suffer severely from the lack of mesh merging, so that would help.

Alternatively, if you can show the output of gltfpack when ran with -v argument maybe I can find something with similar characteristics.

[demike]

The model is exported from siemens nx and converted with three.js
NX generates a material per mesh --> a lot of duplicate meshes so I remove these duplicates
(I can share the removeDuplicateMaterials function if you like)
So here are the stats of '-v':

-km (your pm):
input: 5757 nodes, 4942 meshes, 0 skins, 0 animations
meshes: 57126 triangles, 63929 vertices
output: 10699 nodes, 4942 meshes
output: JSON 3378362 bytes, buffers 1111660 bytes
output: buffers: vertex 768904 bytes, index 342756 bytes, skin 0 bytes, time 0 bytes, keyframe 0 bytes, image 0 bytes

-km (merge unnamed, my modification) (I first remove duplicate materials):
input: 5757 nodes, 4942 meshes, 0 skins, 0 animations
meshes: 57126 triangles, 61327 vertices
output: 1665 nodes, 425 meshes
output: JSON 311238 bytes, buffers 1080436 bytes
output: buffers: vertex 737680 bytes, index 342756 bytes, skin 0 bytes, time 0 bytes, keyframe 0 bytes, image 0 bytes

without -km (first remove duplicate materials first):
input: 5757 nodes, 4942 meshes, 0 skins, 0 animations
meshes: 57126 triangles, 60592 vertices
output: 1 nodes, 1 meshes
output: JSON 18426 bytes, buffers 1071616 bytes
output: buffers: vertex 728860 bytes, index 342756 bytes, skin 0 bytes, time 0 bytes, keyframe 0 bytes, image 0 bytes

without -km (without removing duplicate materials):
input: 5757 nodes, 4942 meshes, 0 skins, 0 animations
meshes: 57126 triangles, 63578 vertices
output: 1 nodes, 1 meshes
output: JSON 2318907 bytes, buffers 1107448 bytes
output: buffers: vertex 764692 bytes, index 342756 bytes, skin 0 bytes, time 0 bytes, keyframe 0 bytes, image 0 bytes

[demike]

By the way:
I get a lot of validation errors with your pm:

            {
                "code": "MESH_PRIMITIVE_ATTRIBUTES_ACCESSOR_INVALID_FORMAT",
                "message": "Invalid accessor format '{VEC3, UNSIGNED_SHORT}' for this attribute semantic. Must be one of ('{VEC3, FLOAT}').",
                "severity": 0,
                "pointer": "/meshes/0/primitives/0/attributes/POSITION"
            },
            {
                "code": "MESH_PRIMITIVE_ATTRIBUTES_ACCESSOR_INVALID_FORMAT",
                "message": "Invalid accessor format '{VEC3, BYTE normalized}' for this attribute semantic. Must be one of ('{VEC3, FLOAT}').",
                "severity": 0,
                "pointer": "/meshes/0/primitives/0/attributes/NORMAL"
            },
...

[zeux]

Right, ok - that makes sense; with lots of tiny meshes merging is essential.

The validator doesn’t recognize the extension used; the extension standardization hasn’t happened yet.

[zeux]

@demike Let's try this one more time? :) I've committed material merging to master - thanks for bringing this to my attention, this ends up being beneficial on some models in my test set - and added a PR #51 that implements -kn flag which preserves named nodes in the output and assumes that named nodes can be transformed later, which roughly follows the logic you had in your changes with a couple minor tweaks that probably won't end up affecting the result much on your test models.

I don't have good test data for this so I wasn't able to test -kn very well, but hopefully this works for your usecase now.

[Shimmen]

Only named nodes show up in the tree view of the application and can therefore be modified. In the files I'm studying all nodes seem to have a name, though.

Now regarding 0ccd614, it seems like it did the trick, and also no assertions are triggered. However, I now get some other crash in getBufferView called by the first writeMeshAttributes in process for the first mesh. It seems as the views vector is empty and when views.size() is called it crashes due to an access violation.

[demike]

@zeux, I tried with -kn and it produces the exact same output size I had with my implementation of material merging and name keeping.
That's really cool!!!

Another example NX-export
Detailed robot model 36019 KB down to 4638 KB

This will actually make NX models usable in the browser (no more 300MB JT2Go three.js models)!!!

[demike]

And of course the resulting node hierarchy is also the same :) ! 👍

[demike]

I might have encountered a problem:

at BufferGeometry.computeBoundingSphere :THREE.BufferGeometry.computeBoundingSphere(): Computed radius is NaN. The "position" attribute is likely to have NaN values.

I use three.js 106 for the test and picking is not working any more because of the bounderySphere error

[zeux]

@demike Ok fantastic - I'll merge the PR. Regarding three.js - yeah this is a bug in three.js, and a bug fix was just submitted yesterday (mrdoob/three.js#16989). Please use GLTFLoader.js from three.js master for the time being - the fix will be released in r107.

[zeux]

For any other problems please feel free to create additional issues. @Shimmen feel free to create a separate issue for the crash - I don't quite see how views.size() can crash but if you post a callstack with a register dump and disassembly view then maybe we can figure this out.

[Shimmen]

@zeux We decided to use the optimization API directly instead of using gltfpack, since it suited our use case better, and everything works fine there. If you would like to pursue this further I can try to help, but otherwise I will let it go. I'm quite sure by now that the files I'm testing with are weird, so it might never happen for other files..

[zeux]

@demike Do the files that you need to optimize have unique material names?

I'm investigating options for preserving material names; I'd like to support this in the "keep named" mode where a named material would not be merged with other materials, but I'm concerned that this change would make your glTF files less optimal - if that's the case I will need to add separate options to do that.

[demike]

Sorry for the late reply, I was on holiday.
I will take a look at it tomorrow

[demike]

Seams like all the materials have unique names