donmccurdy / glTF-Transform

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

Using --pattern with negation on glb's doesn't seem to be possible #1492

Open Kethatril opened 2 weeks ago

Kethatril commented 2 weeks ago

Describe the bug I'm attempting to only process textures in a glb that don't match *comp2 however since there is no texture URI (as its a glb and the textures are embedded) I have to also make it always fail vs an empty string and I can't seem to figure out if that's possible with micromatch.

To Reproduce Steps to reproduce the behavior: 1 Use --pattern with negation (e.g. !*comp2 or any other negative pattern) on a glb with no URI's for textures 2 Notice that every texture is processed including ones where the name ends in comp2

Expected behavior --patterns that contain negation should work as expected on glb's that don't have URI's for textures

Additional context Possibly empty URI's should not be matched against here: https://github.com/donmccurdy/glTF-Transform/blob/23e2bc6c2aae0dd7cdbb1cf589bfa125b6768426/packages/cli/src/transforms/toktx.ts#L211 Or at least not when every texture URI is an empty string.

donmccurdy commented 1 week ago

Interesting difference in how micromatch works when evaluating directly, or creating a regex...

const {isMatch, makeRe} = require("micromatch")

const pattern = '!foo';
const opts = { nocase: true, contains: true };

console.log({
    empty: isMatch('', pattern, opts),
    emptyRe: makeRe(pattern, opts).test(''),
});
// { empty: false, emptyRe: true }

It's possible that's a bug, though I'm not sure which result is expected.

For scripted use, I'd be open to adding callbacks for use like:

await document.transform(
  toktx({
    filter: (texture) => texture.getName().startsWith('foo')
  })
);

But that doesn't really help with CLI use. I'm not sure we can exclude empty URIs automatically without breaking other expectations.

Kethatril commented 1 week ago

Yeah, looks like regex created by matchRe doesn't behave in the same way as isMatch. Seems to be intentional, or at least not something the author thinks is a problem. (Issue from picomatch, which is what micromatch uses for most matching https://github.com/micromatch/picomatch/issues/117 )

For images that point to a bufferView a URI is invalid (according to the gltf spec), it's only an empty string here because the Texture class sets it to that by default. I'm not certain there are any reasons to check against it tbh, it's similar to checking a regex against undefined or null (as that is what an empty string represents in this context)

Currently it always succeeds for patterns with negation (and matches every embedded texture, no matter what the name is) and unless specifically matching an empty string (which means the pattern will always match no matter the name is for embedded textures) it will always fail.

Also because there are 3 cases where the uri can be empty (binary embedded image, data:uri image, or invalid image) I'm not sure matching against it can tell you much of use.