Skip to content

Conversation

@nokotan
Copy link
Contributor

@nokotan nokotan commented Jul 6, 2022

When called glfwSetWindowSize, emscripten runtime will exit unintentionally.

#if USE_GLFW == 3
{{{ makeDynCall('viii', 'GLFW.active.windowSizeFunc') }}}(GLFW.active.id, GLFW.active.width, GLFW.active.height);
#endif
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

The idea is that if the runtime is no longer active we do want to exit here. See the keepRuntimeAlive function.. for some reason it must b returning false in your use case, and its probably worth looking into why.

tests/glfw3.c Outdated
glfwGetWindowPos(window, &x, &y); // stub
glfwGetWindowSize(window, &w, &h);
assert(w == 640 && h == 480);
assert(exited == 0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this assert fire if you won't include your patch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this assert is just in the incorrect line. thanks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

And it fires, without this patch? (i.e. if I add this assert locally I will see the test fail?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This assertion fires with -sEXIT_RUNTIME=1.
(I've used --emrun, which implies -sEXIT_RUNTIME=1)

@nokotan nokotan changed the title Fix runtime exit while calling glfwSetWindowSize Fix unexpected runtime exit with EXIT_RUNTIMEwhile calling glfwSetWindowSize Jul 9, 2022
@sbc100
Copy link
Collaborator

sbc100 commented Jul 14, 2022

I think I found the root of the issue and will upload a patch shortly. Thanks for figuring out where this issue was showing up!

sbc100 added a commit that referenced this pull request Jul 14, 2022
See #17376 for the original report inspiration for the modified test.
sbc100 added a commit that referenced this pull request Jul 14, 2022
See #17376 for the original report inspiration for the modified test.
@sbc100
Copy link
Collaborator

sbc100 commented Jul 14, 2022

I think #17438 should fix this issue.

sbc100 added a commit that referenced this pull request Jul 14, 2022
See #17376 for the original report inspiration for the modified test.
sbc100 added a commit that referenced this pull request Jul 14, 2022
See #17376 for the original report inspiration for the modified test.
sbc100 added a commit that referenced this pull request Jul 15, 2022
sbc100 added a commit that referenced this pull request Jul 15, 2022
@nokotan nokotan closed this Jul 16, 2022
sbc100 added a commit that referenced this pull request Jul 18, 2022
sbc100 added a commit that referenced this pull request Jul 18, 2022
sbc100 added a commit that referenced this pull request Jul 18, 2022
sbc100 added a commit that referenced this pull request Jul 18, 2022
sbc100 added a commit that referenced this pull request Jul 19, 2022
sbc100 added a commit that referenced this pull request Jul 21, 2022
sbc100 added a commit that referenced this pull request Jul 21, 2022
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.

2 participants