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

feat: Add options to configure pruning unused vertex attributes #279

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

hichemfantar
Copy link

@hichemfantar hichemfantar commented Sep 24, 2024

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

…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.
@donmccurdy
Copy link
Member

donmccurdy commented Sep 24, 2024

Thanks @hichemfantar! I'm OK with these changes, though I think naming the options --keepattributes and --keepleaves might be more in line with the existing flags?

@drcmda any concerns here? Would we prefer that unused UVs be kept by default?

@hichemfantar
Copy link
Author

hichemfantar commented Sep 24, 2024

@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?

@donmccurdy
Copy link
Member

With the naming, it's mainly consistency that I'm worried about:

--keepmeshes
--keepmaterials
--pruneKeepAttributes
--pruneKeepLeaves

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 --transform flag.

@hichemfantar
Copy link
Author

hichemfantar commented Sep 25, 2024

I agree with the naming, i made the changes.
as for the other options, it's to make sure the user has a way to opt out of the default behavior which may not be desirable in all circumstances or even be problematic like it was in my case.

@hichemfantar
Copy link
Author

@donmccurdy is there anything blocking this?

@donmccurdy
Copy link
Member

donmccurdy commented Sep 30, 2024

@hichemfantar the --keepleaves flag does not work; the leaves have already been removed before prune() runs. That's not a trivial fix – please be careful of testing if you feel strongly about including that flag.

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.

@hichemfantar
Copy link
Author

@donmccurdy i reverted back my changes and now only the keepAttributes flag remains

@hichemfantar hichemfantar changed the title feat: Add options to configure pruning unused vertex attributes and empty leaf nodes feat: Add options to configure pruning unused vertex attributes Oct 2, 2024
@drcmda
Copy link
Member

drcmda commented Oct 3, 2024

@donmccurdy no issues with that! but i'm a little out of the loop, do you think it's fine?

@donmccurdy
Copy link
Member

donmccurdy commented Oct 3, 2024

@drcmda yes! I think --keepattributes is a good addition. I would only consider the remaining prune() flags on a case-by-case basis, perhaps in future PRs, they do not all translate to gltfjsx directly.

@hichemfantar
Copy link
Author

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/

@hichemfantar
Copy link
Author

Can we get this merged?
this was recently fixed in three but it's only available for future releases

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

Successfully merging this pull request may close these issues.

Normal attributes are removed with --transform bug: ci workflow broken
3 participants