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

Webgl2 #3094

Merged
merged 18 commits into from
Oct 15, 2015
Merged

Webgl2 #3094

merged 18 commits into from
Oct 15, 2015

Conversation

lilleyse
Copy link
Contributor

For #797

Added preliminary WebGL2 support. Fixed any incompatibilities between WebGL1 extensions and WebGL2 core, though there are a few extensions that aren't supported right now.

@bagnell
Copy link
Contributor

bagnell commented Oct 14, 2015

  • OES_standard_derivatives is disabled. dFdx, dFdy and fwidth are not recognized when compiling the shader.
  • EXT_frag_depth is disabled. gl_FragDepth is not recognized when compiling the shader (neither is gl_FragDepthEXT).
  • OES_texture_float is disabled. Could not create a texture with PixelDatatype.FLOAT.
  • WEBGL_depth_texture is disabled. Could not attach a depth-stencil texture to a FBO.

@bagnell
Copy link
Contributor

bagnell commented Oct 14, 2015

There should be one test failure for materials because the PolylineArrowMaterial uses standard derivatives.

@pjcozzi pjcozzi mentioned this pull request Oct 14, 2015
22 tasks
@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 14, 2015

@bagnell I added your comments to #797 (comment)

@bagnell
Copy link
Contributor

bagnell commented Oct 14, 2015

Also changes indices to not use the maximum value of the index type. The maximum value of the index type is used for primitive restart. It may also be beneficial for WebGL 1.0 when using ANGLE.

@@ -104,8 +104,8 @@ define([
*/
TerrainProvider.getRegularGridIndices = function(width, height) {
//>>includeStart('debug', pragmas.debug);
if (width * height > 64 * 1024) {
throw new DeveloperError('The total number of vertices (width * height) must be less than or equal to 65536.');
if (width * height >= 64 * 1024) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should also change COLLADA2GLTF and any terrain tools that break up meshes to use 64K - 1 vertices, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess use CesiumMath.SIXTY_FOUR_KILOBYTES here (and anywhere else) like we do elsewhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should also change COLLADA2GLTF and any terrain tools that break up meshes to use 64K - 1 vertices, right?

Right, split them or use unsigned ints.

Copy link
Contributor

Choose a reason for hiding this comment

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

@tfili @abwood see note above about using 64K - 1 vertices when splitting meshes.

Copy link
Contributor

Choose a reason for hiding this comment

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

We are good for models.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why minus 1? You can fit exactly 64k vertices using unsigned short and the comparison is checking the vertex count, not the max index.

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 14, 2015

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 14, 2015

Good start to WebGL 2. Just those comments.

@bagnell
Copy link
Contributor

bagnell commented Oct 15, 2015

@pjcozzi This is ready for another review.

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 15, 2015

@abwood see the WebGL 2 spec:

5.18 PRIMITIVE_RESTART_FIXED_INDEX is always enabled

The PRIMITIVE_RESTART_FIXED_INDEX context state, controlled with Enable/Disable in OpenGL ES 3.0, is not supported in WebGL 2.0. Instead, WebGL 2.0 behaves as though this state were always enabled. This is a compatibility difference compared to WebGL 1.0.

When drawElements, drawElementsInstanced, or drawRangeElements processes an index, if the index's value is the maximum for the data type (255 for UNSIGNED_BYTE indices, 65535 for UNSIGNED_SHORT, or 4294967295 for UNSIGNED_INT), then the vertex is not processed normally. Instead, it is as if the drawing command ended with the immediately preceding vertex, and another drawing command is immediately started with the same parameters, but only transferring the immediately following index through the end of the originally specified indices.

This compatibility difference was introduced in order to avoid performance pitfalls in Direct3D based WebGL implementations. Applications and content creation tools can be adjusted to avoid using the maximum vertex index if the primitive restart behavior is not desired.

ContextLimits._maximumTextureFilterAnisotropy = defined(textureFilterAnisotropic) ? gl.getParameter(textureFilterAnisotropic.MAX_TEXTURE_MAX_ANISOTROPY_EXT) : 1.0;

if (webgl2) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather only assign to each context property once since I'm not 100% sure that all JavaScript engines will optimize (not enter dictionary mode) when we "create" properties in a branch (even though it is in both branches).

Instead, perhaps something like:

var glCreateVertexArray = undefined;

if (webgl2) {
   glCreateVertexArray = // ...
} else {
   glCreateVertexArray = // ...
}

this.glCreateVertexArray = glCreateVertexArray;

Or leave it if someone can confirm the performance.

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 15, 2015

Looks good. Just that comment.

@bagnell
Copy link
Contributor

bagnell commented Oct 15, 2015

@pjcozzi This is ready.

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 15, 2015

With WebGL 1, I get this when opening the CZML Sandcastle example (and perhaps others):

image

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 15, 2015

@bagnell
Copy link
Contributor

bagnell commented Oct 15, 2015

@pjcozzi The jsHint errors are fixed. Fixing them fixed getting the extensions in WebGL 1.0.

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 15, 2015

WebGL 1 looks good.

On WebGL 2 in Canary, should this test fail?

Renderer/Draw draws two instances of a point with different per-instance colors
DeveloperError: instanced arrays is not supported
Error
    at new DeveloperError (http://localhost:8080/Source/Core/DeveloperError.js:43:19)
    at addAttribute (http://localhost:8080/Source/Renderer/VertexArray.js:62:19)
    at new VertexArray (http://localhost:8080/Source/Renderer/VertexArray.js:288:13)
    at http://localhost:8080/Specs/Renderer/DrawSpec.js:1113:14
    at Object.<anonymous> (http://localhost:8080/Specs/spec-main.js:137:30)
    at attemptAsync (http://localhost:8080/ThirdParty/jasmine-2.2.0/jasmine.js:1793:24)
    at QueueRunner.run (http://localhost:8080/ThirdParty/jasmine-2.2.0/jasmine.js:1746:26)
    at QueueRunner.execute (http://localhost:8080/ThirdParty/jasmine-2.2.0/jasmine.js:1732:10)
    at Spec.Env.queueRunnerFactory (http://localhost:8080/ThirdParty/jasmine-2.2.0/jasmine.js:620:35)
    at Spec.execute (http://localhost:8080/ThirdParty/jasmine-2.2.0/jasmine.js:357:10)

@bagnell
Copy link
Contributor

bagnell commented Oct 15, 2015

@pjcozzi I wasn't seeing that failure but it should be fixed.

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 15, 2015

Looks good!

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 15, 2015

Thanks @bagnell and @lilleyse!

pjcozzi added a commit that referenced this pull request Oct 15, 2015
@pjcozzi pjcozzi merged commit 6d8700e into CesiumGS:master Oct 15, 2015
@pjcozzi pjcozzi deleted the webgl2 branch October 15, 2015 17:05
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.

5 participants