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

SDL2/EGL: handle GL context attributes (#7227) #7242

Merged
merged 5 commits into from Oct 10, 2018

Conversation

@Beuc
Copy link
Contributor

@Beuc Beuc commented Oct 8, 2018

Cf. #7227

Copy link
Member

@kripken kripken left a comment

Thanks @Beuc! Core part looks good, but some questions on other stuff.

alpha: false,
depth: true,
stencil: true,
antialias: true,

This comment has been minimized.

@kripken

kripken Oct 9, 2018
Member

Are these defaults set according to EGL's defaults, or WebGL, or just based on the previous defaults we had here? Please add a comment on this, as e.g. the eglChooseConfig docs mention depth and stencil should be false, so we should document the difference there.

This comment has been minimized.

@Beuc

Beuc Oct 9, 2018
Author Contributor

Right, I just avoided changing the previous behavior, I'll add a comment.
Would you rather we follow EGL's defaults though?

This comment has been minimized.

@kripken

kripken Oct 10, 2018
Member

Let's leave that for a later PR - I'm not sure what's the best thing.

var param = {{{ makeGetValue('attribList', '0', 'i32') }}};
if (param == 0x3021 /*EGL_ALPHA_SIZE*/) {
var alphaSize = {{{ makeGetValue('attribList', '4', 'i32') }}};
EGL.alpha = (alphaSize > 0);

This comment has been minimized.

@kripken

kripken Oct 9, 2018
Member

indentation looks wrong here

@@ -1247,6 +1247,7 @@ def test_webgl_context_attributes(self):
# perform tests with attributes activated
self.btest('test_webgl_context_attributes_glut.c', '1', args=['--js-library', 'check_webgl_attributes_support.js', '-DAA_ACTIVATED', '-DDEPTH_ACTIVATED', '-DSTENCIL_ACTIVATED', '-DALPHA_ACTIVATED', '-lGL', '-lglut', '-lGLEW'])
self.btest('test_webgl_context_attributes_sdl.c', '1', args=['--js-library', 'check_webgl_attributes_support.js', '-DAA_ACTIVATED', '-DDEPTH_ACTIVATED', '-DSTENCIL_ACTIVATED', '-DALPHA_ACTIVATED', '-lGL', '-lSDL', '-lGLEW'])
self.btest('test_webgl_context_attributes_sdl2.c', '1', args=['--js-library', 'check_webgl_attributes_support.js', '-DAA_ACTIVATED', '-DDEPTH_ACTIVATED', '-DSTENCIL_ACTIVATED', '-DALPHA_ACTIVATED', '-lGL', '-s', 'USE_SDL=2', '-lGLEW'])

This comment has been minimized.

@kripken

kripken Oct 9, 2018
Member

is that a new file? maybe forgot to add it to the PR. but also, should it have egl in the name?

This comment has been minimized.

@Beuc

Beuc Oct 9, 2018
Author Contributor

-_-'
That's a new file. I decided to run the test at the SDL2 level, which is what people more commonly use, and which itself relies on EGL.

This comment has been minimized.

@kripken

kripken Oct 10, 2018
Member

Oh, I see, so this PR fixes SDL2 as well?

This comment has been minimized.

@Beuc

Beuc Oct 10, 2018
Author Contributor

Yes, that's the goal :)

@kripken
Copy link
Member

@kripken kripken commented Oct 10, 2018

Thanks! Everything looks good, however browser.test_sdl2glshader fails both in the firefox and other modes. Does it fail for you locally too? Let me know if the problem isn't obvious to you, it could be a weird closure issue that I can look into.

@Beuc
Copy link
Contributor Author

@Beuc Beuc commented Oct 10, 2018

I introduced a new variable without "var" and Closure complained.
It took longer to locate the error in closure's extremely verbose output than to fix ;)

@kripken
Copy link
Member

@kripken kripken commented Oct 10, 2018

Great, thanks!

@kripken kripken merged commit 1ec8881 into emscripten-core:incoming Oct 10, 2018
18 of 22 checks passed
18 of 22 checks passed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
ci/circleci: test-browser-chrome Your tests failed on CircleCI
Details
ci/circleci: test-browser-firefox Your tests failed on CircleCI
Details
ci/circleci: test-f Your tests failed on CircleCI
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build-docs Your tests passed on CircleCI!
Details
ci/circleci: flake8 Your tests passed on CircleCI!
Details
ci/circleci: test-ab Your tests passed on CircleCI!
Details
ci/circleci: test-binaryen0 Your tests passed on CircleCI!
Details
ci/circleci: test-binaryen1 Your tests passed on CircleCI!
Details
ci/circleci: test-binaryen2 Your tests passed on CircleCI!
Details
ci/circleci: test-binaryen3 Your tests passed on CircleCI!
Details
ci/circleci: test-c Your tests passed on CircleCI!
Details
ci/circleci: test-d Your tests passed on CircleCI!
Details
ci/circleci: test-e Your tests passed on CircleCI!
Details
ci/circleci: test-ghi Your tests passed on CircleCI!
Details
ci/circleci: test-jklmno Your tests passed on CircleCI!
Details
ci/circleci: test-other Your tests passed on CircleCI!
Details
ci/circleci: test-p Your tests passed on CircleCI!
Details
ci/circleci: test-qrst Your tests passed on CircleCI!
Details
ci/circleci: test-sanity Your tests passed on CircleCI!
Details
ci/circleci: test-uvwxyz Your tests passed on CircleCI!
Details
@Beuc Beuc deleted the Beuc:patch-1 branch Oct 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.