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

glTF 1.0.1 uint32 indices #4249

Merged
merged 6 commits into from
Nov 11, 2016

Conversation

lasalvavida
Copy link
Contributor

@lasalvavida lasalvavida commented Aug 29, 2016

Part of #4009. Merge #4248 first.

This was referenced Aug 29, 2016
@lasalvavida lasalvavida changed the title Gltf 1.0.1 uint32 indices glTF 1.0.1 uint32 indices Aug 29, 2016
@lasalvavida lasalvavida changed the base branch from master to gltf-1.0.1 August 30, 2016 14:38
@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 30, 2016

CC @lexaknyazev - glTF 1.0.1

@@ -1533,6 +1533,17 @@ define([

///////////////////////////////////////////////////////////////////////////

function loadGLExtensions(model, context) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no need for this and this level of the Cesium stack should avoid making direct GL calls, see http://cesiumjs.org/2015/05/15/Graphics-Tech-in-Cesium-Architecture/

The uint32 extension is initialized here: https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Renderer/Context.js#L277

Model should check Context.elementIndexUint and throw a RuntimeError if the model wants to use the uint32 extension, but the system does not support it.

@lasalvavida
Copy link
Contributor Author

Updated

@lasalvavida
Copy link
Contributor Author

@pjcozzi Can you look at this when you get a chance?

1 similar comment
@lasalvavida
Copy link
Contributor Author

@pjcozzi Can you look at this when you get a chance?

@@ -3115,6 +3115,7 @@ define([
var context = frameState.context;
var scene3DOnly = frameState.scene3DOnly;

checkSupportedGlExtensions(model, context);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move this call to be next to checkSupportedExtensions in update?

@@ -3475,6 +3476,21 @@ define([
}
}

function checkSupportedGlExtensions(model, context) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you document this exception in the update function reference doc? Also add the exception from checkSupportedExtensions which is missing.

if (extension !== 'OES_element_index_uint') {
throw new RuntimeError('Unsupported WebGL Extension: ' + extension);
} else if (!context.elementIndexUint) {
throw new RuntimeError('OES_element_index_uint WebGL extension is not enabled.');
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps 'The OES_element_index_uint WebGL extension is required for this glTF model, but it is not supported by this system'

context._elementIndexUint = false;
return loadModel(boxUint32Indices).otherwise(function(e) {
expect(e).toBeDefined();
context._elementIndexUint = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Save and restore the original value; it could be false on some systems in theory.

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 10, 2016

Just those comments. Sorry for the delayed review.

@lasalvavida
Copy link
Contributor Author

lasalvavida commented Nov 11, 2016

Updated. Please merge #4636 which pulls master into the gltf-1.0.1 branch first. #4636 is merged, this is ready.

@pjcozzi pjcozzi merged commit 95e541e into CesiumGS:gltf-1.0.1 Nov 11, 2016
@pjcozzi pjcozzi deleted the gltf-1.0.1-uint32-indices branch November 11, 2016 17:03
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