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

[Fix] #20600 : Calling GLFW.setWindowSize with the screen size leads to browser error #20699

Merged
merged 2 commits into from
Nov 13, 2023

Conversation

ypujante
Copy link
Contributor

@ypujante ypujante commented Nov 13, 2023

As pointed out in the discussion about this issue, it is very odd that glfwSetWindowSize automatically triggers a fullscreen when the size parameters happen to be the exact size of the screen. When invoking it via code, this triggers the error in browsers because it is not a user event.

If the client code wants to go fullscreen, it should simply call Module.requestFullscreen which is made available to the Module when the Browser library is initialized.

Neither library_sdl.js nor library_glut.js implement such a behavior.

src/library_glfw.js Outdated Show resolved Hide resolved
@sbc100
Copy link
Collaborator

sbc100 commented Nov 13, 2023

Can you add something to the ChangeLog about this.

@ypujante
Copy link
Contributor Author

I added the ChangeLog entry as you suggested. Let me know if the language needs to be tweaked.

Thank you

@@ -981,14 +981,9 @@ var LibraryGLFW = {
if (!win) return;

if (GLFW.active.id == win.id) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about the Browser.exitFullscreen(); thing... should explictly setting the windows size also not exit fullscreen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a good question. SDL does not do it. glut does it. In my opinion SDL is right. When Browser.requestFullscreen is called, a fullscreenchange listener is put in place to detect exiting full screen (the user pressing ESC, or whatever key triggers that).

Calling Browser.exitFullscreen(); might trigger an error for the same reason that Browser.requestFullscreen can (if the code calls glfwSetWindowSize while in fullscreen mode but not from a user triggered event).

I believe the proper way for the client code to handle this is to set a Module.onFullScreen callback which gets called whenever the browser goes in/out of fullscreen and then call glfwSetWindowSize.

I actually used this technique in my code this morning:

    EM_ASM(
        {
            RAYLIB = {}; // used to store width/height before fullscreen
            const cb = Module.requestFullscreen;
            Module.requestFullscreen = (pointerLock, resize) => {
                const canvas = document.getElementById('canvas');
                RAYLIB.screenWidth = canvas.clientWidth;
                RAYLIB.screenHeight = canvas.clientHeight;
                cb(pointerLock, resize);
            };
            Module.onFullscreen = Module.cwrap('FullscreenCallback', null, ['boolean']); // called by emscripten
        });

void FullscreenCallback(bool isFullscreen) {
    if(isFullscreen) {
       // ...
    }
    else  {
        glfwSetWindowSize(EM_ASM_INT(return RAYLIB.screenWidth;), EM_ASM_INT(return RAYLIB.screenHeight;));
    }
}

That being said, if you prefer to leave it, I believe the chances of getting an error are lower and probably have less consequences.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, I think being consistent with SDL and glut makes more sense.

@sbc100 sbc100 enabled auto-merge (squash) November 13, 2023 19:32
@ypujante
Copy link
Contributor Author

Not being lucky with failing tests this morning :(

Connecting to nodejs.org (nodejs.org)|104.20.23.46|:443... connected.
HTTP request sent, awaiting response... 400 Bad Request
2023-11-13 19:50:55 ERROR 400: Bad Request.

@sbc100 sbc100 merged commit a92c42c into emscripten-core:main Nov 13, 2023
25 of 27 checks passed
@ypujante ypujante deleted the glfw-remove-auto-fullscreen branch March 23, 2024 18:34
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