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

GLUT: support more glutGet() attributes #7243

Merged
merged 6 commits into from Oct 16, 2018

Conversation

Projects
None yet
2 participants
@Beuc
Contributor

Beuc commented Oct 8, 2018

Adding support for more glutGet() queries for webgl attributes i.e. antialias/alpha/depth/stencil.

@kripken

Thanks @Beuc, looks good with a test.

@@ -427,6 +427,14 @@ var LibraryGLUT = {
case 700: /* GLUT_ELAPSED_TIME */
var now = Date.now();
return now - GLUT.initTime;
case 0x0069: /* GLUT_WINDOW_STENCIL_SIZE */
return Module.ctx.getContextAttributes().stencil ? 8 : 0;

This comment has been minimized.

@kripken

kripken Oct 9, 2018

Owner

indentation looks wrong here

This comment has been minimized.

@Beuc

Beuc Oct 9, 2018

Contributor

Sorry, a tab got included instead of spaces. Fixed :)

@kripken

This comment has been minimized.

Owner

kripken commented Oct 10, 2018

Thanks, looks good, just need a test before merging.

@Beuc

This comment has been minimized.

Contributor

Beuc commented Oct 13, 2018

I added a test case. It's run twice because there's currently no support for destroying/recreating webgl contexts. I also needed a gratuitous glClear() call to prevent library_gl.js from being stripped (GL.xxx referenced in library_glut.glutCreateWindow).

@Beuc

This comment has been minimized.

Contributor

Beuc commented Oct 15, 2018

For the record I don't understand why the test fails at circleci.
It works fine here with both Firefox and Chromium. Any way to get more info/output?

@kripken

This comment has been minimized.

Owner

kripken commented Oct 15, 2018

I think some or all of those errors are due to previous issues we had on incoming. Please merge in latest incoming to here, that may fix things.

@Beuc

This comment has been minimized.

Contributor

Beuc commented Oct 15, 2018

I rebased the patch on incoming.
It still works OK locally.
Crossing fingers X

@kripken

This comment has been minimized.

Owner

kripken commented Oct 15, 2018

Looks like it fixed everything but the new test added here. I think the issue there is that the bot doesn't have graphics hardware, so you need to annotate the test with @requires_graphics_hardware (see the annotation on test_webgl_context_attributes for example).

@Beuc

This comment has been minimized.

Contributor

Beuc commented Oct 16, 2018

Hmm, still no luck.
The test that requests nothing (no alpha/aa/depth/stencil) passes.
The one that requests all of them doesn't pass.
What kind of graphic context does this CI environment get?
Also, any way to get the printf output for that test?

@kripken

This comment has been minimized.

Owner

kripken commented Oct 16, 2018

It's kind of an odd graphics context, see .circleci/config.yml (search for openbox). But it should run things ok.

Getting printf is possible with emrun, for example, if you replace the btest with

    run_process([PYTHON, EMCC, path_from_root('tests', 'browser_test_hello_world.c'), '-o', 'hello_world.html', '--emrun'])
    run_process([PYTHON, path_from_root('emrun'), 'hello_world.html'])

That will build the file for emrun, then run it in emrun, which will log out stderr to the console. (Note that you need to remove stuff like REPORT_RESULT, unless you also handle that.)

@kripken

This comment has been minimized.

Owner

kripken commented Oct 16, 2018

Might be worth comparing to the other tests that use antialiasing/stencil/etc. - they do pass, so finding what the difference is might help.

@Beuc

This comment has been minimized.

Contributor

Beuc commented Oct 16, 2018

(OMG what have I gotten myself into -_-' I don't even need that patch.. ;))
It looks like the other test cases also suffer from the lack of antialias in the CI environment.
This can be reproduced with locally with export LIBGL_ALWAYS_SOFTWARE=1.
I duplicated and skipped the test case accordingly.

@kripken

This comment has been minimized.

Owner

kripken commented Oct 16, 2018

Cool, makes sense. Will merge after CI finishes.

@kripken kripken merged commit 914cb76 into kripken:incoming Oct 16, 2018

21 of 22 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
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-browser-chrome Your tests passed on CircleCI!
Details
ci/circleci: test-browser-firefox 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-f 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-2 branch Oct 17, 2018

@Beuc

This comment has been minimized.

Contributor

Beuc commented Oct 17, 2018

Yay!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment