Join also does a prune inside, with settings that are overly aggressive for certain use cases. about gltf-transform HOT 16 CLOSED

Yes, I'd like to reduce the nesting of these functions in v4 or a v4.x minor release. The problem at present is that these functions need to clone and modify resources like meshes and materials — a single input mesh might be cloned and then joined into several different batched meshes, for example.

I'd prefer that these functions not leave a lot of garbage lying around in the file, users should not need to remember to use dedup() and prune() at the end of every change, or between each stage of their pipeline.

So probably that requires more careful bookkeeping within these functions, so that they'll clean up only the resources they've touched in some way, without calling out to these 'global' transforms like prune() or dedup().

from gltf-transform.

Hi @Archimagus! I've had similar feedback about quantize() in #1185.

I am a bit nervous about providing options so that these functions (quantize, weld, join, simplify) will leave cruft behind in the file. Nodes are small, but leaving whole vertex attributes around will be costly. Maybe an option with big disclaimers will be fine... but before deciding, I would like to look at the complexity cost of tracking what the functions have touched, and checking only those resources when pruning.

Would you be open to submitting a (failing) unit test for the problem you're hoping to solve — without including a fix yet — adding leaf nodes before running these functions and checking that they're still there afterward?

from gltf-transform.

@donmccurdy

Sure I can add a test for that.

My thinking here is that it would be expected that if use this option, then you should run your own prune at the end, with the options you need.

from gltf-transform.

Modifying joins, prune step to keepLeaves:true makes the test pass.

I was thinking about instead of just bypassing cleanup entirely, about adding optional prune parameters to the join parameters, which would override the default prune behavior if supplied?

Alternatively, maybe prune should have an option to keep nodes that have extra data, as they are not truly empty? But what should the default be? I could see an argument for either way.

from gltf-transform.

As a workaround, while you decide how this should be handled. Do you have any suggestions for how I could tag these nodes in some way so they don't get pruned?

from gltf-transform.

I'm hoping there's a way to have these functions only clean up data they've touched. For example — join() should never get rid of a node that didn't have a mesh to begin with. I do want the cleanup to be as limited in scope as possible, but join() adds leaf nodes so it should be able to clean up the leaf nodes it creates.

If that turns out to be too complicated (keeping track of what has changed) then I think bypassing cleanup as you suggest might be the next best thing. My concern with that is mainly that I'll probably have to deal with more bug reports about it later, for things like:

1. operation X fails if document contains unused accessors
2. operation Y runs slowly because it's processing many unused meshes

Perhaps a short-term workaround would be to put something simple — a camera, light, or a no-op extension — into these nodes before simplifying, then remove the attachment afterward.

from gltf-transform.

Adding a camera to the empty nodes, then removing it at the end seems to do the trick as a workaround.

from gltf-transform.

I am running into this exact same issue. I notice that "weld", "simplify", "reorder", quantize", "partition", "join", "instance", "dedup" and probably others call prune to remove a bunch of stuff. I need leaf nodes specifically for my use case, because I need placeholders for attachment points and gltf-transform just clobbers them.

I would suggest that we do not nest prune inside of these other options, instead require users to specify --prune or something as an additional parameter if they want to prune.

from gltf-transform.

Or we add a new option called --noprune and I can specify it while running other commands to avoid doing unnecessary prunes.

from gltf-transform.

I think that glTF-Trasnform in general is more valuable if it has separable atomic actions that you can mix and match and re-order, rather than having mostly compound actions that do a lot of stuff that you can not control.

from gltf-transform.

Could we in the meantime, could I add a parameter to the functions weld, simplify, reorder, quantize, partition, join, instance, dedup that just disables the global prune operation? I am not using the command line tool, rather calling these functions directly so just adding this as a function option makes sense to me.

from gltf-transform.

Ok, I'm happy with that! Not blocking v4 on the other refactoring might be nice anyway.

I'll be traveling for the next week, and won't get much done in that time. But I'm happy to review PRs like @Archimagus's #1279, and/or failing tests like #1281 are very helpful as well. Or I can look more into this when I'm back home.

Perhaps we should name the option something positive (e.g. "cleanup", not "suppressCleanup") and default to true. Then if it does make its way into the CLI, it will be consistent with the other boolean flags having a --foo vs. --no-foo convention.

add a parameter to the functions weld, simplify, reorder, quantize, partition, join, instance, dedup that just disables the global prune operation?

Minor correction — dedup() doesn't call any other functions. But dedup() is called inside of some of these other functions, and (like prune) should probably be disabled by the cleanup: false setting.

from gltf-transform.

I'm good either way, I just assumed you'd want the defaults to not change behavior. And that the new behavior of skipping cleanup would be Opt In.

For me, using the API I was able to add a camera to my placeholders, and then remove it at the end just before running draco. Kind of hacky but it worked.

@bhouston If you want to take over from #1279 I don't think I'll have time as I've moved on from this at this point.

from gltf-transform.

Ah yeah, I do think this must be opt-in — at least until I can do the other work in #1278 (comment). It's a breaking change otherwise. But we can set the default to cleanup: true and that will be fine.

from gltf-transform.

Work in progress:

• #1300

from gltf-transform.

Published to v4.0.0-alpha.9, thanks @Archimagus and @bhouston!

from gltf-transform.