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

Draft for fixing disappearing I3DM models #12105

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

javagl
Copy link
Contributor

@javagl javagl commented Aug 1, 2024

Description

Under certain conditions, parts of models in I3DM could disappear in certain view configurations. This has been reported in several forum threads. Most recently, in https://community.cesium.com/t/disappearing-i3dms-on-map-navigation-again/34035 . It is currently tracked in #11176.

A short ping @bertt : Maybe you want to try this out. It's not yet tested extensively, but might fix the issue.


For the reviewers:

Some details from investigating the reason for the error are summarized in #11176 (comment)

The attempt of a tl;dr summary:

  • The boundingVolume that is associated with a DrawCommand determines whether that command is executed or not (depending on the view configuration)
  • For a glTF model, the bounding volume of the draw command was computed from the PrimitiveRenderResources.positionMin/positionMax properties. These positionMin/positionMax properties already include the instancing translation
  • The bounding sphere that was computed from these values was transformed with the node matrix
  • This means that the contribution of the instancing translation was affected by the node transform

For nodes with different transforms, this had to be inconsistent.

This PR is currently a DRAFT state, also because it contains some debug output. Some of this output - from the example that was posted at the bottom of #11176 (comment) - is shown here to convey the idea. It is an I3DM with 4 instances of a GLB that contains 4 primitives with different sizes that are attached to nodes with different scaling factors.

Before the fix, the output is this:

buildDrawCommand
  positionMin Cartesian3 {x: -5, y: -5, z: 0}
  positionMax Cartesian3 {x: 6, y: 6, z: 1}
  instancingTranslationMin  Cartesian3 {x: -5, y: -5, z: 0}
  instancingTranslationMax  Cartesian3 {x: 5, y: 5, z: 0}
  node scale  1
  instances transformInWorldSpace  true
  radius 7.794228634059948
buildDrawCommand
  positionMin Cartesian3 {x: -5, y: -5, z: 0}
  positionMax Cartesian3 {x: 5.1, y: 5.1, z: 0.1}
  instancingTranslationMin  Cartesian3 {x: -5, y: -5, z: 0}
  instancingTranslationMax  Cartesian3 {x: 5, y: 5, z: 0}
  node scale  10
  instances transformInWorldSpace  true
  radius 71.41953514270448
buildDrawCommand
  positionMin Cartesian3 {x: -5, y: -5, z: 0}
  positionMax Cartesian3 {x: 15, y: 15, z: 10}
  instancingTranslationMin  Cartesian3 {x: -5, y: -5, z: 0}
  instancingTranslationMax  Cartesian3 {x: 5, y: 5, z: 0}
  node scale  0.1
  instances transformInWorldSpace  true
  radius 1.5
buildDrawCommand
  positionMin Cartesian3 {x: -5, y: -5, z: 0}
  positionMax Cartesian3 {x: 6, y: 6, z: 1}
  instancingTranslationMin  Cartesian3 {x: -5, y: -5, z: 0}
  instancingTranslationMax  Cartesian3 {x: 5, y: 5, z: 0}
  node scale  1
  instances transformInWorldSpace  true
  radius 7.794228634059948

The instancingTranslationMin/Max are always the same. They are determined directly from the instancing information.
The computed positionMin/Max show differences that depend on the node scale ...
The most obvious (and relevant) effect is on the bounding shere radius:

  • When the node scale is 1, then the radius is 7.79
  • When the node scale is 10, then the radius is 71.4
  • When the node scale is 0.1, then the radius is 1.5 (which is already smaller than the instancing translations...)

Shown in this screenshot:

screenshotBefore


I considered different approaches for fixing this. One difficulty is...

  • the positionMin/Max do not say whether they are in world space or object space
  • some places already assume that they are in world space (and have to be transformed with the node transform to obtain their "true value")
  • the instancingTranslationMin/Max does not say whether it is in world space. But there is a subtle hint via ModelComponents/Instances.transformInWorldSpace which says that it can be both...

The suggested fix is summarized in this comment.

It's a bit... pragmatic, and having to compute the inverse of the node transform is ... not so nice. But other approaches had a larger potential for messing up other uses of the positionMin/Max (for which I now added a few lines of comments, by the way).

With this fix, the log output is as follows:

buildDrawCommand
  positionMin Cartesian3 {x: -5, y: -5, z: 0}
  positionMax Cartesian3 {x: 6, y: 6, z: 1}
  instancingTranslationMin  Cartesian3 {x: -5, y: -5, z: 0}
  instancingTranslationMax  Cartesian3 {x: 5, y: 5, z: 0}
  node scale  1
  instances transformInWorldSpace  true
  radius 7.794228634059948
buildDrawCommand
  positionMin Cartesian3 {x: -0.7, y: -0.5, z: 0}
  positionMax Cartesian3 {x: 0.4, y: 0.6, z: 0.1}
  instancingTranslationMin  Cartesian3 {x: -5, y: -5, z: 0}
  instancingTranslationMax  Cartesian3 {x: 5, y: 5, z: 0}
  node scale  10
  instances transformInWorldSpace  true
  radius 7.794228634059948
buildDrawCommand
  positionMin Cartesian3 {x: -50, y: -70, z: 0}
  positionMax Cartesian3 {x: 60, y: 40, z: 10}
  instancingTranslationMin  Cartesian3 {x: -5, y: -5, z: 0}
  instancingTranslationMax  Cartesian3 {x: 5, y: 5, z: 0}
  node scale  0.1
  instances transformInWorldSpace  true
  radius 7.794228634059948
buildDrawCommand
  positionMin Cartesian3 {x: -7, y: -7, z: 0}
  positionMax Cartesian3 {x: 4, y: 4, z: 1}
  instancingTranslationMin  Cartesian3 {x: -5, y: -5, z: 0}
  instancingTranslationMax  Cartesian3 {x: 5, y: 5, z: 0}
  node scale  1
  instances transformInWorldSpace  true
  radius 7.794228634059948

One can see that the positionMin/Max now all have to be transformed with the node transform, but then yield the actual, correct result - and it computes the same bounding sphere radius for all cases. The result is shown here:

screenshotAfter

(Yes, I see that the bounding sphere does not really include the whole model - where the mouse cursor is. Should I fix this? Is this my fault? We can talk about that...)

Issue number and link

Addresses #11176

Testing plan

What remains to be investigated is the effect of other instancing transforms. Right now, the transforms only contain translations. I would expect unexpected effects when the instancing transforms contain additional rotations ot scaling. (But... maybe not, if the instanceTranslationMin/Max are already computed properly and taking this into account 🤞 )

For testing: Locally load the tileset (+Sandcastle) that are attached at the bottom of #11176 (comment) , and check whether there is still a view configuration where parts of the model disappear. Maybe also try out some of the examples that had been posted in https://community.cesium.com/t/disappearing-i3dms-on-map-navigation-again/34035 (that's also still on my TODO list)

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with a short summary of my change
  • I have added or updated unit tests to ensure consistent code coverage
  • I have updated the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

(This is a DRAFT state with debug/log output and TODO comments - only for an early review)

Copy link

github-actions bot commented Aug 1, 2024

Thank you for the pull request, @javagl!

✅ We can confirm we have a CLA on file for you.

@bertt
Copy link

bertt commented Aug 2, 2024

Quick n = 1 test with the windmills with rotor animation:

1] Issue windmill disappear on map navigation (without this PR):

Demo: https://bertt.github.io/cesium_issues/i3dm_disappearing2/issue/

2] Testing this PR

Demo: https://bertt.github.io/cesium_issues/i3dm_disappearing2/testing_pr_12105/

Result:

  • Windmills don't disappear 👍

  • Animation rotor still works 👍

@bertt
Copy link

bertt commented Aug 2, 2024

Another testcase I see still an instance disappear (the western one)

https://bertt.github.io/cesium_issues/i3dm_disappearing2/testing_pr_12105_kast/

kast

GLB used is quite simple: https://bertt.github.io/cesium_issues/i3dm_disappearing2/testing_pr_12105_kast/kast.glb

@javagl
Copy link
Contributor Author

javagl commented Aug 2, 2024

Thanks for the quick test and feedback, I really appreciate that 👍

From very quickly skimming/debugstepping over the case with the "blocks" that doesn't work: Yes, in this case, the bounding sphere is still wrong. It doesn't include the second block:

StillWrong

I think that this might be related to some of the points that I already mentioned

  • The I3DM for my simple test only uses POSITION
  • The I3DM for the windmills additionally normals and SCALE, but the scales are all (1,1,1) (!)
  • The I3DM for the blocks also uses normals and SCALE, with the scales being (2,2,2) (!)
  • The I3DM for the blocks contains an RTC_CENTER

These are just observations for now, and I haven't yet identified the reason.


Admittedly, just transforming the instanceTranslationMin/Max with the inverse node transform was a shot from the hip. I already had some doubts (and these doubts become stronger the more I think this through), and they refer to the idea of computing the bounding sphere from instanceTranslationMin/Max in general.

I'll probably need pen+paper to "mathematically confirm" this, but intuitively:

  • Imagine an I3DM with two instances, and POSITION and rotation attributes.
  • The positions are both (0,0,0). So the instanceTranslationMinMax will be (0,0,0) as well. They will not affect the size of the bounding sphere at all
  • Imagine the rotations being
    • 0° around the x-axis
    • 90° around the x-axis
  • Imagine the GLB containing a primitive that has a translation of 100 along the y-axis
  • The rotations will affect the bounding sheres of the primitive instance
  • When only looking at the instanceTranslationMin/Max, this rotation is never taken into account anywhere...

Well ... I could go through this with pen+paper, or just create a test model - and I'm probably lazy and do the latter.
I know that the formal approach could already be a step towards a solution, but my gut feeling is that the solution will have to involve storing the actual transform matrices of the instances ... somewhere, somehow.
(Just like for a ModelInstanceCollection 🤔 )


EDIT:

A minor update: I did check that case of a GLB with translation in an I3DM with all positions being (0,0,0), but different rotations, and the result is as expected: It computes the same bounding sphere for all primitives (with and without the fix from this PR). So... actually fixing this may be a bit more effort. The whole instanceTranslationMin/Max will likely have to be replaced somehow (maybe with the full list of transform matrices, but I'm aware that this would be "bad" from a memory perspective...)

@bertt
Copy link

bertt commented Aug 5, 2024

Another thing to consider: when the used glTF has larger textures.

For example see https://bertt.github.io/cesium_issues/i3dm_disappearing2/issue_textured_trees/ , the used model https://bertt.github.io/cesium_issues/i3dm_disappearing2/issue_textured_trees/tree_textured.glb has 2 textures to visualize the trees

textured_trees

@bertt
Copy link

bertt commented Aug 5, 2024

Btw I'm also wondering what's the purpose of this instancingTranslationMin, instancingTranslationMax compution, as the visibility of the i3dm tile is already defined in tileset.json...

@javagl
Copy link
Contributor Author

javagl commented Aug 5, 2024

@bertt I don't understand in how far textures come into play here. I'd think that the issue is independent of the material of the objects...?

Btw I'm also wondering what's the purpose of this instancingTranslationMin, instancingTranslationMax compution, as the visibility of the i3dm tile is already defined in tileset.json...

I also only started zooming into this. So this attempt to summarize a few things should be taken with a grain of salt:

  • The code is examining each mesh primitive that is attached to a node in the glTF

  • Each node may have instancing information. This instancing information can come...

    • either from an EXT_mesh_gpu_instancing in that node
    • or be instancing information that was created from the I3DM data, and basically "attached to all nodes of the glTF"

    The idea here apparently is that the same mechanism is used for both the EXT_mesh_gpu_instancing and the I3DM instancing. Which leads to the issue mentioned in the forum thread, namely that EXT_mesh_gpu_instancing cannot work properly in I3DMs. It's not yet clear how this could (or should) be addressed...

  • The code that uses the instancingTranslationMin/Max seems to be an attempt to optimize the number of draw calls. Imagine one glTF with a primitive that is attached to a node with a translation of (1000, 0, 0), and that uses instancing. When looking at the origin, then the instanced primitive will not have to be rendered.... unless one of the instance translations is (-1000, 0, 0), because then, this instance would have to appear at the origin (roughly like that...)

The computation of the bounding volumes, and how they affect the execution of the draw commands, will have to be revised. In the context of the investigations here, I noticed that the bounding volumes of the models are not computed properly to begin with (regadless of the instancing). This is now tracked in #12108 . So my "quick shot" in this PR will not be sufficient, but my gut feeling is that when ~"the bounding volume computation" is addressed (properly and holistically), the resulting state could also already include a fix for the I3DM issue...

@javagl javagl mentioned this pull request Aug 11, 2024
6 tasks
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