-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Remove extensions in shaders when upgrading to WebGL 2.0 #8969
Conversation
Thanks for the pull request @baothientran!
Reviewers, don't forget to make sure that:
|
@baothientran thanks for the fix! I should be able to review tonight. |
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.
Great catch @baothientran. I can confirm this fixes the issue.
Source/Renderer/modernizeShader.js
Outdated
var versionThree = "#version 300 es"; | ||
var foundVersion = false; | ||
for (i = 0; i < splitSource.length; i++) { | ||
if (/#version/.test(splitSource[i])) { | ||
splitSource[i] = versionThree; | ||
splitSource[i] = versionThree + "\n" + webgl2DefineMacro; |
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.
Since #version is required to be on top, should I break the loop early?
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.
Yes, that should be fine
@dennisadams 's comment is the solution for the last puzzle I have for depth texture. The new commit should fix the below issues: @lilleyse Can you please take a look at it again? The new commit should convert the sampler's minification and magnification filter to have NEAREST for WebGL 2.0 |
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.
Looks good, just a couple comments
Source/Renderer/Texture.js
Outdated
minificationFilter = mipmap | ||
? TextureMinificationFilter.NEAREST_MIPMAP_NEAREST | ||
: TextureMinificationFilter.NEAREST; |
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.
Can depth textures be mipmapped?
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.
The spec doesn’t seem to mention if depth texture can be mipmapped. I will try it out to see if there are any error
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 hardcode the MinificationFiltering to be NEAREST_MIPMAP_NEAREST
if pixelFormat is depth format. Chrome gives me error on it. So I think it can't have mipmap
That latest changes look good to me 👍 |
Replace extension preprocessor that isn't required in WebGL 2.0 in shaders code with WEBGL_2 preprocessor. All the shaders will automatically have
#define WEBGL_2
after#version 300 es
to indicate the context version being used by CesiumThis should fix the issue where log depth doesn't work properly when zooming out from the earth in WebGL 2.0:
The issue comes from czm_writeLogDepth function. It checks if GL_EXT_frag_depth extension is available, but the extension is only available in WebGL 1.0. So the code doesn't run.
@lilleyse Can you please take a look at the changes?