CesiumGS / 3d-tiles-tools

Apache License 2.0
302 stars 45 forks source link

I3dm to glb upgrade - tree not visible #153

Open bertt opened 1 week ago

bertt commented 1 week ago

Hi,

I'm trying to upgrade a 3D Tileset with 1 I3dm (1 instance on Dam square Amsterdam) to 3D Tiles 1.1 GLB.

Upgrade command used:

npx  3d-tiles-tools upgrade -i tileset.json -o ../glb -f --targetVersion 1.1

Version 3d-tiles-tools (npx 3d-tiles-tools --version): unknown (but installed with npm install 3d-tiles-tools)

With a simple box it works ok:

But with a tree model (https://bertt.github.io/cesium_issues/instance_upgrade/tree/tree.glb) the tree doesn't show up:

Only difference is in the used model. Any ideas how to show the tree in glb format after upgrade?

bertt commented 1 week ago

Notice: When inspecting the resulting tree glb, there are 2 trees instead of 1...

image

javagl commented 1 week ago

Just a quick note: It looks like the I3DM (which works) also contains two trees:

Cesium I3DM to GLB

(nearly at the same position, though).

I'll have a closer look and probably start by logging the positions/TRS that come from the I3DM and go into the GLB

bertt commented 6 days ago

ah yes well spotted, I've updated the tree samples (result tree glb has now 1 tree, but it doesn't show up in Cesium)

javagl commented 6 days ago

Found'em 😑

Cesium I3DM to GLB there they are!

Note that the trees (at the cursor) are floating somewhere below the ground, and it's difficult to navigate to make them visible (the bounding box has to remain visible as well, otherwise, they'll be clipped away...)

Logging did first not bring toooo many insights: The main difference between the trees is a slight difference in the rotation. Their world positions, as they are coming from the I3DM, are basically equal:

worldPositions [
  [ 3887940.5, 332858.03, 5028256 ],
  [ 3887940.5, 332858.03, 5028256 ]
]

What may be relevant here: These positions do include the RTC_CENTER from the I3DM:

  "RTC_CENTER" : [
    3887940.5,
    332858.03,
    5028256
  ]

And for the reasons laid out in Precisions, Precisions: Using such large values for rendering is difficult. Specifically: Writing such values into the TRANSLATION accessor for EXT_mesh_gpu_instancing does not make sense. Doing so would cause distinct instances to "collapse" to the same position, and/or it would cause the well-known "jittering"...

(This is the reason why the RTC_CENTER was introduced to begin with...)

What the I3DM-to-GLB upgrade is doing in an attempt to handle that: Is tries to generically "relativize" the positions/translations. Specifically, referring to this part of the code:

(This is actually similar to what CesiumJS is doing for I3DM internally)

Now there are basically two options:

  1. It could be a precision issue. This seems unlikely. Any precision error should be (unrecognizably) small, given that all the computations are happening with 64bit float.
  2. Something could be wrong with the computation itself This also seems unlikely. The computation is relatively straightforward. The specs do include the InstancedRTC spec from CesiumJS, and this is converted and renderered properly, despite containing the RTC_CENTER. The translation of the root note that is eventually written into the glTF is "translation" : [ 3887940.5, 5028256, -332858.03 ], which is exactly the former RTC_CENTER.

So.... right now, I have no idea why the position of the model may be so wrong... 😕

javagl commented 6 days ago

(Some sort of rubber-duck-debugging here - but also the keep track of some thoughs:)

The TRS properties for the EXT_mesh_gpu_instancing extension are computed as follows:

This inevietably will introduce small errors. I wondered whether even small rounding errors could be emphasized by the node translation that corresponds to the RTC_CENTER. But even when filling the accessors with all identity values, the position seems waaay off. It looks like the root node translation is "wrong", even though it matches the RTC_CENTER, down to the last digit...

bertt commented 18 hours ago

one thing that's special about this tree model: it has a rotation (1/4 pi) in the root node. But I don't see it handled in applyRtcCenter (there a new node is created, the new node gets the translation and the old node is added as a child to the new node). Well it doesn't explain to me the big translation, but might be a pointer.

javagl commented 2 hours ago

Yeah, on the one hand, that tree model is haunting me, due to all the issues (this one, disappearing I3DM parts...), and I think that it "originated" from me casually throwing that into https://github.com/CesiumGS/3d-tiles-samples/tree/main/1.1/MetadataGranularities so I somehow "regret" that. But ... I think that it is good to have a model that "breaks" things, to point out flaws, and increase the robustness.

The issues (both the disappearing I3DM parts, as well as this one) are likely, somehow rooted in the specific node transforms that the model is using. For the disappearing I3DM parts, this is undoubtedly the case (we already figured that out, and drafted solutions). But for the case of the upgrade, any node transform should already be removed. Specifically: The part at https://github.com/CesiumGS/3d-tiles-tools/blob/d549136b9edb5af0a8a6ea105e2e87d782204d07/src/tools/migration/TileFormatsMigrationI3dm.ts#L101 should "bake" all transforms into the mesh, and the resulting glTF should not have any node transforms any more. (Maybe some precision issue is happening there...? I have not yet investigated this much further...)