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

Upgrade generalization #41

Merged
merged 86 commits into from
Jul 21, 2023
Merged

Upgrade generalization #41

merged 86 commits into from
Jul 21, 2023

Conversation

javagl
Copy link
Contributor

@javagl javagl commented May 22, 2023

This PR addresses the generalization steps of the upgrade command. This addresses #5 , with the building blocks that are supposed to be implemented here being summarized in this comment. Further details will be added here when the PR is updated.

@javagl
Copy link
Contributor Author

javagl commented May 23, 2023

The first steps for the migration will be

  • convert the BatchID to EXT_mesh_features
  • convert PNTS to GLB

For the conversion of the BatchID to EXT_mesh_features, I created some basic support for the EXT_mesh_features extension in glTF-Transform, but this is not (yet) part of this PR - it may have to undergo more tests and reviews.

For the conversion of PNTS to GLB, my current line of thinking is that there should be some functionality to conveniently read positions, normals and colors from PNTS, and shove them into something that eventually generates a GLB. First drafts for that are in the (preliminary, raw, undocumented) TileFormatsMigration class. The basic idea is that one can obtain iterators for the PNTS data as in

    const positions = PntsData.createPositions(featureTable, binary);
    const normals = PntsData.createNormals(featureTable, binary);
    const colors = PntsData.createColors(featureTable, binary);

These will always offer the positions+normals as 3 float values, and the colors as 4 float values, regardless of the quantization, oct-encoding or things like RGBA565.

These things can then be added to something that is currently called GltfPointCloudBuilder, as in

      for (const normal of normals) {
        pointCloudBuilder.addNormal(normal[0], normal[1], normal[2]);
      }

Actually creating the GLB data from that data could be done in a different class, but that may be decided later.

Some thoughts and doubts have been revolving around the question of the types that are used: Should these be number[] arrays, or [number,number,number] arrays, or should there be a typedef for the latter, or should all this operate on Cartesian3 to begin with...? All that is not yet sorted out. The current state is only a snapshot.

Drafts for point cloud interfaces.

Iterables should preferably be only iterable (and not
IterableIterator), and they should be iterable multiple
times.
@javagl
Copy link
Contributor Author

javagl commented May 24, 2023

The last update contains DRAFTS for some basic classes like PointCloudReader and PointCloudWriter, but there are a million degrees of freedom for the design, and many of them still have to be sorted out. The high-level functionality for converting PNTS to GLB that is implemented based on these classes so far is that...

  • POSITION, POSITION_QUANTIZED
  • NORMAL, NORMAL_OCT16P
  • RGBA, RGB, RGB565
  • CONSTANT_RGBA (global)

are extracted, and can be written into a GLB in an "un-encoded" form (i.e. all VEC3/VEC4 FLOAT attributes). Compressing that GLB in various forms should be doable with glTF-Pipeline as a standalone step, with little effort.

Support for BatchID will require an integration of the EXT_mesh_features extension, which is still WIP.


As something that is not directly related to this PR, but came up in this context:

Previously, several functions worked with IterableIterator instances. But ... long story short, they messed that up: As the name suggests, IterableIterator is a mix of an Iterable and an Iterator, and while there are seemingly appealing reasons why something should be both, mixing these things does not make sense (at all - not even a little bit). Specifically, all these IterableIterator instances could only be iterated over once.
Now, instead of working with IterableIterator, the implementation uses Iterable, and these Iterable instances are iterable multiple times. It may require more review and test coverage, but I'm sure that this will avoid lots of confusion in the long run. It's probably a good idea to make sure that an Iterable is iterable...

Minor cleanups and generalizations
Added basic BatchID-to-feature ID support
Draft of EXT_mesh_features support for glTF-Transform
Adjust naming of EXT_mesh_features extension.
Rename point cloud interfaces.
Be VERY explicit about sRGB vs. linear RGB.
@javagl
Copy link
Contributor Author

javagl commented May 30, 2023

A basic implementation of EXT_mesh_features for glTF-Transform has been added. The structures for reading point cloud data (from PNTS right now) have been generalized a bit. Basic support for reading Draco-compressed point clouds has been added. So it is now possible to read nearly arbitrary point clouds, regardless of aspects like quantization, oct-encoding or draco, and generate a ReadablePointCloud (which contains the data is a standardized "in-memory" form) that can be converted into a GLB.

Further conversions (like compressing that GLB) can probably be implemented easily with glTF-Transform, but that still has to be implemented/tested/integrated. Also, the input for now accepts most (all?) information from the Feature Table - but the Batch Table will have to be translated into EXT_structural_metadata, and there is no support for that yet.

@javagl
Copy link
Contributor Author

javagl commented Jun 11, 2023

The previous commits focussed on implementing support for EXT_structural_metadata in glTF-Transform. It would probably have been better to extract this as an isolated task, and not implement it along that update-generalization. But the most important parts of the generalized upgrade - namely, converting the legacy tile formats into glTF+Extensions - requires an implementation of these extensions. It could also have been possible to just manually twiddle the extension into the raw, untyped JSON. But having an "object oriented layer" on top of that might be more convenient for potential users (and maybe less error-prone ... for the users, because bugs will then rather be our fault). An example of this abstraction layer for EXT_mesh_features is shown in this demo, but the EXT_structural_metadata extension is far (!) more complex, and will have to be aligned with the current metadata/binary implementation that is used for 3D Tiles.

@javagl
Copy link
Contributor Author

javagl commented Jul 12, 2023

The latest changes focussed on further integration and testing ... and smaller issues that have been discovered during the tests.

The upgrade command now accepts a --targetVersion X.Y argument (with 1.0 being the default).

For all versions:

  • The asset version will be set to the target version
  • Content that uses a 'url' will be upgraded to use 'uri'
  • The 'refine' value will be converted to be in all-uppercase.

For 1.0:

  • glTF 1.0 models in B3DM or I3DM will be upgraded to glTF 2.0.

For 1.1:

  • The '3DTILES_content_gltf' extension declaration will be removed
  • PNTS and B3DM content will be converted to glTF

The last one is the most critical one (more details below).


One issue that I stumbled over was (of course) the y-up-vs-z-up one. The point positions are stored in z-up. But glTF expects y-up. It was not entirely clear what's the best place or way to handle this. One could just insert a new transform in the root node of the glTF for this, or modify the point positions. For now, I'm modifying the point positions, but maybe one could make a case for handling that differently.


Regarding the tile format migration:

The tests for this are currently based on the CesiumJS Specs/Data/Cesium3DTiles and PointCloud tilesets. An initial draft of the tests only converted the tile content files individually. But I now decided to apply the full upgrade to these tilesets. So the tests will operate on these files, in the specs/data/migration/ directory, and they will

  • read the tilesets from input/
  • write the upgraded versions to output/
  • Write the glTF JSON parts to output_gltf/
  • Compare these JSON parts to what is contained in golden_gltf/

(When there are differences, then these have to be examined, and potentially be commited as part of the golden_gltf).

As an additional layer for testing, I added a Sandcastle (and a README.md) in the migration directory. When serving the migration directory (after running the tests - i.e. when the output directory was filled), then this sandcastle allows selecting the respective tilesets and compare the input and output:

Cesium Migration 0001

Yes, this GIF (intentionally) shows one of the cases that is "not working": The opacity information from the PNTS in the RGBA case seems to be lost. (But on closer inspection, it is there in the GLB - so that might be a CesiumJS issue...). Another difference is visible e.g. for quantized positions, but they look like a plain precision issue due to the RTC_CENTER. Other differences will have to be analyzed in a similar manner, to see whether they are bugs in the migration, or in CesiumJS, or just something that cannot sensibly be migrated at all.

Updated for alpha handling for point clouds
and y-up-vs-z-up transforms
Using BLEND only when non-opaque alpha components are present.
Applying quantization when input is quantized.
Try to be a bit clearer that "name" usually refers to
the property name, and "ID" to the name AFTER it was
sanitized with Ids.sanitize.
For proper handling of unicode property names.
@javagl
Copy link
Contributor Author

javagl commented Jul 16, 2023

The last commits have mainly been focussing on fixes for issues that had been identified by visually inspecting the migration results, using the sandcastle as described in the previous comment. This included

  • Proper handling of alpha values in point clouds: When the point cloud may contain alpha values, then the colors are checked and if they indeed do contain non-opaque values, then the alpha mode of the output GLB is set to BLEND (instead of the default OPAQUE)
  • When the input point cloud is quantized (including oct-encoded normals as a special form of quantization), then the output should be quantized as well. There is no direct correspondence to the NORMAL_OCT16P normals. But using glTF-Transform as a tremendously convenient black box here, the approach now is: If the input positions/normals are quantized, then the output will use KHR_mesh_quantization for the positions/normals, respectively, with 16-bit quantization for both.

A very high-level summary of the additions of this PR is actually given by the last commit with the update of the implementation notes, and this may be an "entry point" for some sort of review.

@javagl
Copy link
Contributor Author

javagl commented Jul 17, 2023

The last commits focussed on cleanups, fixes from local tests, and an update pass for the demos.

I'll mark this as 'Ready For Review', because there shouldn't be any significant functional additions be added to this state. (Minor updates of documentation (e.g. CHANGES.md) or fixes that fall out of further tests may be added, but no implementations of new features). In terms of the actual functionality and infrastructure, the main goals that have been outlined in #5 (comment) should be achieved with the current state of this PR.

Some points (e.g. upgrading I3DM to GLB+Extensions, Batch Table Hierarchies, or handling CMPT) are still open, but should be tackled as dedicated PRs. (This one already did grow a bit too large anyhow). Further steps would involve defining the pre- and post conditions and configuration options of the upgrade command more precisely, and increasing the test coverage based on the decisions there.

@javagl javagl marked this pull request as ready for review July 17, 2023 17:04
@lilleyse
Copy link
Contributor

I won't be able to give this as deep a review as I would like, but it seems like the last things we talked about offline have been ironed out (BLEND mode and quantized positions) so I'm good with merging this. Thanks for the huge amount of work here @javagl.

@lilleyse lilleyse merged commit 66d841d into main Jul 21, 2023
2 checks passed
@lilleyse lilleyse deleted the upgrade-generalization branch July 21, 2023 20:57
@lilleyse
Copy link
Contributor

Any doc changes like updating CHANGES.md could happen prior to publishing a new version to npm.

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.

2 participants