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

Convert to monorepo #64

Merged
merged 61 commits into from
Feb 2, 2024
Merged

Convert to monorepo #64

merged 61 commits into from
Feb 2, 2024

Conversation

javagl
Copy link
Contributor

@javagl javagl commented Sep 12, 2023

Considering the extended scope and functionality that has been added to the 3D Tiles Tools after its revival, it probably makes sense to split it into multiple packages, to improve maintainability and dependency management.

(I say 'improve', hesitatingly. It does cause efforts on the side of the maintainers. But I think that it is better for consumers to have smaller dependendencies, and there definitely are advantages in having small(er), potentially reusable building blocks, in order to decide what exactly should be exported from each package)

The IMPLEMENTATION.md summarizes the current directory structure of the 3D Tiles Tools. The intention behind this structure is ROUGHLY that each directory constitues something that could be considered to be a "package". But still, the 3D Tiles Tools have been a single package until now.

This PR converts the repository into a monorepo with several packages. I considered two approaches:

  • Start with an empty repo, and add one by one package, incrementally
  • Start with a single package that contains everything, and carve out the smaller packages from that

I took the second approach, meaning that there currently is a package called all that contains everything that has not been split into smaller packages yet. The first packages to be carved out are those that don't have ("internal") dependencies.

At the time of writing this, the packages are:

  • structure: The plain TypeScript classes that reflect the JSON types of 3D Tiles (largely auto-generated from the schema)
  • gltf-extensions: The implementations of EXT_mesh_features, EXT_instance_features, and EXT_structural_metadata (based on glTF-Transform)
  • ktx - The utility classes that are wrapped around the BinomialLLC Basis Encoder WASM module
  • all - The part that is not handled yet (and has all other packages as dependency, except for cli)
  • cli - The command line interface. This only consists of the main.ts/ToolsMain.ts, and has all as its dependency

The rough structure for the next packages could be

  • something like base, with the basic IO, spatial, maybe the 'content type detection', and utility classes
  • something like tilesets with stuff like the traversal and basic tileset processing infrastructure,
  • something like metadata, focussing on the part that can be considered to be the implementation of the 3D Tiles Metadata spec (including binary metadata)
  • something like tileContent, with the classes for B3DM/PNTS/etc

For the final decisions here, the dependencies have to be sorted out more clearly, and some judgement calls will have to be made about what makes a package "sensible" and "coherent".


There are some orthogonal questions that still have to be answered. These are roughly on the level of

  • Which scripts should be in which package.json?
    I think that only the top-level package.json will contain the stuff for linting or coverage. The package-package.jsons usually contain only the build script
  • Where exactly are the specs located, and how are they run?
    The specs are currently still in the top-level specs directory (not handled yet, cannot be run). They will have to be moved to the respective <packageName>/specs directory. (But I'm not sure whether it should be possible to run them for each package, or whether they are also (only) triggered from the top-level package.json )
  • There are quite a few other "infrastructure" questions (e.g. the configuration and running of things like api-documenter) that have to be sorted out

Any feedback is appreciated.

@javagl
Copy link
Contributor Author

javagl commented Sep 15, 2023

A summary of the current state (with some overlap from the previous post, but with some additional detail):

  • structure: The plain TypeScript classes that reflect the JSON types of 3D Tiles (largely auto-generated from the schema).
    The rationale for this package is that it is standalone, and does not offer any functionality. All the classes are just plain structures, acting as "typings" for the JSON structure in TypeScript.

    • Depends on:
      • Nothing
  • gltf-extensions: The implementations of EXT_mesh_features, EXT_instance_features, and EXT_structural_metadata.
    I think it makes sense to offer this as a standalone package, and it might even become a completely standalone one (not under the 3D Tiles Tools umbrella), so that these extensions are available for anyone who uses glTF-Transform

    • Depends on:
      • "@gltf-transform/core"
  • ktx - The utility classes that are wrapped around the BinomialLLC Basis Encoder WASM module
    Encoding things to KTX is a pain in the back. Compiling the BinomialLLC module requires some work with CMake, Emscripten, and for integrating the resulting module in applications. It would really be great to be able to do an npm install thatKtxThingy and then do that result = Magic.encode(imageData) call. The ktx package does exactly that.

    • Depends on:
      • "sharp" (a PNG/JPG loading library, also used in glTF-Transform)
  • base - Basic classes... of which I wasn't sure where else to put them 😕
    This may be the most "controversial" one, because it is the least "coherent" one. It contains some things that are completely unrelated (see the subdirectories in the current state):

    • base: Utilities for paths, buffers, URIs, iterables...
    • binary: A generic representation of the buffers/bufferViews concept, used in subtrees, and glTF
    • contentTypes: The definition of "content types" and functions for detecting the content type from raw data blobs
    • io: Generic IO classes (most importantly, a ResourceResolver, that receives a URI and returns a buffer)
    • logging: The main logger class
    • spatial: Basic classes for quadtrees/octrees

    One could make a case for subdividing that further, or just (preliminarily) claim that nothing in this package should be considered to be "public" or "stable"

    • Depends on:
      • "@3d-tiles-tools/structure"
      • "pino"/"pino-pretty" (the logging library)
  • metadata: Classes that can be seen as an implementation of the 3D Metadata standard.
    This is some sort of "object-oriented abstraction layer" for accessing metadata, regardless of whether it is stored in JSON or binary form. This should already be in a somewhat reasonable shape, but may require some polishing if it is supposed to be endorsed as such.

    • Depends on:
      • "@3d-tiles-tools/structure"
      • "@3d-tiles-tools/base"
  • tilesets: Classes related to tilesets and tile contents.
    This includes the definition of the low-level TilesetSource and TilesetTarget interface, and implementations of these interfaces for 3tz and 3dtiles files, as well as classes for implicit tiling and traversal, and handling of the (legacy) tile formats. This includes classes for reading B3DM, accessing the GLB, accessing feature tables and such. What is ... kind of awkward in terms of the overall structure here: The methods for accessing the feature tables are actually using the metadata classes under the hood, because these are just a generic way of ~"accessing structured (and typed!) data that is stored in binary blobs". (This fact should not be visible in the interfaces, though...)

    • Depends on:
      • "@3d-tiles-tools/structure"
      • "@3d-tiles-tools/base"
      • "@3d-tiles-tools/metadata"
      • "archiver"
      • "better-sqlite3"
      • "cesium"
      • "node-stream-zip"
  • tools: Classes that offer the actual 3D Tiles Tools functionality
    This contains the infrastructure for tileset processing, built on top of the tilesets classes. This involves things like "applying a certain operation (like b3dmToGlb) to each tile content", or the "migration" (upgrade) functionality for tile contents, as well as implementation of the "pipeline" functionality.

    • Depends on:
      • "@3d-tiles-tools/structure"
      • "@3d-tiles-tools/ktx"
      • "@3d-tiles-tools/gltf-extensions"
      • "@3d-tiles-tools/base"
      • "@3d-tiles-tools/metadata"
      • "@3d-tiles-tools/tilesets"
      • "@gltf-transform/core"
      • "@gltf-transform/extensions"
      • "@gltf-transform/functions"
      • "cesium"
      • "draco3d"
      • "gltf-pipeline"
      • "gltfpack"
  • cli - The command line interface.
    This only consists of the main.ts/ToolsMain.ts, parsing the command line and executing the corresponding function from the tools package

    • Depends on:
      • "@3d-tiles-tools/base"
      • "@3d-tiles-tools/tilesets"
      • "@3d-tiles-tools/tools"
      • "yargs"

Some packages (mainly base, tilesets, and tools) will probably undergo some reviews/cleanups, and that may involve moving or renaming a few classes. But I think that the overall packages should already reflect a reasonable separation of concerns.

The orthogonal questions mentioned in the previous comment will be addressed next, most importantly: How to set up the specs and jasmine config and scripts in a reasonable way.

Any feedback is appreciated.

@javagl javagl mentioned this pull request Oct 21, 2023
@javagl
Copy link
Contributor Author

javagl commented Nov 29, 2023

Further steps with attempts to get the tests running are in https://github.com/CesiumGS/3d-tiles-tools/tree/convert-to-monorepo-testing

It is not possible to create a "modern monorepo with TypeScript" and Jasmine tests. There are other test frameworks, and we could start converting hundreds of tests to a new framework, but from what I've read and experienced in the last week, the chance that any of them will be working as intended is zero.

An alternative approach would be to extract the suggested packages into standalone GitHub repositories and publishing them as individual packages. But for now, we'll have to treat the 3D Tiles Tools purely as a command-line application, without an API, and without a separation into useful building blocks.

@javagl
Copy link
Contributor Author

javagl commented Nov 30, 2023

It is not possible to create a "modern monorepo with TypeScript" and Jasmine tests.

I still stand behind that, and would like to see a counterexample. The last updates here at least got the compilation and tests running again - with caveats and quirks. Further details will be added here (and in IMPLEMENTATION.md) soon. In many cases, I'll probably link to StackOverflow Q/As and issues that just illustrate how broken the whole ecosystem is, and conclude with "it has to be the way that it is now, because otherwise, something won't work", but I'll try to summarize the relevant information there, distilled from the hundreds of non-working "solutions" that I found in the past few days.

They all refer to a "root" directory with the data,
and resolving paths depends on where exactly the
npm run test command was issued. Workaround with
jasmine spec helpers and environment variable.
…-merge

# Conflicts:
#	etc/3d-tiles-tools.api.md
#	packages/metadata/specs/metadata/PropertyTableModelsSpecialValuesSpec.ts
#	packages/metadata/src/metadata/binary/BinaryMetadata.ts
#	packages/tools/specs/contentProcessing/GltfTransformSpec.ts
#	packages/tools/src/tilesetProcessing/ContentBoundingVolumes.ts
#	packages/tools/src/tilesetProcessing/OrientedBoundingBoxes.ts
#	packages/tools/src/tilesetProcessing/external/README.md
#	packages/tools/src/tilesetProcessing/external/dito.ts
#	src/metadata/MetadataUtilities.ts
#	src/metadata/binary/BinaryPropertyTable.ts
#	src/metadata/binary/BooleanPropertyModel.ts
#	src/tilesetProcessing/BoundingVolumes.ts
#	src/tilesetProcessing/TilesetJsonCreator.ts
@javagl
Copy link
Contributor Author

javagl commented Dec 14, 2023

Right now, this shows many conflicting files. The reason is, as a rough summary:

  • The convert-to-monorepo was branched off from main
  • There have been changes in main in the meantime
  • The convert-to-monorepo completely changes the directory layout and structure

There is no way to merge that automatically. The changes from main have therefore been transferred into this branch in commit f34e782 . This was done manually - basically by taking each file from the main state at this time, and putting it into the directory that it belongs to according to the monorepo structure. Now, merging the main into this branch basically meant 1. merging main as usual and 2. resolving all conflicts by taking the state from this branch.

All this felt quirky, and maybe someone with better Git foo could have found a "simpler" way, but this seemed to be the only way to avoid losing changes from main and achieve a consistent, new "monorepo" state. The state with main merged into this branch now resides at https://github.com/CesiumGS/3d-tiles-tools/tree/convert-to-monorepo-merge - if nobody has objections to that approach (and nobody can propose a "better" merging strategy), I'm merge that into this branch, which should resolve all conflicts 🤞

Merge main branch into monorepo conversion
@javagl javagl marked this pull request as ready for review December 18, 2023 14:08
It was ONLY used for the logger, which is now
replaced with a simple log callback.
If somebody thinks that there is a way to solve this
without the frowned-upon "noImplicitAny": false,
then: Pull requests welcome.
@javagl
Copy link
Contributor Author

javagl commented Dec 23, 2023

The last updates here:

  • Removed the @3d-tiles-tools/base dependency from the ktx package.
    It was only used for the Loggers class, and this was only logging some "debug" level messages. So now this dependency has been replaced with a simple log callback in the KtxUtility class
  • Added specs for the KtxUtility class.
    This just compares the result of encoding a small image to some "golden" output/reference, once with ETC1S and once with UASTC compression
  • Follow-ups:
    • In order to ~"get the tests running", I had to set "noImplicitAny": false in the tsconfig.json files. I know this is frowned upon. Feel free to open a PR with a better solution.
    • Setting noImplicitAny: false triggered some compilation errors because of types that had not explicitly been declared. In fact, these had been errors that I would have expected to be caught by noImplicitAny being true, but ... TypeScript is a box of chocolates, you never know what errors you get, and for what reason. I think that the fixes make sense, regardess of why or why not a certain error shows up.
  • Updated the README.md and package.json files to use the "ESM workaround" mentioned in Convert to monorepo #64 (comment)

@lilleyse lilleyse merged commit ffa48c3 into main Feb 2, 2024
2 checks passed
@lilleyse lilleyse deleted the convert-to-monorepo branch February 2, 2024 13:51
@javagl javagl mentioned this pull request Feb 17, 2024
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.

None yet

2 participants