Skip to content
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

Prune fails to clean up accessors referenced by non-root properties #1389

Open
donmccurdy opened this issue May 3, 2024 · 2 comments
Open
Labels
bug Something isn't working package:functions
Milestone

Comments

@donmccurdy
Copy link
Owner

Resources like Accessors may be considered 'in use' by prune() if they're referenced by unused primitives or primitive targets. Because no references exist to the primitives or primitive targets, we won't find those and dispose them when iterating over meshes, and the empty accessors are written to disk despite no real use.

The other non-root entity types in the core spec are TextureInfo, AnimationSampler, and AnimationChannel. TextureInfo won't have this issue, but the other two might. Conceivably this could also happen with extensions, but prune() doesn't currently attempt to identify unused extension resources, so that's out of scope for now.

Perhaps it would be better to do a formal tree-shake, iterating all nodes in the graph, and pruning those we can't trace to the Root? This might simplify the current implementation of prune() as well.

@donmccurdy donmccurdy added bug Something isn't working package:functions labels May 3, 2024
@donmccurdy donmccurdy added this to the v4.0 milestone May 3, 2024
@hybridherbst
Copy link

I think doing a full prune of everything could potentially be problematic if there are other unused resources purposefully in the file (e.g. some extra materials).

@donmccurdy
Copy link
Owner Author

Agreed, it should remain at least as granular as it is now:

// Where simplification removes meshes, we may need to prune leaf nodes.
if (options.cleanup) {
await document.transform(
prune({
propertyTypes: [PropertyType.ACCESSOR, PropertyType.NODE],
keepAttributes: true,
keepIndices: true,
keepLeaves: false,
}),
dedup({ propertyTypes: [PropertyType.ACCESSOR] }),
);
}

@donmccurdy donmccurdy modified the milestones: v4.0, v4.1 May 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working package:functions
Projects
None yet
Development

No branches or pull requests

2 participants