donmccurdy / glTF-Transform

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

CLI "validate --format csv" should output a valid CSV #1251

Closed bobbens closed 9 months ago

bobbens commented 10 months ago

Is your feature request related to a problem? Please describe.

I wish to write a check for pre-commit using gltf-transform validate, however, there are two major issues for using gltf-transform more or less directly:

  1. It always returns 0, even with errors.
  2. csv format is not directly usable as CSV and requires additional parsing.

While it would be possible to write a parser for the CSV format of gltf-transform, it does seem like it's a bit inelegant and won't be very robust.

Describe the solution you'd like

At the very least, have gltf-transform validate return a non-zero value when an error is detected. There could be an argument to determine what severity is counted, but just having non-zero values returned would be a great benefit.

Secondly, ideally have csv output mode not have any extra formatting and be a csv document that you can directly pipe into some csv formatting or load directly in python. For example, on a gltf file I have I get the follow output in csv mode:

 ERROR
 ────────────────────────────────────────────
code,message,severity,pointer
BUFFER_BYTE_LENGTH_MISMATCH,Actual data byte length (456352) is less than the declared buffer byte length (456780).,0,/buffers/0

 WARNING
 ────────────────────────────────────────────
info: No warnings found.

 INFO
 ────────────────────────────────────────────
code,message,severity,pointer
UNUSED_OBJECT,This object may be unused.,2,/meshes/1/primitives/0/attributes/TEXCOORD_0

 HINT
 ────────────────────────────────────────────
info: No hints found.

It should be compressed to:

code,message,severity,pointer
BUFFER_BYTE_LENGTH_MISMATCH,Actual data byte length (456352) is less than the declared buffer byte length (456780).,0,/buffers/0
UNUSED_OBJECT,This object may be unused.,2,/meshes/1/primitives/0/attributes/TEXCOORD_0

Describe alternatives you've considered

Write a custom parser to directly use the output as is, and convert it to some internally usable format, then detect how many errors there were over a certain severity. I don't think it should be too hard to do, but it's likely to not be very robust and prone to errors for something that would make more sense as an upstream feature.

Additional context

Testing output with:

$ gltf-transform validate utils/foo/admonisher.gltf --format csv

 ERROR
 ────────────────────────────────────────────
code,message,severity,pointer
BUFFER_BYTE_LENGTH_MISMATCH,Actual data byte length (456352) is less than the declared buffer byte length (456780).,0,/buffers/0

 WARNING
 ────────────────────────────────────────────
info: No warnings found.

 INFO
 ────────────────────────────────────────────
code,message,severity,pointer
UNUSED_OBJECT,This object may be unused.,2,/meshes/1/primitives/0/attributes/TEXCOORD_0

 HINT
 ────────────────────────────────────────────
info: No hints found.

$ echo $?
0

Using gltf-transform on arch Linux installed with sudo npm install --global @gltf-transform/cli

Sort of a side note, but, in general, the output formats seem to have too much white space.

donmccurdy commented 10 months ago

Thanks @bobbens! I've made these changes in #1252 and published with the latest alpha release of v4. If you'd like to test that:

npm install --global @gltf-transform/cli@next

gltf-transform validate scene.glb --format csv > report.csv
bobbens commented 10 months ago

That was quick! Thanks!

I tried with a gltf file I purposely damaged and got the following output with gltf-transform validate scene.glb --format csv > report.csv

warn: Unable to validate "/home/ess/.local/share/naev/plugins/3dtest/gfx/ship3d/admonisher.bin": Error: ENOENT: no such file or directory, open '/home/ess/.local/share/naev/plugins/3dtest/gfx/ship3d/admonisher.bin'.
warn: Unable to validate "/home/ess/.local/share/naev/plugins/3dtest/gfx/ship3d/base.png": Error: ENOENT: no such file or directory, open '/home/ess/.local/share/naev/plugins/3dtest/gfx/ship3d/base.png'.
code,message,severity,pointer
ACCESSOR_TOO_LONG,"Accessor (offset: 0, length: 1181740) does not fit referenced bufferView [0] length 118176.",0,/accessors/0
MESH_PRIMITIVE_UNEQUAL_ACCESSOR_COUNT,All accessors of the same primitive must have the same count.,0,/meshes/0/primitives/0/attributes/NORMAL
MESH_PRIMITIVE_UNEQUAL_ACCESSOR_COUNT,All accessors of the same primitive must have the same count.,0,/meshes/0/primitives/0/attributes/TEXCOORD_0
IO_ERROR,"Node Exception: Error: ENOENT: no such file or directory, open '/home/ess/.local/share/naev/plugins/3dtest/gfx/ship3d/admonisher.bin'",0,/buffers/0/uri
IO_ERROR,"Node Exception: Error: ENOENT: no such file or directory, open '/home/ess/.local/share/naev/plugins/3dtest/gfx/ship3d/base.png'",0,/images/0/uri

error: Validation detected errors.

Would it be possible to get the first couple of warnings and last error sent to stderr so that only the csv is sent to stdout? Thanks!

donmccurdy commented 10 months ago

Hm, the error handling is handled by Caporal — I would've thought it would write exceptions to stderr. I'll have to look into what I can do there without hacking around it too much.

In case it's helpful as a workaround in the meantime, note that the gltf-transform validate command is a thin wrapper around https://github.com/KhronosGroup/glTF-Validator. The validator has a CLI, and also has an NPM package, but to my understanding they're two different things, and the CLI is not on NPM.

donmccurdy commented 9 months ago

Unfortunately this looks like a bigger surgery on the CLI framework than I can take on. Piping models through glTF Validator is a fairly small part of the glTF Transform CLI functionality. For more control over stdout/stderr and exit codes, it might be best to copy the contents of validate.ts into a dedicated CLI or script for validation:

https://github.com/donmccurdy/glTF-Transform/blob/e4cefb63b96eaf5cc6bb80c075f2e96065eb9351/packages/cli/src/validate.ts#L22-L58