donmccurdy / glTF-Transform

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

Sparse accessor with many non-zero elements (78.0%) may increase file size #1309

Closed marwie closed 8 months ago

marwie commented 8 months ago

Describe the bug I'm getting this warning around 30 times in a row when passing this model through gltf transform NodeIO

To Reproduce My code is roughly like this (i uncommented all processing so this should be enough to reproduce the warnings)

const io = new NodeIO()
        registerExtensions(io);
        io.registerDependencies({
            'meshopt.decoder': MeshoptDecoder,
        })
const document = await io.read(file).catch((err) => {...});
io.write(filename, document);

Versions:

Additional context

image

File that produces this warning Min.zip

donmccurdy commented 8 months ago

@marwie do you feel that the message is inaccurate or shouldn't be a warning? Or just that the logging is too noisy? Note that sparse() will detect these and remove them.

marwie commented 8 months ago

It feels noisy I'm also not sure how/why the percentage rate increases - is this expected? Regarding sparse: would you recommend that I add a call to sparse() by default?

donmccurdy commented 8 months ago

Current behavior is that for each accessor marked as sparse, I/O will log a warning if the % of non-zero values in the accessor is above the threshold at which use of sparse accessors increases final file size:

https://github.com/donmccurdy/glTF-Transform/blob/759bfd7276f9cdee5cacbe3b53aba96f7ed415ea/packages/core/src/io/writer.ts#L271-L275

Perhaps I've got the threshold slightly wrong though, there's a (small) file size increase if we make these accessors non-sparse. In general there's no harm including sparse() by default, it should only change accessors if they benefit. It's included in the CLI 'optimize' command for example:

https://github.com/donmccurdy/glTF-Transform/blob/759bfd7276f9cdee5cacbe3b53aba96f7ed415ea/packages/cli/src/cli.ts#L396

This does require a pass over ever element of every accessor, so if you're really tight on the processing time budget, maybe better to do something targeted only to morph target accessors.

In any case, I'm open to making this logging less noisy.