donmccurdy / glTF-Transform

glTF 2.0 SDK for JavaScript and TypeScript, on Web and Node.js.
https://gltf-transform.dev
MIT License
1.3k stars 145 forks source link

Join also does a prune inside, with settings that are overly aggressive for certain use cases. #1278

Closed Archimagus closed 4 months ago

Archimagus commented 4 months ago

I have a workflow where I am using empty objects with a custom attribute as placeholders that get loaded and replaced at run time. prune clears them because they end up being empty leaves.

Describe the solution you'd like I would like an option to suppress the clean up step (or override the prune properties)

Describe alternatives you've considered I tried making my own local version of join, but had dependency issues. I also tried detaching and keeping a reference to the empties, and then re-attaching them at the end, but they are disposed and I can't find a way to keep them alive through all the transforms.

Additional context I am also including a PR to add the functionality. It's small, self contained, documented, and doesn't change any default behavior.

donmccurdy commented 4 months ago

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?

Archimagus commented 4 months ago

@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.

Archimagus commented 4 months ago

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.

Archimagus commented 4 months ago

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?

donmccurdy commented 4 months ago

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.

Archimagus commented 4 months ago

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

bhouston commented 4 months ago

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.

bhouston commented 4 months ago

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

bhouston commented 4 months ago

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.

donmccurdy commented 4 months ago

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().

bhouston commented 4 months ago

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.

donmccurdy commented 4 months ago

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.

Archimagus commented 4 months ago

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.

donmccurdy commented 4 months ago

Ah yeah, I do think this must be opt-in — at least until I can do the other work in https://github.com/donmccurdy/glTF-Transform/issues/1278#issuecomment-1971690075. It's a breaking change otherwise. But we can set the default to cleanup: true and that will be fine.

donmccurdy commented 4 months ago

Work in progress:

donmccurdy commented 4 months ago

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