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

feat(ImplicitTiling): add octree support #791

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

AnthonyGlt
Copy link
Contributor

Add octree support for implicitTiling.

TODO:

  • add a method to get directly the type of subdivision or use getBoundsDivider ?
  • test on region octree tileset. I don't have a tileset to try on

@AnthonyGlt
Copy link
Contributor Author

AnthonyGlt commented Oct 7, 2024

@gkjohnson
Is there really a thing to add in the case of a box ?

// Iterate over the three obb axes. Skip the z axis since octree
// implicit bounds are not supported.

Indeed, it looks like the box is not subdvided:

image

(example 3d-tiles-samples/1.1/SparseImplicitOctree/tileset.json )

@gkjohnson
Copy link
Contributor

add a method to get directly the type of subdivision or use getBoundsDivider ?

What do you mean by this?

test on region octree tileset. I don't have a tileset to try on

Looks like 3d-tiles-samples/1.1/SparseImplicitOctree/tileset.json works for this, right?

Is there really a thing to add in the case of a box ?

The loop referenced adjusts the bounding box center and half-axes to describe the specific subdivided child. Note that the array for a bounding box is defined as 3 values for the center, and 3 values for each half-vector on each axis.

Currently the loop for adjusting the bounds does two things - it adjusts the half axes and adjusts the bounding box center by each necessary axis. Currently the loop only iterates over two axes to adjust their scale and the center because a quad tree only splits along X and Y.

So two things need to happen:

  • The loop needs to iterate over all the half-width vectors (ie changing 2 to 3 when using an OCTREE split).
  • And accommodate the z-axis (ie i===2) when computing the axisOffset value so the center can be offset in the z-half-width direction in addition to the x and y offsets currently applied.

Hopefully that makes sense - let me know if anything is still unclear

@AnthonyGlt
Copy link
Contributor Author

Thank you for the answer :D

add a method to get directly the type of subdivision or use getBoundsDivider ?

What do you mean by this?

  • I meant that I'm using this condition tile.__implicitRoot.implicitTiling.subdivisionScheme === 'OCTREE' , could be smarter to use an other method

The loop needs to iterate over all the half-width vectors (ie changing 2 to 3 when using an OCTREE split).

  • Yes, I must have had a brainfreeze to miss that 😬
test on region octree tileset. I don't have a tileset to try on

Looks like 3d-tiles-samples/1.1/SparseImplicitOctree/tileset.json works for this, right?

It's bounding box, I'll modify it to have region then :)

@AnthonyGlt
Copy link
Contributor Author

Screenshot from 2024-10-08 10-14-38

@gkjohnson
Copy link
Contributor

I meant that I'm using this condition tile.__implicitRoot.implicitTiling.subdivisionScheme === 'OCTREE' , could be smarter to use an other method

Yeah it's a little long - maybe a function called "isOctreeSubdivision" that returns true or false would make it more readable.

Looks great! It would be nice to get parent bounds rendering at some point so we can see the full hierarchy a la #730.

Comment on lines 776 to 788
const axisOffset = i === 0 ? tile.__x : tile.__y;
const axisOffset = i === 0 ? tile.__x : ( i === 1 ? tile.__y : tile.__z );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the comment above can be updated.

Comment on lines 747 to 756
if ( tile.__implicitRoot.implicitTiling.subdivisionScheme === 'OCTREE' ) {

const minZ = region[ 4 ];
const maxZ = region[ 5 ];

const sizeZ = ( maxZ - minZ ) / Math.pow( 2, tile.__level );
region[ 4 ] += tile.__z * sizeZ;
region[ 5 ] = region[ 4 ] + sizeZ;

}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good to add some comments here, I think, regarding what this is adjusting and how.

Comment on lines 25 to 26
const z = tile.__implicitRoot.implicitTiling.subdivisionScheme === 'OCTREE' ?
2 * parentTile.__z + ( Math.floor( tile.__subtreeIdx / 4 ) % 2 ) : 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interestingly just setting z to "0" here would make all the octtree / quadtree logic "just work" even if we don't have conditions for the two tree types 🤔. Maybe worth making a comment for later

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that the 0 value even matters, it could be whatever. In the case of a quadtree, the Z value is not used in any calculus and is called only at the url parsing, but there wont be a "z" coordinate in a quadtree url

@AnthonyGlt AnthonyGlt marked this pull request as ready for review October 8, 2024 13:55
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