Skip to content

Conversation

@juj
Copy link
Collaborator

@juj juj commented Nov 6, 2018

Add -s WORKAROUND_OLD_WEBGL_UNIFORM_UPLOAD_IGNORED_OFFSET_BUG=1 option.

@juj juj added the GL label Nov 6, 2018
self.btest('webgl_draw_triangle.c', '0', args=['-lGL', '-s', 'OFFSCREEN_FRAMEBUFFER=1', '-DEXPLICIT_SWAP=1'])

# Tests that -s WORKAROUND_OLD_WEBGL_UNIFORM_UPLOAD_IGNORED_OFFSET_BUG=1 rendering works.
@requires_graphics_hardware
Copy link
Member

Choose a reason for hiding this comment

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

Does this actually test that code path - does it force the polyfill on somehow?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It just tests that code compiles with that flag enabled. I don't think it is worth spending time to maintaining a test infrastructure where we would enforce the path to kick in.

}
} else {
view = {{{ makeHEAPView('F32', 'value', 'value+count*4') }}};
#if WORKAROUND_OLD_WEBGL_UNIFORM_UPLOAD_IGNORED_OFFSET_BUG
Copy link
Member

@kripken kripken Nov 6, 2018

Choose a reason for hiding this comment

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

is this needed for every single F32 heap view?

How about creating a helper function to avoid all the code duplication, makeGLHEAPView (adding "GL" in the name, and it adds this check)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, it is needed only specifically for these views that it was added to. I would not want to craft any abstractions for this. If I added a makeGLHEAPView() (which lives in core src/parseTools.js) that we'd have to maintain just for this case, then someone would wonder why only parts of src/library_gl.js used makeGLHEAPView() and others used regular makeHEAPView() and it might leak to being used everywhere. I'd prefer to do this as KISS as possible, not building any complexity on top of this.

@juj juj force-pushed the workaround_old_chromium_webgl_bug branch from bd06255 to b140131 Compare November 7, 2018 15:46
@juj juj force-pushed the workaround_old_chromium_webgl_bug branch from b140131 to 0e91891 Compare November 7, 2018 15:47
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

I see, thanks, lgtm.

@juj juj merged commit 6435eff into emscripten-core:incoming Nov 7, 2018
Beuc pushed a commit to Beuc/emscripten that referenced this pull request Nov 17, 2018
@sbc100
Copy link
Collaborator

sbc100 commented Dec 14, 2023

Its been 5 years since this landed. I wonder if we can remove this workaround now?

@sbc100
Copy link
Collaborator

sbc100 commented Dec 14, 2023

Speculative PR: #20925

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.

3 participants