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 I3DM to GLB with EXT_mesh_gpu_instancing #52

Merged
merged 29 commits into from
Aug 24, 2023
Merged

Conversation

javagl
Copy link
Contributor

@javagl javagl commented Jul 26, 2023

This PR addresses the task of converting I3DM files to GLB with EXT_mesh_gpu_instancing during the upgrade.

Further details will be added here soon.

- Moving functions from the PntsPointClouds class to the
  TileTableData classes, because they are applicable
  to both PNTS and I3DM feature tables
- Setting up the tests and TilesetUpgrader functions
  for the conversion
- Adding a first draft of the convertI3dmToGlb function
  in the TileFormatsMigration - highly preliminary!
The state here is HIGHLY preliminary, focussing on
getting the right output for all test cases. It will
(have to) be cleaned up in the next iterations.
Split PNTS/B3DM/I3DM migration into dedicated classes.
Tried to get the rotation right in any many cases as
possible...
@javagl
Copy link
Contributor Author

javagl commented Aug 2, 2023

Getting the conversion of I3DM to GLB right is far trickier than it might look at the first glance. There certainly are cases where it is "relatively easy". But there are many possible corner cases, and eventually, even cases where the conversion is not possible in practice.

The cases where the conversion is not possible are related to the fact that in I3DM, the transforms are applied to the whole glTF. With EXT_mesh_gpu_instancing, the translation/rotation/scale of the instancing extension are applied to the mesh primitive, which in turn is affected by the transform of the node that it is attached to.

The current state tries to compensate for that difference: It is computing the inverse of the node transform, and modifying the data for the instancing extensions so that these modifications basically "cancel out" the node transform. This seems to work for the cases for which tests exist in the CesiumJS specs, but these are still structurally simple. Creating further test cases (e.g. an I3DM where the glTF contains a mesh primitive is attached to a node that has a complex transform, involving rotations, translations and scales) could be worthwhile, although even the current spec cases involved a few headaches and caused me to doubt myself. The discussions in CesiumGS/cesium#10255 and related/linked issues/PRs convinced me that it is indeed tricky to get the order and coordinate system conversions right...

In any case: As soon as glTF animations come into play, there will be a limit: The instancing extension can be attached to an animated node and create instances. But it cannot create animated instances.

Points that are still TODO here:

  • External GLBs in I3DMs are not resolved yet (that should be trivial)
  • Batch Table information is not yet assigned (but I hope that is is simple, given the infrastructure that was created already)
  • The instancing extension has to be created multiple times. Namely, once for each group of nodes that have the same global transform and that contain a mesh. That should be doable with reasonable effort.

@javagl
Copy link
Contributor Author

javagl commented Aug 12, 2023

The last commits addressed some of the points that still had been open:

External GLBs in I3DMs are not resolved yet (that should be trivial)

Solved by passing in a function that receives the GLB URI, and returns the buffer with the GLB data.

The instancing extension has to be created multiple times. Namely, once for each group of nodes that have the same global transform and that contain a mesh. That should be doable with reasonable effort.

This was addressed by grouping the nodes of the glTF by having epsilon-equal transforms. The current spec data does not contain such a case, so this has to be created.

Other commits have been for testing the computation of the positions for the instancing extension more thoroughly. For that, I used an I3DM that I created dedicatedly for these tests:

Cesium InstancedAxesSimple

The nodes in this glTF contain non-trivial rotations, translations, and scales, and seem to be converted properly. There is a difference in the lighting that might be caused by differences in the normal computation in the shader, but this has to be investigated separately.

Cesium InstancedAxesSimple

This is added as InstancedAxesSimple spec data. The reason for the name is that there will have to be further test cases that will then have names like InstancedAxesWithRotation, InstancedAxesWithNonUniformScale etc.


Still open:

  • In the current state, there still are cases where the rotations are wrong, but I will try to sort that out based on the ...Axes... samples, which should be far easier than looking at the existing spec examples and guessing which rotations might be wrong for which reason...
  • Batch Table information is not yet assigned (but I hope that is is simple, given the infrastructure that was created already)

@javagl
Copy link
Contributor Author

javagl commented Aug 12, 2023

A small update/addendum: One issue that would come with directly writing the I3DM positions into the extension would be the usual jittering...

Cesium Instanced No RTC

This should not be avoided, by computing the center of the positions, and using it (like the RTC_CENTER) to insert a translation into the glTF root node:

Cesium Instanced With RTC

@javagl
Copy link
Contributor Author

javagl commented Aug 13, 2023

So far the intended approach was to compute the positions/translations/scales of the instancing extensions from the respective property of the I3DM. But there may be cases where this is not possible.

For example: This is an I3DM, where the instance in the lower right is translated about (2,0,0), and rotated about 22.5 degrees around the x-axis.

Cesium I3dm rotated input

The z object (under the cursor) is translated about (2,0,0). But it is also moved down a little bit, due to the rotation that is applied to the whole GLB. When considering the translation and rotation independently, this is the result:

Cesium I3dm rotated output

It is translated. And it is rotated. But the translation that resulted from the rotation of the whole GLB is not taken into account here. In order to create proper glTF instancing extension transforms for each object (i.e. for the axes, the xes, ys, and zs...), one would have to take into account that each part of the I3DM transform (i.e. the translation and rotation and scale) may affect the position that has to be used for the glTF instancing extension transform (and the position may affect the rotation, and the scale may affect everything...). This may seem to be "manageable" with pen and paper, but the implementation would certainly not be trivial.

At some point, it could make sense to consider a completely different approach here - namely, to not ("analytically") compute the proper position/rotation/scale, but to stupidly apply all the transforms of the I3DM to the glTF, one after another, then just 'reverse engineer' the resulting positions/rotations/scale from the resulting (global) node matrices, and then throw away all glTF node transforms and replace them with the instancing extension transforms. This may be costly in some ways (also because it basically requires the extension to use rotation/translation/scale, even in cases where the actual instancing of the I3DM originally only used one of them). But it might be easier and more maintainable. I'll probably give it a try and see how it goes...

@javagl
Copy link
Contributor Author

javagl commented Aug 15, 2023

But it might be easier and more maintainable. I'll probably give it a try and see how it goes...

From a first test, this seems to be orders of magnitude easier. All the manual computations and special cases collapse into a few simple steps:

  • Compute the matrices from the I3DM data
  • Dump the T/R/S of these matrices into the extension accessors
  • "Bake"/collapse all glTF nodes (this is trivial with glTF-Transform) and assign the extension to each of them

Previously, I had played "whack-a-mole" for quite a while, consistently being ""successful"" in coming up with new test cases where the previous approach failed, but all these test cases seem to work with the new approach. (Except for animations, but these are not only difficult, but in many cases impossible to convert anyhow).

I still have to do a cleanup pass, which will remove lots of code, but so far, this looks like a far more reasonable way to go here.

@javagl
Copy link
Contributor Author

javagl commented Aug 23, 2023

The approach of computing the matrices from the I3DM data, splitting them into T/R/S, and applying the results to the "flattened" glTF seems to be far easier. The fact that this completely rules out animations is unfortunate. But in most cases, the GLBs in I3DMs will not be animated, and many forms of animations that could theoretically appear cannot be translated into the instancing extension anyhow.

The last updates included an implementation of EXT_instance_features for glTF-Transform, and using this to represent the data from the I3DM as metadata. There's the small caveat that the instance feature IDs have to be created when no BATCH_IDs are defined, even though this should cause consecutive IDs to be generated automatically. (It is probably not directly related to CesiumGS/cesium#9884 , but may involve similar code paths).

Judging from the TileFormatsMigrationTestSandcastle the migration data directory, all instanced models that are part of the CesiumJS specs (and the new InstancedAxes models that have been created for further tests) seem to be converted properly now.

@javagl javagl marked this pull request as ready for review August 23, 2023 20:11
@lilleyse lilleyse merged commit 6c9c752 into main Aug 24, 2023
2 checks passed
@lilleyse lilleyse deleted the upgrade-i3dm branch August 24, 2023 15:25
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