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

Fix computation of model bounding volume #12112

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

Conversation

javagl
Copy link
Contributor

@javagl javagl commented Aug 7, 2024

Addresses #12108

Only an early draft for now. Further details will be added here soon.

Description

...

Issue number and link

#12108

Testing plan

...

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

Copy link

github-actions bot commented Aug 7, 2024

Thank you for the pull request, @javagl!

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

@javagl
Copy link
Contributor Author

javagl commented Aug 10, 2024

I'll try to post intermediate updates, even though they are HIGHLY preliminary. I hope that they help to illustrate why all this takes far longer than it should.

Here is the output of a debugging Sandcastle. This is similar to the one that I posted in the issue. But it shows...

  • the bounding volume of the first primitive in gray
  • the bounding volume of the second primitive in red
  • the bounding volume of the whole model in yellow.

For the current main state:

Cesium Bounding Spheres 0001 old

("Hey, there is no 'gray' one! And/or the one for the model is completely wrong! And/or one of the bounding volumes seems to be missing!" - "Yeah. That's the point...")

For the current (highly preliminary) state of this PR:

Cesium Bounding Spheres 0002 newer

It looks "less wrong". In fact, it looks completely correct. Yay! 🥳

But... ... some .... things are ... happening at different places. What is currently shown in that sandcastle are the actual bounding spheres of the model and the primitives. Let's look at the bounding spheres that are stored in the draw commands (dense red grid):

Cesium Bounding Spheres 0003 newer with debug

I could say that "the bounding spheres are wrong", but ... in many cases, they are not even spheres 😬 Tracking down which bounding spheres this are, where they come from, where they are modified, and where they are becoming "wrong" is a challenge. It's easy to point to "some place where this might be happening". But not where it actually is happening. I'll keep digging.

@javagl javagl mentioned this pull request Aug 11, 2024
6 tasks
@javagl
Copy link
Contributor Author

javagl commented Aug 12, 2024

A small update, starting with some comments that I wrote locally in one of the files. I'm trying to summarize this as a form of "rubber duck debugging":


The PrimitiveRenderResources contains positionMin/Max and a bounding sphere. It does not say what these properties ARE - and specifically, which transforms they include or not.

The ModelRuntimePrimitive stores a clone of the bounding sphere of the PrimitiveRenderResources. The bunding sphere is documented as

The bounding sphere of this primitive in object-space.

It is not clear what the "object space" is. Is it the space of the MODEL, or the space of the PRIMITIVE? (I.e. does it include the transform of the node that the primitive is attached to or not?)

The ModelDrawCommand contains a bounding sphere that is computed as
drawCommand.boundingSphere = runtimePrimitive.boundingSphere * drawCommand.modelMatrix
This will actually be updated in the setter of the ModelDrawCommand.modelMatrix, which is called as part of ModelMatrixUpdateStage.updateRuntimeNode. The ModelDrawCommand.modelMatrix at this point includes the transform
of the node that the primitive is attached to (so it is not really a modelMatrix, but rather a primitiveMatrix)


The bunding volumes that are computed right now include

  • the instancing transforms (if present)
  • the nodeRenderResources.model.sceneGraph.components.transform
  • the nodeRenderResources.model.sceneGraph._axisCorrectionMatrix
  • the runtimeNode.computedTransform

and are "correct" in terms of the visual output. But but when they are part of the drawCommand and will later be multiplied with the modelMatrix, which also includes the runtimeNode.computedTransform, then the result will be wrong. Still, they have to be transformed with a modelMatrix. But one that does not include the node transform. Whether or not it has to include any other elements of this transform (axis correction, components.transform) remains to be investigated.

I'm tempted to either

  • remove the 'node transform' part from the ModelDrawCommand.modelMatrix (but this will likely break a thousand other places)
  • rename the ModelDrawCommand to PrimitiveDrawCommand (because that's what it is), and pull through the refactoring steps that this entails (but I cannot foresee what this would mean, and it may introduce other bugs...)

tl;dr The questions of 'Which bounding sphere includes which transform?' and 'Which matrix contains which transform?' are hard to answer.

@javagl
Copy link
Contributor Author

javagl commented Aug 26, 2024

These "broken bounding volume visualizations" are actually tracked in #5684 (from 2017...)

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.

1 participant