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

MAX_WEBGL_VERSION #10020

Merged
merged 2 commits into from
Dec 17, 2019
Merged

MAX_WEBGL_VERSION #10020

merged 2 commits into from
Dec 17, 2019

Conversation

juj
Copy link
Collaborator

@juj juj commented Dec 12, 2019

Remove -s USE_WEBGL2=1 and replace it with -s MIN_WEBGL_VERSION=1/2 and -s MAX_WEBGL_VERSION=1/2.

This is originally the direction we decided for in #7612, splicing that part off from there and bringing it in here.

Does not yet actually enable dropping WebGL 1 support, that will be a followup.

@hujiajie
Copy link
Contributor

@juj Overall I love this future-facing direction, despite there isn't a clear future to me. Can you provide a few links where WebGL 1 can be dropped in the future? I didn't find many when working on #7612, so finally GL_MIN_FEATURE_LEVEL was removed there as requested.

In #7612, I had to borrow the term "feature level" from D3D so that internally we can distinguish between WebGL 2.0 and WebGL 2.0 Compute. I'm sorry that I have no extra bandwidth to continue maintaining the PR at the moment. If no one would like to take over the work on WebGL 2.0 Compute, then I doubt "feature level" is the correct term here, especially when it becomes user-facing.

@juj
Copy link
Collaborator Author

juj commented Dec 13, 2019

Can you provide a few links where WebGL 1 can be dropped in the future?

Sorry, I am not sure I understand the question? The goal is not to drop WebGL 1 from Emscripten in the future, but allow applications that only work on WebGL 2 (e.g. ones that use rendering effects that require WebGL 2 specific features) to build without needing to even include WebGL 1 specific support code in them.

... then I doubt "feature level" is the correct term here, especially when it becomes user-facing.

Also think that feature level is a bit of a lent name. Agree I could rename these settings to MIN_WEBGL_VERSION=1/2 and MAX_WEBGL_VERSION=1/2 instead? Or alternatively, could use simple binary USE_WEBGL2=0/1 and USE_WEBGL1=0/1?

@hujiajie
Copy link
Contributor

Sorry, I am not sure I understand the question? The goal is not to drop WebGL 1 from Emscripten in the future, but allow applications that only work on WebGL 2 (e.g. ones that use rendering effects that require WebGL 2 specific features) to build without needing to even include WebGL 1 specific support code in them.

Sorry, I mean if you can provide a few links to certain lines in the Emscripten repo, which could be dropped in WebGL2-only applications.

Also think that feature level is a bit of a lent name. Agree I could rename these settings to MIN_WEBGL_VERSION=1/2 and MAX_WEBGL_VERSION=1/2 instead? Or alternatively, could use simple binary USE_WEBGL2=0/1 and USE_WEBGL1=0/1?

I have no strong preference when there are only two versions of WebGL.

@juj
Copy link
Collaborator Author

juj commented Dec 13, 2019

Sorry, I mean if you can provide a few links to certain lines in the Emscripten repo, which could be dropped in WebGL2-only applications.

A major trend that follows in several WebGL functions is that when targeting WebGL 2 only, the current context version no longer needs to be checked for, but can avoid WebGL 1 entry points and unconditinoally use the WebGL 2 entry points.

Another change is that can drop code for WebGL 1 specific extensions initialization.

@hujiajie
Copy link
Contributor

I see, thanks! Hope code readability won't be affected a lot after that.

@juj juj changed the title GL_MAX_FEATURE_LEVEL MAX_WEBGL_VERSION Dec 16, 2019
@kripken
Copy link
Member

kripken commented Dec 16, 2019

In general the code looks good, but I don't have the full picture for GL. Do we expect anything after WebGL2? If not (if WebGPU is the only future) then why not keep the boolean flag of WebGL2 or 1?

@juj
Copy link
Collaborator Author

juj commented Dec 16, 2019

why not keep the boolean flag of WebGL2 or 1?

The intent is to be able to drop WebGL 1 support from build if doing WebGL 2 only project. So then USE_WEBGL2=0/1 should be paired with a USE_WEBGL1=0/1 to achieve that?

Copy link
Collaborator

@kainino0x kainino0x left a comment

Choose a reason for hiding this comment

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

I'm neutral between USE_WEBGL1+USE_WEBGL2 and MIN_WEBGL_VERSION+MAX_WEBGL_VERSION.
There are no plans to continue significant work on on WebGL 2 Compute, so I don't think it's necessary to plan for any future WebGL versions.

Code lgtm.

@juj
Copy link
Collaborator Author

juj commented Dec 17, 2019

I have a small preference towards MIN_WEBGL_VERSION+MAX_WEBGL_VERSION over USE_WEBGL1+USE_WEBGL2, but could do either way. @kripken: what do you think?

@kripken
Copy link
Member

kripken commented Dec 17, 2019

I also don't have a strong preference here, so lgtm as you feel best @juj

@juj
Copy link
Collaborator Author

juj commented Dec 17, 2019

Ok, let's go with MIN&MAX_WEBGL_VERSION, they'll match the MIN_IE/SAFARI/etc_VERSION, and in the off chance that something new does come up in WebGL space in the future, that can then be marked with a WebGL version 3.

@juj juj merged commit b06e696 into emscripten-core:incoming Dec 17, 2019
petersalomonsen pushed a commit to petersalomonsen/emscripten that referenced this pull request Dec 22, 2019
* Remove -s USE_WEBGL2=1 and replace it with -s GL_MIN_FEATURE_LEVEL=10/20 and -s GL_MAX_FEATURE_LEVEL=10/20

* Rename to MIN_WEBGL_VERSION and MAX_WEBGL_VERSION
belraquib pushed a commit to belraquib/emscripten that referenced this pull request Dec 23, 2020
* Remove -s USE_WEBGL2=1 and replace it with -s GL_MIN_FEATURE_LEVEL=10/20 and -s GL_MAX_FEATURE_LEVEL=10/20

* Rename to MIN_WEBGL_VERSION and MAX_WEBGL_VERSION
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants