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

Configuration: ability to use global gltf-transform/cli for commands #865

Closed
hybridherbst opened this issue Mar 21, 2023 · 11 comments
Closed

Comments

@hybridherbst
Copy link

hybridherbst commented Mar 21, 2023

Describe the bug
When using --config and passing in an .mjs file, it's sometimes desired to apply additional steps that are already built-in in gltf-transform. This seems to work trivially in some cases and in others it's unclear to me why it fails.

  • using "prune()" works
  • using "toktx" doesn't work
  • using "meshopt" doesn't work

The latter two produce error: Cannot read properties of null (reading 'getRoot').

To Reproduce
image

I also tried calling await document.transform(someTransform) instead of the array form.

I also tried pushing these into transform "one level up":
image

This produces the same error. Originally I had omitted the ... and got a different error, error: r is not a function.

Versions:

  • Version: 3.1.1
  • Environment: Node.js
@hybridherbst hybridherbst added the bug Something isn't working label Mar 21, 2023
@donmccurdy
Copy link
Owner

@hybridherbst @marwie possibly related to #857. I'm not sure I fully understand the problem and its resolution there, but I suspect it may be dual package hazard, and you'll need to make sure your codebase is consistently using either ESM or CJS and not both.

@donmccurdy donmccurdy added question and removed bug Something isn't working labels Mar 21, 2023
@donmccurdy donmccurdy added this to the Backlog milestone Mar 21, 2023
@hybridherbst
Copy link
Author

@donmccurdy the above is independent of any Needle tools / packages; it's just a single .mjs file loaded via the new --config.

@donmccurdy
Copy link
Owner

@hybridherbst still, the shape of this error strongly suggests an issue related to code compilation and/or Node.js module resolution. No change to this project can prevent dual package hazard, short of dropping CommonJS support entirely. Can you share code rather than screenshots?

@marwie
Copy link
Contributor

marwie commented Mar 21, 2023

@donmccurdy the above is independent of any Needle tools / packages; it's just a single .mjs file loaded via the new --config.

If it helps: part of the change in the linked issue was adding type: module to the package.json (besides the tsconfig changes)

@donmccurdy
Copy link
Owner

Testing so far:

  1. In a new directory with no existing package.json, install dependencies:
npm install @gltf-transform/core @gltf-transform/extensions @gltf-transform/functions @gltf-transform/cli meshoptimizer
  1. Create config.mjs:
import { prune, meshopt } from '@gltf-transform/functions';
import { ETC1S_DEFAULTS, toktx } from '@gltf-transform/cli';
import { MeshoptEncoder } from 'meshoptimizer';

export default {
 onProgramReady: ({ program, io, Session }) => {
     program
         .command('test', 'Custom command')
         .help('Lorem ipsum dolorem...')
         .argument('<input>', 'Path to read glTF 2.0 (.glb, .gltf) model')
         .argument('<output>', 'Path to write output')
         .action(({ args, options, logger }) =>
            Session.create(io, logger, args.input, args.output).transform(
                prune(),
                toktx(ETC1S_DEFAULTS),
                meshopt({encoder: MeshoptEncoder}),
            )
         );
 },
};
  1. Test:
gltf-transform test in.glb out.glb --config ./config.mjs --verbose

I wasn't able to reproduce the problem with those steps yet, with either "type": "module" or "type": "commonjs". I do expect that using "type": "module" is a good default, though.

@hybridherbst
Copy link
Author

hybridherbst commented Mar 21, 2023

I'm basically just continuing experimenting from the state here;
full file:

import path from "path";

import { ALL_EXTENSIONS } from '@gltf-transform/extensions';
import { prune, meshopt } from '@gltf-transform/functions';
import { ETC1S_DEFAULTS, toktx } from '@gltf-transform/cli';
import { MeshoptEncoder } from 'meshoptimizer';

function clearMaterials(options) {
    return async (document) => {
        for (const material of document.getRoot().listMaterials()) {
            material.dispose();
        }
    };
}

export default {
    extensions: [...ALL_EXTENSIONS],
    onProgramReady: ({ program, io, Session }) => {
        // for testing: two commands exposed by the same --config file
        // this one is from the sample and just clears materials
        program
            .command('clearMaterials', 'Clear Materials')
            .help('Removes all materials from the file')
            .argument('<input>', 'Path to read glTF 2.0 (.glb, .gltf) model')
            .action(({ args, options, logger }) => {
                const filename = path.basename(args.input, '.glb');
                const dir = path.dirname(args.input) + '/' + filename;
                const outputPath = dir + ".noMaterials.glb";

                Session.create(io, logger, args.input, outputPath).transform(
                    ...[
                        clearMaterials(options),
                        prune(),
                        toktx(ETC1S_DEFAULTS),
                        meshopt({encoder: MeshoptEncoder})
                    ]
                )
            });
    },
};

Usage:
gltf-transform clearMaterials .\011-1.6.1.glb --config minimalConfig.mjs

Logs:

info: prune: No unused properties found.

error: Cannot read properties of null (reading 'getRoot')

error: Cannot read properties of null (reading 'getRoot')

(I understand this is a stupid sample - clearing materials and then doing toktx - just for the sake of example. You can also remove the toktx line and it will error in meshopt too)

I'm also a bit confused about what additional setup a user needs to do to use --config; in my previous test I think it "just worked" with the global installation of gltf-transform, now I seem to need to do a local installation (+ have a package.json next to the config), kind of defeating the purpose of reducing the steps needed to run something custom, but maybe I'm now doing something wrong.

@hybridherbst
Copy link
Author

hybridherbst commented Mar 21, 2023

Trying to reproduce with your example.
I uninstalled my global gltf-transform 3.1.2 and ran your steps. Step (3) then fails for me with

gltf-transform : The term 'gltf-transform' is not recognized as the name of a cmdlet, function, script file, or
operable program. Check the spelling of the name, or if a path was included, verify that the path is correct and try
again.
At line:1 char:1
+ gltf-transform test in.glb out.glb --config ./config ...
+ ~~~~~~~~~~~~~~
    + CategoryInfo          : ObjectNotFound: (gltf-transform:String) [], CommandNotFoundException
    + FullyQualifiedErrorId : CommandNotFoundException

If I reinstall the global gltf-transform (that I usually use) with npm install -g @gltf-transform/cli and re-run the command, I again get

info: prune: No unused properties found.
debug: prune: Complete.
debug: toktx: Found KTX-Software v4.1.0-rc3.
error: Cannot read properties of null (reading 'getRoot')
error: Cannot read properties of null (reading 'getRoot')

If I only keep clearMaterials(options), prune() I can run using the global gltf-transform without any npm install steps from an empty directory (no package.json, no node_modules) with only the config.mjs file, which I think is the ideal case.

@donmccurdy
Copy link
Owner

I believe you can force the use of the local @gltf-transform/cli by either putting your command in a package.json#scripts entry, or running:

node node_modules/@gltf-transform/cli/bin/cli.js test --config ./config.mjs ...

That said, I haven't been able to reproduce the issue with any of these methods so far.

I guess what we really want is to force the config file to import the same /core and /extensions modules as the (possibly global) /cli module is using. It's not obvious to me how Node.js will be handling these imports, but it seems to be loading two copies of the /core module, and that'll cause problems. Node's experimental loaders API might be a workaround, but that's a while out yet. 😕

@hybridherbst
Copy link
Author

hybridherbst commented Mar 22, 2023

Putting a new command into scripts is a viable workaround!

Here's what I ended up with:

{
  "dependencies": {
    "@gltf-transform/cli": "^3.1.2"
  },
  "scripts": {
    "gltf-transform": "node node_modules/@gltf-transform/cli/bin/cli.js"
  }
}

Can be invoked with:

npm run gltf-transform -- clearMaterials in.glb --config minimalConfig.mjs   

I agree that the goal would be to make the config code run against the modules that come from the /cli module. Unfortunately I don't (yet!) know enough to debug where it's actually pulling them from / how to enforce that.

Out of curiosity, would this maybe be a scenario where npx helps? E.g. running npx gltf-transform ... with a config but without a package.json?
(I tried npx @gltf-transform/cli clearMaterials in.glb --config .\minimalConfig.mjs just for the sake of it, but in its current form it can't find @gltf-transform/extensions)

@hybridherbst hybridherbst changed the title Configuration: ability to append other commands Configuration: ability to use global gltf-transform for commands Mar 22, 2023
@hybridherbst hybridherbst changed the title Configuration: ability to use global gltf-transform for commands Configuration: ability to use global gltf-transform/cli for commands Mar 22, 2023
@donmccurdy
Copy link
Owner

Out of curiosity, would this maybe be a scenario where npx helps? E.g. running npx gltf-transform ... with a config but without a package.json?

Yes, I think the CLI itself can run this way, equivalent to invoking node_modules/@gltf-transform/cli/bin/cli.js directly. It should resolve the local node_modules/ copy of dependencies on behalf of the CLI rather than any global copies. But you'd still need the dependencies installed locally, with a package.json file, I think. And if any bundling or compiling of the config or local extensions is going on, it would need to target compatible ES Module output to avoid the dual package hazard.

@donmccurdy
Copy link
Owner

Closing for now, can reopen if there's more we can do.

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

No branches or pull requests

3 participants