-
Notifications
You must be signed in to change notification settings - Fork 33
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
TypeScript rewrite #16
Conversation
Cleanups, basic tests, comments...
Added basic specs for core functionality Ported/updated legacy specs Moved all spec data outuput to spec/data/output Started basic specs for 'upgrade' functionality
The current state is a rewrite based on TypeScript that replicates most of the previous command-line functionality. The supported command line functions are described in the README at https://github.com/javagl/3d-tiles-tools/blob/tools-ts/README.md Implementation notes for the current structure can be found at https://github.com/javagl/3d-tiles-tools/blob/tools-ts/IMPLEMENTATION.md The test coverage (both in terms of specs as well as in terms of manual "integration tests" based on real data sets) has to be increased. There also are some future developments for extended functionality that are already on the radar. These can be described in dedicated issues and handled as dedicated PRs. |
I wonder if the CLI should catch the error and print the message instead of showing a DeveloperError.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some initial comments from looking at the README.
README.md
Outdated
Quantize floating-point attributes and oct-encode normals: | ||
``` | ||
npx ts-node ./src/main.ts optimizeB3dm -i ./specs/data/batchedWithBatchTableBinary.b3dm -o ./output/optimized.b3dm --options -q -n | ||
``` | ||
|
||
To use tileset texture compression, pass the [`texcomp` flags](https://github.com/CesiumGS/gltf-pipeline/blob/main/README.md#command-line-flags) | ||
``` | ||
node ./bin/3d-tiles-tools.js optimizeB3dm -i ./specs/data/Textured/batchedTextured.b3dm -o ./output/optimized.b3dm --options --texcomp.dxt1.enable --texcomp.dxt1.quality=5 --texcomp.etc1.enable | ||
``` | ||
This example optimizes the b3dm and compresses the textures into `dxt1` and `etc1` formats. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A Draco example would be better since these options no longer exist in gltf-pipeline >= 2.0.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed that to be a draco example, but also noticed that there had been some issues with passing the options to gltf-pipeline
: The old version exposed a parseArguments
function that was applied to the "raw" string[]
to obtain the actual options
object. I now tried to "emulate" that, with some quirks (the name is draco.foo
at the CLI, but dracoOptions.foo
in the actual options
object...).
In a test with the example that is now in the README,
npx ts-node ./src/main.ts optimizeB3dm -i ./specs/data/Textured/batchedTextured.b3dm -o ./output/optimized.b3dm --options --draco.compressMeshes --draco.compressionLevel=9
the output indeed was a B3DM where the GLB contained the Draco part, so that seems to work now...
src/main.ts
Outdated
"upgrade", | ||
"Upgrades the input tileset to the latest version of the 3D Tiles spec. Embedded glTF models will be upgraded to glTF 2.0.", | ||
{ i: inputStringDefinition } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should upgrade
be included in the README?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added an example, with the disclaimer that The exact behavior of the upgrade operation is not yet specified....
|
||
* To debug the tests in Webstorm, open the Gulp tab, right click the `test` task, and click `Debug 'test'`. | ||
* To run a single test, change the test function from `it` to `fit`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any building, testing, or generating documentation instructions that should still be included in the README?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I extended the package.json
with the same commands as in the validator, and added notes in IMPLEMENTATION.md accordingly
package.json
Outdated
"homepage": "https://github.com/CesiumGS/3d-tiles-tools", | ||
"repository": { | ||
"type": "git", | ||
"url": "https://github.com/CesiumGS/3d-tiles-tools.git" | ||
}, | ||
"bugs": { | ||
"url": "https://github.com/CesiumGS/3d-tiles-tools/issues" | ||
}, | ||
"main": "index.js", | ||
"engines": { | ||
"node": ">=16.0.0" | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will these fields be added back prior to publishing on npm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@javagl is this roughly what's left after this PR is merged? Anything big I'm missing?
|
Generalize tileset processing for building pipelines
Extended pipeline processing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@javagl Awesome work here, this will be a much better foundation for all our 3D Tiles tooling needs. I was only able to scratch the surface in my review and just left some comments for the README, which you can address and then merge.
In the near future I'm looking forward to the 3D Tiles 1.0 to 1.1 upgrade path and deciding on the public API.
- `gzip`: Apply GZIP compression to all files (with optional filters) | ||
- `ungzip`: Uncompress all files that are compressed with GZIP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do gzip/ungzip also apply to the root tileset.json? Since they're content stages I wasn't sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. We could try to find a better term than "content" here. It's all files/entries of a tileset data set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, this is embarassing... :-( #22
I'm currently trying to see whether it makes sense to convert them into tileset stages. Not only would that avoid confusion, it would also resolve a bunch of questions about the special handling of the tileset.json
when they are supposed to be a "content stage"...
Co-authored-by: Sean Lilley <lilleyse@gmail.com>
An initial draft of a possible 3D Tiles Tools rewrite. Further details will be added here soon.
Update (2023-04-12)
The previous update summarized the main part of the rewrite, with two further PRs still being pending. These PRs have been
They have been reviewed and merged into the
ts-tools
staging branch in the meantime.Similarly, the following, additional PRs have been created, reviewed and merged into the the staging branch:
Both contain further, detailed descriptions of the changes. The README.md and IMPLEMENTATION.md in this branch have been updated to reflect these changes as well.
Previous updates:
Update (2023-03-14) :
The main part here has reviewed, and the summary of that first review is in the comment below. But for many reasons, it probably makes sense to not hastily merge this PR into
main
, but instead, wait until certain anticipated features are implemented, some futher cleanups have been made, and there have been more tests on actual data sets.In the meantime, there are some PRs that are supposed to be added here, using the
tools-ts
branch as a sort of "staging" branch: