-
-
Notifications
You must be signed in to change notification settings - Fork 35.3k
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
WebGLRenderer WebGL2.0 initial support with GLSL 1 to 3 runtime conversion #13717
Changes from 28 commits
d1d38c9
63839ee
b17356f
24e0ec4
b3af65a
b1f0db4
a04bea6
5392545
2b0dac6
cf3ff01
32cb672
2456569
6065f4f
f02117f
c1ff8fc
7a4dc58
fd7ced8
949623b
89c1738
42831e8
51dad3f
e11ed2b
ddc4893
4ec384f
b53cec2
5219c0f
3f0dfd9
062d7db
8ca782f
325d8e9
3cdf35b
dc11ded
bdbc68b
46ee5b6
ab65cb7
acee27d
f696296
d177df5
158fc6c
fda6fac
a18d1a2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -65,7 +65,8 @@ function WebGLRenderer( parameters ) { | |
_antialias = parameters.antialias !== undefined ? parameters.antialias : false, | ||
_premultipliedAlpha = parameters.premultipliedAlpha !== undefined ? parameters.premultipliedAlpha : true, | ||
_preserveDrawingBuffer = parameters.preserveDrawingBuffer !== undefined ? parameters.preserveDrawingBuffer : false, | ||
_powerPreference = parameters.powerPreference !== undefined ? parameters.powerPreference : 'default'; | ||
_powerPreference = parameters.powerPreference !== undefined ? parameters.powerPreference : 'default', | ||
_webgl2 = parameters.webgl2 !== undefined ? parameters.webgl2 : false; | ||
|
||
var currentRenderList = null; | ||
var currentRenderState = null; | ||
|
@@ -190,11 +191,15 @@ function WebGLRenderer( parameters ) { | |
_canvas.addEventListener( 'webglcontextlost', onContextLost, false ); | ||
_canvas.addEventListener( 'webglcontextrestored', onContextRestore, false ); | ||
|
||
_gl = _context || _canvas.getContext( 'webgl', contextAttributes ) || _canvas.getContext( 'experimental-webgl', contextAttributes ); | ||
var webglVersion = _webgl2 && typeof WebGL2RenderingContext !== 'undefined' ? 'webgl2' : 'webgl'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you think we should try to create WebGLContext if we fail to create WebGL2Context? (I don't) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can avoid a fallback if the user explicitly requests WebGL2 with a parameter. Compatibility can be verified on app level. Maybe we can enhance Detector in order to support the user with this task. Something like: if ( Detector.webgl2 ) // use WebGL 2 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added. Thanks! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with @Mugen87 I think that if we explicitly request WebGL2 context and it's not available, it shouldn't fallback to webgl1. Probably someone requesting There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added two options. We may want autoWebgl2 as the default once Three.js + WebGL 2.0 becomes stable (See #13701 (comment)) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would just leave one option. Something like:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Combining the two options into a single one is nice. But I don't really like mix type, boolean or strings. So how about
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @takahirox yeah! I like that approach better 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated. |
||
|
||
_gl = _context || _canvas.getContext( webglVersion, contextAttributes ); | ||
|
||
if ( _gl === null && webglVersion === 'webgl' ) _gl = _canvas.getContext( 'experimental-webgl', contextAttributes ); | ||
|
||
if ( _gl === null ) { | ||
|
||
if ( _canvas.getContext( 'webgl' ) !== null ) { | ||
if ( _canvas.getContext( webglVersion ) !== null ) { | ||
|
||
throw new Error( 'Error creating WebGL context with your selected attributes.' ); | ||
|
||
|
@@ -206,6 +211,8 @@ function WebGLRenderer( parameters ) { | |
|
||
} | ||
|
||
_gl.isWebGL2 = typeof WebGL2RenderingContext !== 'undefined' && _gl instanceof WebGL2RenderingContext; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wouldn't pollute the webgl context. What about adding a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the problem is that certain modules of the renderer like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I was thinking about it too, in any case the change will be small, just replacing the WebGLcontext parameter by WebGLRenderer and access the context from the renderer inside the module. Not strong opinion either, but If we pollute the webgl context, should we open the door to practices like babylon's #13717 (comment) ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup, I was gonna comment the same thing about no reference to And also, tbh I don't really wanna pollute There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think? @mrdoob |
||
|
||
// Some experimental-webgl implementations do not have getShaderPrecisionFormat | ||
|
||
if ( _gl.getShaderPrecisionFormat === undefined ) { | ||
|
@@ -2427,7 +2434,7 @@ function WebGLRenderer( parameters ) { | |
|
||
}; | ||
|
||
this.setRenderTarget = function ( renderTarget ) { | ||
this.setRenderTarget = function ( renderTarget, optType ) { | ||
|
||
_currentRenderTarget = renderTarget; | ||
|
||
|
@@ -2469,7 +2476,7 @@ function WebGLRenderer( parameters ) { | |
|
||
if ( _currentFramebuffer !== framebuffer ) { | ||
|
||
_gl.bindFramebuffer( _gl.FRAMEBUFFER, framebuffer ); | ||
_gl.bindFramebuffer( optType || _gl.FRAMEBUFFER, framebuffer ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thinking to move this The purpose of this PR is to enable existing core to run on WebGL 2.0 rather than to add WebGL2.0 features. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep I agree with that approach There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reverted. This change should be discussed well in another PR. |
||
_currentFramebuffer = framebuffer; | ||
|
||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,7 +21,17 @@ function WebGLExtensions( gl ) { | |
switch ( name ) { | ||
|
||
case 'WEBGL_depth_texture': | ||
extension = gl.getExtension( 'WEBGL_depth_texture' ) || gl.getExtension( 'MOZ_WEBGL_depth_texture' ) || gl.getExtension( 'WEBKIT_WEBGL_depth_texture' ); | ||
|
||
if ( gl.isWebGL2 ) { | ||
|
||
extension = gl; | ||
|
||
} else { | ||
|
||
extension = gl.getExtension( 'WEBGL_depth_texture' ) || gl.getExtension( 'MOZ_WEBGL_depth_texture' ) || gl.getExtension( 'WEBKIT_WEBGL_depth_texture' ); | ||
|
||
} | ||
|
||
break; | ||
|
||
case 'EXT_texture_filter_anisotropic': | ||
|
@@ -41,7 +51,26 @@ function WebGLExtensions( gl ) { | |
break; | ||
|
||
default: | ||
extension = gl.getExtension( name ); | ||
|
||
if ( gl.isWebGL2 && | ||
[ 'ANGLE_instanced_arrays', | ||
'OES_texture_float', | ||
'OES_texture_half_float', | ||
'OES_texture_half_float_linear', | ||
'OES_element_index_uint', | ||
'OES_standard_derivatives', | ||
'EXT_frag_depth', | ||
'EXT_shader_texture_lod', | ||
'EXT_blend_minmax', | ||
'WEBGL_draw_buffers' ].indexOf( name ) >= 0 ) { | ||
|
||
extension = gl; | ||
|
||
} else { | ||
|
||
extension = gl.getExtension( name ); | ||
|
||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Returning For example, Three.js devs would think as if WebGL 2.0 also has ANGLE_instanced_arrays extension although it's built-in feature in WebGL 2.0 with this code.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I made branch, removing the workaround from dev...takahirox:WebGLRendererWebGL2.0_2 The code will be
Change will be a bit bigger, but less misleading. Personally I prefer this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that returning There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In any case I don't see it as a blocking issue, we could merge it and later decide the best way to fix this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree with brushing up later. Personally I prefer starting with the second one because I like less misleading and probably we couldn't remove all There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated |
||
|
||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would just leave the same
module
code as previously. If we're targeting a modern browser that support webgl2, it will support modules too and we could use it as an example of modern implementation.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very depends on the purpose of
webgl2_sandbox.html
. If it's just sandbox for WebGL 2.0, it's ok to replace. If it's also formodule
in addition to WebGL 2.0, we should leave it.As of me, it's just sandbox so we can replace. And we can make another example for
module
if necessary.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah so maybe it makes sense to have another entry on the examples as
webgl_module
or so, to showcase the use of modules /cc @mrdoobThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the use of
type="module"
also means code can be modified from the source without having to rebundle/rebuild the library.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay. Yes, I would prefer to keep using modules in this file as it will allow us to iterate this new code faster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I don't have strong opinion here.