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

VRM gets treated as glTF instead of GLB #1390

Open
marwie opened this issue May 3, 2024 · 11 comments
Open

VRM gets treated as glTF instead of GLB #1390

marwie opened this issue May 3, 2024 · 11 comments
Labels
Milestone

Comments

@marwie
Copy link
Contributor

marwie commented May 3, 2024

Describe the bug

Applying e.g. draco compression to a .vrm file results in all resouces being externalized. The VRM is then saved as a glTF json (with the vrm extension)

According to https://github.com/vrm-c/vrm-specification/blob/master/specification/0.0/README.md however the VRM is a GLB format

To Reproduce
Steps to reproduce the behavior:

  1. Download zip
  2. Run gltf-transform draco "AvatarSample_A.vrm" "AvatarSample_A.vrm"
  3. See unpacked png textures

Expected behavior
VRM gets saved as GLB

Versions:

  • Version: 3.10.1
  • Environment: node20

Additional context

AvatarSample_A.zip

Before
image

After
image

@marwie marwie added the bug Something isn't working label May 3, 2024
@donmccurdy donmccurdy added feature New enhancement or request package:extensions package:core and removed bug Something isn't working labels May 3, 2024
@donmccurdy donmccurdy added this to the v4.1 milestone May 3, 2024
@donmccurdy
Copy link
Owner

donmccurdy commented May 3, 2024

I'm going to retag this as a feature, the library currently knows nothing about .vrm or other third-party variants of .gltf/.glb. But I think I agree that this should be made to 'just work' for the .vrm file extension (other than not understanding the VRM extensions embedded in the file).

I suspect it's mostly a change in the CLI, for script-based usage, io.writeBinary works.

Related:

@donmccurdy
Copy link
Owner

donmccurdy commented May 3, 2024

Also, probably the library should log a warning when it's asked to write a file extension it doesn't know the format of (json vs. binary). Perhaps with some way to register new extensions and their format. Like:

import { Format } from '@gltf-transform/core';

io.registerFileSuffix('vrm', Format.GLB);

@hybridherbst
Copy link

I think another option could be if the check "is this a binary or a text file" is based on the GLB magic header instead of relying on file extensions (which could possibly not exist at all for entirely different reasons).

@donmccurdy
Copy link
Owner

donmccurdy commented May 3, 2024

True – that'd work for the CLI, though not in the script-based I/O case, which may not read from disk. Not sure if something like ...

gltf-transform cp blob_no_extension other_blob

... is worth trying to guarantee same binary vs. json output, I think I'd prefer to log a warning that the library doesn't know what you mean.

@hybridherbst
Copy link

hybridherbst commented May 4, 2024

which may not read from disk

Maybe I'm misunderstanding what you mean – the magic header for GLB ("glTF") is there no matter if it's just a blob or read from disk.

@donmccurdy
Copy link
Owner

donmccurdy commented May 4, 2024

In this scenario...

const document = io.read('path/to/input.unknown');

...

await io.write('path/to/output.unknown', document);

... there is nothing in document to tell the I/O class whether the original source on disk was JSON or binary, so no guarantees can be made that it will be written in the original format without user/application input.

@marwie
Copy link
Contributor Author

marwie commented May 5, 2024

It's that what @hybridherbst means: take notice of the original file header in the io class and - if using write and if the format is unknown then use that information to write as either json or binary?

@donmccurdy
Copy link
Owner

donmccurdy commented May 5, 2024

Understood, but I don't think that I agree with the suggestion. Creates open-ended issues:

  • what happens when two documents are combined?
  • what happens when a different I/O instance is used to read than to write?
  • what about .writeJSON / .readJSON, which supports both .glb/.gltf but doesn't get to see the magic header?
  • if buffers are added to the file, must they be merged, or do we convert from .glb to .gltf?

I'm OK with making changes to ensure .vrm is handled like .glb, and perhaps allowing registration of other extensions... but I do think that unknown extensions should trigger warnings rather than attempting auto-detection, which I don't think can be supported consistently.

@hybridherbst
Copy link

hybridherbst commented May 5, 2024

Do I understand right that the complexity here comes from the fact that

looks at the string path only, and it's not easy to add "look at the first 4 bytes to check what it actually is" since the actual file reads currently happen later / after that decision?

@donmccurdy
Copy link
Owner

From my perspective the complexity comes from trying to keep a persistent knowledge of what the original source-on-disk of a particular Document might have been. It's 'easy' at an application level, if your pipeline looks exactly like this:

const document = await io.read('in.unknown');

// ... make some edits ...

await io.write('out.unknown', document);

But at the library level, I don't want to make an assumption that the pipeline above is what's happening. Documents can be merged or cloned, applications might use different I/O classes for reading and writing. I think it's too much magic, when the alternative is much clearer and not particularly complex:

import { writeFile } from 'node:fs';

await writeFile('out.unknown', await io.writeBinary(document));

@hybridherbst
Copy link

hybridherbst commented May 5, 2024

OK, understood! I think I looked in the wrong place, what you meant with "adding custom filetypes" is probably that the ".vrm is to be treated as a glb file" check would have to happen somewhere around here (at the end of the pipeline, not at the beginning).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants