-
-
Notifications
You must be signed in to change notification settings - Fork 325
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
feat: Add options to configure pruning unused vertex attributes #279
base: master
Are you sure you want to change the base?
Conversation
…mpty leaf nodes The code changes introduce two new options to the CLI tool: - `--pruneKeepAttributes`: Determines whether to keep unused vertex attributes, such as UVs without an assigned texture. - `--pruneKeepLeaves`: Determines whether to keep empty leaf nodes. These options provide more control over the pruning process, allowing users to optimize the resulting glTF files based on their specific requirements.
Thanks @hichemfantar! I'm OK with these changes, though I think naming the options @drcmda any concerns here? Would we prefer that unused UVs be kept by default? |
@donmccurdy the new flags are opt in and shouldn't change the existing behavior. as far as naming goes, i just wanted to be explicit but feel free to change as you see fit. I'm also considering adding flags for all the prune options. what do you think? btw do you mind taking a look at #275 while i have your attention? |
With the naming, it's mainly consistency that I'm worried about:
The prune() transform is an implementation detail; to a reader "prune" and "keep" will sound contradictory. I would advise against adding more flags than you're able to spend time testing — the effect of the full pipeline may give different results than the options of any one transform step would suggest. For example, leaf nodes skipped by prune() may nonetheless be removed by flatten(), or other optimizations. ... that said, I'm primarily the maintainer of glTF Transform, and not a frequent contributor on this repository, so I'm hoping others will chime in with preferences about the API and configurability. Personally, I usually pre-process directly with glTF Transform and then run gltfjsx without the |
I agree with the naming, i made the changes. |
@donmccurdy is there anything blocking this? |
@hichemfantar the I'm not planning to merge new features into this repository myself — I am not the right person to approve and merge your PR, someone else will need to do that. |
@donmccurdy i reverted back my changes and now only the keepAttributes flag remains |
@donmccurdy no issues with that! but i'm a little out of the loop, do you think it's fine? |
@drcmda yes! I think |
Forgot to mention that I included some changes to the gitignore for proper yarn support according to https://yarnpkg.com/getting-started/qa#which-files-should-be-gitignored and some markdown linting fixes according to https://github.com/DavidAnson/markdownlint/ |
Can we get this merged? |
@drcmda do you mind taking a look at this? I removed |
The code changes introduce two new options to the CLI tool:
--pruneKeepAttributes
: Determines whether to keep unused vertex attributes, such as UVs without an assigned texture.--pruneKeepLeaves
: Determines whether to keep empty leaf nodes.These options provide more control over the pruning process, allowing users to optimize the resulting glTF files based on their specific requirements.
closes #278
closes #280