Fix glutReshapeFunc not being called (closes #7133)

[haberbyte]

This fixes the issue in #7133 for me and the reshape function callback is called on resize.

I also added { passive: false } to the mouse event listeners as Chrome is complaining about it.
I also noticed some quite old "requestFullScreen" deprecations, should i remove those methods as well? I would like to cleanup the glut library code a bit.

[welcome]

Thank you for submitting a pull request! If this is your first PR, make sure to add yourself to AUTHORS.

[kripken]

Thanks @haberbyte ! Would be good to improve this code.

At first glance this looks good, but I'll need to read this more carefully. First thing, though, can you see the test failures on CI here locally? Let me know if you need help debugging those.

[haberbyte]

Ah yes I see the failures and will try to fix them over the weekend, thanks! Will ping you once i think this is in a good state.

[haberbyte]

@kripken Ok CI is now passing.

I removed the deprecated requestFullScreen and cancelFullScreen stuff as well. Not sure if that is ok to do, but i figured it would be since it is marked deprecated for years now.

Let me know if you would rather have that in a separate PR.

[kripken]

@haberbyte I think that should be in a separate PR, yes. While it's been deprecated for a while, some users will break by that change. We should at least have it in a separate commit so bisection is more clear. But thanks for doing it, it's a good idea!

[haberbyte]

Not sure why I had to retrigger the build for it to pass, but the failure was unrelated to the changes here.

I removed the deprecated functions in a separate PR here: #9861

[kripken]

this changes the capture property, doesn't it? shouldn't we add capture: true to be backwards compatible? or is this part of the fix?

[haberbyte]

You are right. The "passive: false" change is not part of the fix. This is just to silence Chrome's warning about passive event listeners trying to preventDefault.

[kripken]

Do we really need both a resizeListener and a listener on the window itself? I'm not sure why the resizeListener isn't enough by itself. (If I'm missing something here, maybe adding a comment here would help other people reading the code in the future?)

[haberbyte]

The bug i was hitting is that the reshape function would not be called.

I have a canvas element styled with CSS to 100% width/height, so a simple resizing of the window would update the size of the canvas element.

I am not too familiar with what the library_browser.js does out of the box, but it seems like it does not handle those resizes of the canvas.

[kripken]

I see, thanks.

Reading the existing code some more, it looks like none of the other input systems (sdl, etc.) listens for window resize events. And so this looks like a general issue, that maybe the resizeListeners should be called for.

And IIUC the CSS you describe, then your canvas spans the whole window, so I think a resize event should be fired on the canvas, if the window is resized. So listening on the canvas should be enough - but it looks like we don't do that either - we just look for fullscreen resizing.

In summary how about if in library_browser.js, in the init function, we add an event listener for the resize event, at the very end to the canvas, that calls Browser.updateResizeListeners (alongside the other events listened to in the block with if (canvas) {. Would that fix your use case? If so I think it might be a more general fix.

[haberbyte]

That would fix my case.

I will implement the more general fix then (within this PR I guess, so we can keep the discussion).

[kripken]

Sounds good! Yes, in this same PR is simplest and best, I think.

[sbc100]

The deletion of the incoming branch triggered the automatic closing of this branch. My apologies. This was an unexpected side effect.

Re-opening and re-targeting to master.

[stale]

This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 30 days. Feel free to re-open at any time if this issue is still relevant.

[sbc100]

Any chance this could be re-based and landed still?

[haberbyte]

Sorry for the force pushes, had some trouble getting this rebased right. But i think now it is correct.
I am not using glut anymore, so i forgot about this issue. But i guess this can still land, still fixes the same issue (which was just closed since it went stale).

[kripken]

Thanks @haberbyte !

I think this fix is correct. But it's a little odd to me that we didn't have more bug reports on this, as the fix here is quite general - more than glut, it's basically a fix for all the UI APIs. That makes me wonder if I'm not missing something here - ?

It would be good to add a test for this. It could be like tests/sdl_resize.c except instead of emscripten_set_canvas_size to cause a resize, we could synthesize and dispatch an event to the window.

[erik-larsen]

But it's a little odd to me that we didn't have more bug reports on this, as the fix here is quite general - more than glut, it's basically a fix for all the UI APIs. That makes me wonder if I'm not missing something here - ?

I'm the author of the original issue (#7133 - sorry for disappearing!) and I'm also curious, why a general UI fix is needed here and the fix is not limited to GLUT. Maybe #7133 and #9835 are in fact different problems?

In regards to #7133, I never encountered any window resizing issues with SDL+OGLES code. For example, https://erik-larsen.github.io/emscripten-sdl2-ogles2/hello_triangle.html works fine with window resizing by watching for SDL_WINDOWEVENT_SIZE_CHANGED and then updating the GL side with a glViewport call:

void EventHandler::windowResizeEvent(int width, int height)
{
    glViewport(0, 0, width, height);
...
}

void EventHandler::processEvents()
{
    // Handle events
    SDL_Event event;
    while (SDL_PollEvent(&event))
    {
...
            case SDL_WINDOWEVENT:
            {
                if (event.window.windowID == mWindowID
                    && event.window.event == SDL_WINDOWEVENT_SIZE_CHANGED)
                {
                    int width = event.window.data1, height = event.window.data2;
                    windowResizeEvent(width, height);
                }
...
}

GLUT on the other hand, never receives any window resize event (not on window init, nor subsequent resizes). Part of the problem here may be that the html generated by emcc /hello_world_gles.c -o /hello_world_gles.html doesn't enable the canvas to resize dynamically on window resize, so GLUT resize events never got tested? The html I've been using does enable canvas resizing on window resizes:

<!DOCTYPE html>
<html lang="en">
    <head>
        <meta charset="utf-8">
        <meta http-equiv="X-UA-Compatible" content="chrome=1, IE=edge">
        <title>Hello World GLES original without resize fix</title>
        <style>
            .fullwindow {
                position: absolute;
                top: 0px;
                left: 0px;
                margin: 0px;
                border: 0;
                width: 100%;
                height: 100%;
                overflow: hidden;
                display: block;
            }
        </style>
    </head>
    <body style="background-color: #FFFFFF;">
        <canvas class = "fullwindow" id="canvas"/>
        <script type="text/javascript">
            var Module = {};
            Module.canvas = document.getElementById('canvas');
        </script>
        <script async type="text/javascript" src="./hello_world_gles.js"></script>
    </body>
</html>

I did try the fix proposed by @haberbyte, but this still doesn't seem to fix my case of tests/hello_world_gles.c with the above resizable-canvas html. By the way, this is precisely the same HTML I'm using for SDL+OGLES code, which as mentioned above, works fine in getting resize events.

I know @haberbyte you mentioned you weren't working with GLUT any more. I'd be glad to keep looking at this, at least for the case of getting window resize events into GLUT.

[erik-larsen]

Oof, please disregard my last wall of text. After more code research and re-reading this thread, I do believe @haberbyte has the right fix. It is consistent with this GLUT change from 2013.

I just need to figure out why it is not working for my case.

[erik-larsen]

I figured out why my case is still not working. The fix here (window.addEventListener("resize", Browser.updateResizeListeners, true);) does indeed enable resize events to be sent to GLUT.reshapeFunc. However, the canvas width and height never change from their initial values, regardless of how the window is resized.

So, it seems an additional fix is needed here, that updates the canvas width and height on window resizing.

[erik-larsen]

So the following solves the problem completely for me (canvas = 100% window case), although I am not sure about other cases:

library_glut.js:

    Browser.resizeListeners.push(function(width, height) {
      Browser.setCanvasSize(width, height, true); // FIX: update canvas size
      if (GLUT.reshapeFunc) {
        {{{ makeDynCall('vii', 'GLUT.reshapeFunc') }}}(width, height);
      }
    });

library_browser.js:

    updateResizeListeners: function() {
      var canvas = Module['canvas'];
      var rect = canvas.getBoundingClientRect();
      Browser.resizeListeners.forEach(function(listener) {
        // listener(canvas.width, canvas.height); // FIX: canvas doesn't resize
        listener(rect.width, rect.height); // on window resize, use window size
      });
    },