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

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

Open
javagl opened this issue Apr 17, 2023 · 1 comment
Open

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

javagl opened this issue Apr 17, 2023 · 1 comment

Comments

@javagl
Copy link
Contributor

javagl commented Apr 17, 2023

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:

  • A library for the "structure" classes (related: Consider auto-generating the structure classes #20 ) - that could be useful for other TypeScript users in general
  • Basic operations like the "content type detection" that detect whether given content data is, for example, a B3DM or a PNG file
  • The functionalities for reading/writing 3D Tiles package files (3TZ and 3DTILES)
  • Basic functions for reading/writing tile formats (B3DM, CMPT...)
  • ...

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
Copy link
Contributor Author

javagl commented May 8, 2023

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

  • the new API could have the potential to do "more" (and with more fine-grained control) than the old one
  • the old API functions could be emulated by that (and that already exists, or could be achieved by some "convenience wrappers")

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:

  • Should the class be called TileFormats, or IO or TileDatas or B3dms or ...?
  • Should the method be called readB3dmData, extractB3dm, parse, read, or ...?
  • Is the TileData class final and stable enough to claim that it will exist, without changes, forever?
  • ...

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

Tileset-database conversions

  • Old API:
    • function tilesetToDatabase(inputDirectory, outputFile)
    • function databaseToTileset(inputFile, outputDirectory)
  • New API:
    The convert function from ToolsMain, but in a class that is designated to be an API. As a first shot, this could be a convert method in a class like TilesetConversion in the tilesetProcessing directory.

Extracting information from tile data

  • Old API:
    • function extractB3dm(b3dmBuffer) : object
    • function extractI3dm(i3dmBuffer) : object
    • function extractCmpt(cmptBuffer) : Buffer[]
  • New API:
    • const tileData = TileFormats.readTileData(buffer); (for B3DM, I3DM and PNTS)
    • const tileData = TileFormats.readCompositeTileData(buffer); (for CMPT)

Zipping/unzipping:

  • Old API:
    • function gzipTileset(options)
      With options containing mainly inputDirectory, outputDirectory, gzip=false to unzip (!), and tilesOnly...
  • New API:
    • The existing implementation (as part of a pipeline execution) could be offered as a method, for example:

      class TilesetCompression {
        static async gzip(
          input: string,
          output: string,
          overwrite: boolean,
          tilesOnly: boolean
        ) { ... }
        static async ungzip(
          input: string,
          output: string,
          overwrite: boolean
        ) { ... }
      }

      where the tilesOnly requires most thought:

      • It could remain a boolean (with a meaning that is hard to specify)
      • It could be an included: string[], for example [ CONTENT_TYPE_GLB, CONTENT_TYPE_B3DM ]
      • It could be included/excluded : string[] (both being optional with included=undefined meaning "everyting")
      • It could be a predicate (as it is in the existing implementation), saying whether a certain type:string should be zipped or not.

      Each option can be emulated by the last one (that's why it's currently implemented like that). But users might appreciate more convenient options here...

Pipelines

  • Old API:
    function runPipeline(pipeline, options)
  • New API:
    Let's omit that for now...

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

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

No branches or pull requests

1 participant