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

[3d-tiles] Handle GLTF w/ CESIUM_RTC extension #645

Merged
merged 2 commits into from
Feb 11, 2020
Merged

Conversation

Pessimistress
Copy link
Collaborator

Copy link
Collaborator

@ibgreen ibgreen left a comment

Choose a reason for hiding this comment

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

Nice find!

I wonder if it would be a good idea to issue warnings for unsupported 3d-tile extensions, to help us catch such issues.

At least in this case this does not seem to be an optional extension that can just be ignored.

Also I wonder if this is just an old extension from before there was a global property for this, and we are dealing with tilesets that should have been retiled against the latest standard.

(While easy enough to supporting it, ideally there should be a test case as well, so the work adds up...)

@loshjawrence

@@ -12,10 +12,16 @@ import {parse3DTileGLTFViewSync, extractGLTF, GLTF_FORMAT} from './helpers/parse
export async function parseBatchedModel3DTile(tile, arrayBuffer, byteOffset, options, context) {
byteOffset = parseBatchedModel(tile, arrayBuffer, byteOffset, options, context);
await extractGLTF(tile, GLTF_FORMAT.EMBEDDED, options, context);

const {extensions} = tile.gltf;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this go into the function below instead, next to the featureTable.getGlobalProperty('RTC_CENTER', GL.FLOAT, 3);?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The function below is synchronous. This field can only be accessed after the GLTF is loaded.

@coveralls
Copy link

coveralls commented Feb 11, 2020

Coverage Status

Coverage decreased (-3.3%) to 53.941% when pulling bbe0aea on x/rtc-fix into 8671935 on master.

@Pessimistress Pessimistress merged commit 588ff8c into master Feb 11, 2020
@Pessimistress Pessimistress deleted the x/rtc-fix branch February 11, 2020 00:51
@ibgreen
Copy link
Collaborator

ibgreen commented Feb 11, 2020

Oh this is not even a proper 3D tiles extension? It’s actually a gltf extension? Normally we handle those in the glTF loader, but presumably this one will only be used in this context.

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.

None yet

4 participants