CesiumGS / 3d-tiles-tools

Apache License 2.0
295 stars 44 forks source link

Decide on the library usage, packaging, and API #21

Open javagl opened 1 year ago

javagl commented 1 year ago

This may have to be broken down into smaller issues at some point, but some context is summarized here:

Much of the functionality of version 0.2.0 of the tools was built by extracting "generic" functionality from the 3d-tiles-validator. Nearly all of this functionality is exposed by the 3d-tiles-tools on an API level, because it is supposed to be used by the validator, as an internal project. Therefore, all of the API is marked as @internal in the documentation.

The overarching question is now: What should be made 'public', and how?

This refers to the API level for each part, but also to the structure of the libraries. There are some "low-level" functionalities that are basic enough so that one could justify offering them as a dedicated library. (Note: The directory structure in the current state of the tools already reflects a possible way to break down the tools into smaller libraries....). Such libraries could be, for example:

In many ways, this boils down to the question of the intended granularity of the libraries.

For each library itself, there's still the quesition about the exact API.

(From my Java backround, I'm a fan of the purist approach, which can be summarized as: "No public constructors, period". But I'm not (yet) sure whether this is considered to be "idiomatic" in TypeScript).

The importance of this question is already apparent: The 3d-tiles-validator is supposed to use nearly everything of the 3d-tiles-tools. So the tools will be a dependency of the validator. On the other hand, it would make a lot of sense to use the validator to check whether input/output tilesets of the tools are really valid, meaning that the validator would be a dependency of the tools. This shows that there is a line that should be drawn, but it's not yet clear where to draw this line.

javagl commented 1 year ago

The current state of the (non-) API of the 3D Tiles Tools is: Everything is exported, but nothing is public.

This is caused by the fact that the current state of the 3D Tiles Tools has largely been implemented as internal (non-public) functionality within the 3d-tiles-validator, but carved out to be a standalone library later. Everything is exported because the validator needs (nearly) everything from the tools. Nothing is public because none of that was designed to be a completely public library.

But even though it was not explicitly designed to be a public library, the possibility for things to become public had been kept in mind during the development: The project already is divided into "packages", namely by the directory structure. Each directory contains certain classes that are candidates to become part of a public API. A description of the overall project structure, including the most important classes for each "package", can be found in the implementation notes.

It's difficult to come up with a proper API definition without a clear idea about what people might expect to be offered by the tools. But one can start, based on a mix of 1. knowledge about what is required by the 3d-tiles-validator, 2. a gut feeling about ~"what might be useful", and 3. some ideas that may be derived from the ./demos/.

These initial thoughts can then be refined, iteratively.


Level 1: Restore the previous API

Before the 'TypeScript rewrite', the 3d-tiles-tools (version 0.1.3) offered an API for the the library usage:

module.exports = {
    databaseToTileset : require('./lib/databaseToTileset'),
    extractB3dm : require('./lib/extractB3dm'),
    extractCmpt : require('./lib/extractCmpt'),
    extractI3dm : require('./lib/extractI3dm'),
    glbToB3dm : require('./lib/glbToB3dm'),
    glbToI3dm : require('./lib/glbToI3dm'),
    gzipTileset : require('./lib/gzipTileset'),
    runPipeline : require('./lib/runPipeline'),
    tilesetToDatabase : require('./lib/tilesetToDatabase')
};

These functions resemble a subset of what is offered as command line commands. (They do not exactly correspond to the command line functions, but some details will be discussed below). Command line commands that have not been supported are combine, (merge), upgrade, optimizeB3dm, optimizeI3dm. The ungzip operation is implemented with gzipTileset({ gzip = false }) ...

A similar API could be offered based on the current state, for people who have a dependency to the 0.1.3 version, and want to use the newer version. However, this should probably be marked as deprecated to begin with.

The API could be offered based on the ToolsMain class, which summarizes exactly the functions that correspond to the command line commands. (Note: The ToolsMain class was not intended as an API - quite the contrarary: It is only the internal implementation of the CLI functionality. But as such, it could serve as a suitable entry point of "emulating" the old API here).

Whether or not this API can be exactly compatible in terms of importing and behavior of the functions would have to be sorted out. I think that users of the old API did write something like

import { gzip } from '3d-tiles-tools';
gzip(...).then(...);

And one could probably define a compatible API by adding

import { ToolsMain } from "./ToolsMain";
export function gzip(...) {
    return ToolsMain.gzip(...);
}

to the index.ts. Possible re-routings (like forwarding databaseToTileset to the new convert method and such) could probably happen there as well.


Level 2: Provide an API that is equivalent to the old one

There is no public API for the tools yet, but some things that may become an API. And in many ways...

Then, in the first step, for clients, one of the largest differences would be that of "namespaces" (aka "classes") that are introduced, and ... that there are types.

Disclaimer: If someone has feedback about the best practices in TypeScript here, then drop your thoughts here. Most of what is about to be exported as the first new API will be based on (static) functions that resemble the functions from the old API. Whether or not that is "good" or "bad"? I don't know. I have seen things like the statement in this stackoverlow answer, which says

In practice, I find that static methods usually only appear for two reasons: (1) whoever wrote the code is used to Java [...]

and that's a clear "Yes" from my side, but that doesn't imply that it should be solved differently...

One example: The old API had a function extractB3dm, and users could call it like this

import { extractB3dm } from '3d-tiles-tools';
...
const b3dm = extractB3dm(b3dmBuffer);
...

A similar function exists in the TileFormats class now. So users could call

import { TileFormats } from '3d-tiles-tools';
...
const b3dm = TileFormats.readTileData(b3dmBuffer);
...

and this functionality is equivalent, insofar that the returned object contains the same information as before, with a slightly different structure of the returned value. (Specifically, the returned object has the type TileData).

An API that is equivalent to the old one

The exact way of how the new API should be exposed is not yet clear, and this is where the bazillion questions can come up. Even just referring to the above example:

But some high-level ideas about functions that could be "equivalent" to the old API:

Tileset-database conversions

Extracting information from tile data

Zipping/unzipping:

Pipelines

Level 3+: Further thoughts about the API

One important thing - even if it sounds trivial at the first glance - is whether the API operations should work on files or objects.

For certain operations, it's relatively easy: There could be aconvertB3dmToGlb function that receives a B3DM Buffer and returns a GLB Buffer. One could trivially wrap this into a convertB3dmFileToGlbFile method that just does the fs.readFileSync/writeFileSync and internally calls the Buffer version.

(Even though there's already the question: Should both of them be public, and offered by the same class?)

But for other things it's not so easy. For example, the convert function (that emulates tilesetToDatabase and databaseToTileset), might have input:string, output:string parameters, denoting file names. But in the future, it might be desirable to not have to operate on files. The functionality could be offered on the TilesetSource and TilesetTarget objects. Similarly to the Buffer/readFileSync/writeFileSync case, the file-based implementation could then be a trivial wrapper, as in

doItWithFiles(input: string, output: string) {
  souce = open(input);
  target = open(output);
  doItWithObjects(source, target);
  close(input);
  close(output);
}

Several existing classes have a tendency to use file names (e.g. in the pipeline package, because the pipeline JSON contains strings). But some of them could benefit from generalizing that to TilesetSource/TilesetTarget, be it at least as another option, but still offering the "wrapper" that accepts file names...