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

Feature/tile utils #5

Open
wants to merge 4 commits into
base: mp_main
Choose a base branch
from
Open

Feature/tile utils #5

wants to merge 4 commits into from

Conversation

dbuck
Copy link
Collaborator

@dbuck dbuck commented Jan 4, 2022

[util] - expose isTileDownloadFinished as utility function

  • to allow a way to get overall initial loading progress via traverse

[util] - separate / share-able 3d-tiles -> three conversion utils

  • Maybe this is useful enough to try and push upstream? I'm probably not the only one who will need to translate between three bounds and 3d-tiles bounds?

It seemed slightly nicer to export a set of utility functions from the package as a way to keep the tile internal/cached properties from being relied upon by external package users / exposed via types?

Dave Buchhofer added 4 commits December 28, 2021 12:27
allows a way to get overall progress via traverse
Maybe? So far I have needed to duplicate the majority of this functionality in order to work with my custom extension? Is it too much to export the utility functions from the package as a way to keep internal/cached properties from being relied upon by external package users / exposed via types?
Copy link
Collaborator

@bzztbomb bzztbomb left a comment

Choose a reason for hiding this comment

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

Looks good!

Naming suggestions are little nits, but I think they would be a bit more clear.


// Convert 3d-tiles boundingVolume optional definitions into concrete THREE box/sphere/matrix description
// - ex: used in 'tile.cached'
export function convertTileBoundingVolume( boundingVolume: TileBase['boundingVolume'], transform: Matrix4 ): BoundingVolumeDescriptor;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe this should be convertBoundingVolumeToBox3 to match the function below? Or maybe just boundingVolumeToBox3 to be shorter?

import type { TileBase } from '../base/TileBase';

// Convert optional 3d-tiles transform object into a THREE.Matrix4
export function convertTileTransform( transform: TileBase['transform'], parentMatrix: Matrix4 ): Matrix4;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe tileTransformToMatrix4 is a bit clearer?

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