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

[wasm64] Fix reading/writing of gl attributes #21187

Closed
wants to merge 1 commit into from

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Jan 27, 2024

See #21177

src/library_html5_webgl.js Outdated Show resolved Hide resolved
@@ -9,10 +9,9 @@
#include <cmath>
#include <iostream>
#include <vector>
extern "C" {
Copy link
Member

Choose a reason for hiding this comment

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

A lot of these changes look unrelated?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I just cleaned up the test a little while I was working on it.

src/library_html5_webgl.js Show resolved Hide resolved
@sbc100 sbc100 force-pushed the wasm64_testing branch 2 times, most recently from c8381c5 to 136a725 Compare January 27, 2024 01:19
@sbc100
Copy link
Collaborator Author

sbc100 commented Jan 27, 2024

Sorry I had another change mixed up in here.. fixed now. Looking into avoiding the size regression somehow.

HEAP8[a + ({{{ C_STRUCTS.EmscriptenWebGLContextAttributes.antialias }}})] =
HEAP8[a + ({{{ C_STRUCTS.EmscriptenWebGLContextAttributes.premultipliedAlpha }}})] =
HEAP8[a + ({{{ C_STRUCTS.EmscriptenWebGLContextAttributes.majorVersion }}})] =
HEAP8[a + ({{{ C_STRUCTS.EmscriptenWebGLContextAttributes.enableExtensionsByDefault }}})] = 1;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is just way to set the low bit of each of these 32-bit values. We set the whole struct to 1 above.. then set these individual fields to 1.

Its a bit of a hack but it avoid the shift (which is the problem.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see. This seems kind of fragile though, like it could easily get switched back. maybe it should at least have a comment.

src/library_html5_webgl.js Outdated Show resolved Hide resolved
HEAP8[a + ({{{ C_STRUCTS.EmscriptenWebGLContextAttributes.antialias }}})] =
HEAP8[a + ({{{ C_STRUCTS.EmscriptenWebGLContextAttributes.premultipliedAlpha }}})] =
HEAP8[a + ({{{ C_STRUCTS.EmscriptenWebGLContextAttributes.majorVersion }}})] =
HEAP8[a + ({{{ C_STRUCTS.EmscriptenWebGLContextAttributes.enableExtensionsByDefault }}})] = 1;
Copy link
Member

Choose a reason for hiding this comment

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

Ah I see. This seems kind of fragile though, like it could easily get switched back. maybe it should at least have a comment.

@sbc100 sbc100 force-pushed the wasm64_testing branch 2 times, most recently from e7ab4e3 to 1e40e2d Compare January 28, 2024 23:24
static void glut_draw_callback(void) {
printf("glut_draw_callback\n");
Copy link
Member

Choose a reason for hiding this comment

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

do we want to keep these printfs in the test?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, I think it better overall to have more print statements in the test.

@sbc100
Copy link
Collaborator Author

sbc100 commented Jan 29, 2024

Still not ready to land this until I can figure out how to avoid the size regression.

@sbc100
Copy link
Collaborator Author

sbc100 commented Jan 31, 2024

Closing in favor of #21228

@sbc100 sbc100 closed this Jan 31, 2024
@sbc100 sbc100 deleted the wasm64_testing branch January 31, 2024 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants