Skip to content

Conversation

@donmccurdy
Copy link
Member

@donmccurdy donmccurdy commented Jan 2, 2026

Description

While reviewing uses of glTF Pipeline in the CesiumJS codebase, I ran across GltfBuilder.js (a utility in the packages/engine/Specs/ directory). This file is currently unused, with its original use case (ModelOutlineLoader) removed by #10644 in 2022. My feeling is that it's easy enough to restore the file from version history if needed, and that it can't easily be tested/maintained in the meantime, and so could be removed for now.

If the original use case, from #8776, were coming up again tomorrow - another option would be to generate these test files with https://gltf-transform.dev (disclaimer: I'm the author of glTF Transform).

Issue number and link

Testing plan

n/a, removing unused code.

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with a short summary of my change
  • I have added or updated unit tests to ensure consistent code coverage
  • I have updated the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

PR Dependency Tree

This tree was auto-generated by Charcoal

@github-actions
Copy link

github-actions bot commented Jan 2, 2026

Thank you for the pull request, @donmccurdy!

✅ We can confirm we have a CLA on file for you.

@javagl
Copy link
Contributor

javagl commented Jan 2, 2026

I wasn't aware of this class, and have used a ... *clears throat*... "pragmatic approach" occasionally. This could have been avoided by using this class, but ... it doesn't seem to support textures (and even less extensions), so it would need some ... extensions.

More generally: Creating differently-flavored, small glTFs for the specs is something that we should do more extensively (another example where this could be useful). Maybe pulling in glTF-Transform as a dependency (for the specs) could streamline some of that.

@donmccurdy
Copy link
Member Author

donmccurdy commented Jan 2, 2026

Yes, this is roughly the approach I've taken when generating test data for glTF Transform itself. Example generator here:

Generally I think it's nice to have test assets generated at test time. As opposed to static test assets, where it can be easy to end up with test assets that we no longer fully understand or can't easily recreate.

@ggetz ggetz added this pull request to the merge queue Jan 9, 2026
@ggetz
Copy link
Contributor

ggetz commented Jan 9, 2026

Thanks for the cleanup @donmccurdy!

Merged via the queue into main with commit 9e1f702 Jan 9, 2026
5 checks passed
@ggetz ggetz deleted the chore/rm-gltfbuilder branch January 9, 2026 20:59
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.

4 participants