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

Projects
None yet
2 participants
@Beuc
Contributor

Beuc commented Oct 8, 2018

Cf. #7227

@kripken

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

Owner

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

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

Owner

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

Owner

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

Owner

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

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

Owner

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

This comment has been minimized.

@Beuc

Beuc Oct 10, 2018

Contributor

Yes, that's the goal :)

@kripken

This comment has been minimized.

Owner

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

This comment has been minimized.

Contributor

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

This comment has been minimized.

Owner

kripken commented Oct 10, 2018

Great, thanks!

@kripken kripken merged commit 1ec8881 into kripken:incoming Oct 10, 2018

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