sokol_app.h html5: cleanup canvas lookup handling (see #1154)

floooh commented 4 days ago

Proposal for #1154:

change sapp_desc.html5_canvas_name to .html5_canvas_selector
remove the canvas-id vs canvas-selector hack (instead sokol-app now expects a 'css selector string' to be passed in.
uses the Emscripten helper function findCanvasEventTarget() to lookup the JS canvas name by canvas selector (which also considers Module['canvas'] and specialHMTLTargets[])

TODO:

[x] test with custom canvas selectors (e.g. <canvas id="bla"> => .html5_canvas_selector="#bla"
[x] ~~test with setting canvas in index.html via Module['canvas']~~ this doesn't actually work any longer
[x] test with adding canvases to specialHTMLTargets[] in index.html (see below)
[x] in sokol-samples shell.html, remove the Module.canvas() and Module.postRun() functions (those are not actually called anymore)

To register canvases that can't be looked up via document.querySelector(), add them to the specialHTMLTargets[] array after the Emscripten environment is setup but before the WASM is started. For instance via Module.preRun() in index.html:

        preRun: [
          () => {
            specialHTMLTargets['my_canvas'] = my_canvas;
          },
        ],

...then in sokol_main() use .html5_canvas_selector = "my_canvas"

konsumer commented 4 days ago

This doesn't seem terribly different from my solution (switch id called "name" to selector) but you need an extra step (to setup the selector in a preRun.) Additionally, does that actually work with your Module.sapp_emsc_target, I think it might not. What does this solve over my solution? In mine, it supports the usecase of setting your own selector (you don't have to use !canvas for example, you can use my_canvas, like here, if you prefer that for some reason) and handles a few more usecases, but also has sensible defaults (if you set the canvas element, it will set it up for you.)

floooh commented 3 days ago

Additionally, does that actually work with your Module.sapp_emsc_target, I think it might not.

It does, but it's only a cache to prevent calling findCanvasEventTarget() each time the canvas object is required in sokol_app.h JS code.

What does this solve over my solution?

I think the main difference is that it automatically works with all the emscripten_*() functions, also for canvases that can't be looked up via document.querySelector() (because in that case the canvas object is provided via specialHTMLTargets[] where it will be found by the Emscripten functions with the string provided in sapp_desc.html5_canvas_selector (it doesn't have to be !canvas but can be any string).

I also checked, and the Emscripten functions do not find a canvas that's provided via Module['canvas'] (I guess because this option has been default-enabled for quite a while: https://github.com/emscripten-core/emscripten/blob/b3edd4cdb6f8055f7b50e3cc406b8ec7b2232afb/src/settings.js#L2000-L2005)

...e.g. for clarity, this is the line of code which the emscripten_*() functions use to find their 'event target' with the default Emscripten build settings, note that this doesn't take Module['canvas'] into account:

https://github.com/emscripten-core/emscripten/blob/b3edd4cdb6f8055f7b50e3cc406b8ec7b2232afb/src/library_html5.js#L342

...and this is the deprecated behaviour when DISABLE_DEPRECATED_FIND_EVENT_TARGET_BEHAVIOR is explicitly switched off:

https://github.com/emscripten-core/emscripten/blob/b3edd4cdb6f8055f7b50e3cc406b8ec7b2232afb/src/library_html5.js#L382-L399

(note the special case for #canvas => Module['canvas'])

(in that case there will also be a warning on the browser console Rules for selecting event targets... which I actually remember seeing before the first round of canvas-lookup behaviour cleanup in sokol_app.h a couple of years ago).

konsumer commented 3 days ago

I think the main difference is that it automatically works with all the emscripten_*() functions, also for canvases that can't be looked up via document.querySelector()

I'm not sure what you mean here. My functions works with selectors too.

(it doesn't have to be !canvas but can be any string).

That's true for mine too. I just used!canvas as default because it's common. You can still do it manually, as you are suggesting with my path, as a separate step, if you prefer that for some reason. it's only the default, if you passed a canvas option.

I also checked, and the Emscripten functions do not find a canvas that's provided via Module['canvas'] (I guess because this option has been default-enabled for quite a while

That is exactly what I was saying. It's no longer automatic, and that is what those options are about. The old behavior was that you need #canvas in the same page, and it was used automatically, and that was mega-jenky, then they allowed override with canvas option, then they deprecated all that as built-in to the emscripten layer, automatic, but libs kept following it, like in their own layer. You do very similar currently, just not totally the same (more like the deprecated emscripten behavior) and not in a way that can be used with web-components or multiple canvases with multiple instances of the same wasm. Even if you consider my usecase so edge that it's not worth considering, it's nice to give users a few ways to connect their canvas to sokol, and that will work as they expect. Currently, if you provide a canvas option, in js, the standard libs will do roughly what I am proposing, and my path doesn't disable anything that sokol did before.

For my solution, you get more options then how it currently works, and a bit more than (but including options provided by) this PR:

you can hardcode id, as sokol already did, before
you can use canvas option, and it will bind the selector to it
you can use hardcode a query-selector, and it will also work with everything
you can bind your own specialHTMLTargets[] to some other (or the same !canvas) selector, and juggle things in preRun

In each path on my solution you get the same 2 things (Module.canvas for js, and a string selector for the C wrappers.)

I think there is a communication breakdown somewhere here, since it seems so clearly better to me to do it my way, and I see no benefit to doing it this way. Like, it's not simpler, more elegant, and it gives users less options, and is a bit more cumbersome. There is no difference with performance or memory-usage, or anything else, and It also does not work like other libs people might be used to (SDL, glfw, raylib, etc.)

Fundamentally, it's your lib, and you can do whatever you like with that. It's not really usable to me, as it is, passing the id from C-side, in my current project, and I don't see the point of forcing users to set up that extra layer with specialHTMLTargets & preRun, when it's so simple to just do it in sokol, and support all the options, but it's your thing, and your choice to make. I can use a forked version of sokol_app.h, or wait for your PR to go in, and use it in the cumbersome fashion you suggest, or just not use it.

floooh commented 3 days ago

I'm not sure what you mean here. My functions works with selectors too.

I mean these:

https://github.com/floooh/sokol/blob/8c989e39c008149a8a6b89cd001a2af3dbe3a8c2/sokol_app.h#L5834-L5850

...and these:

https://github.com/floooh/sokol/blob/8c989e39c008149a8a6b89cd001a2af3dbe3a8c2/sokol_app.h#L5941-L5951

...those functions take a selector string and then call Emscripten's findCanvasEventTarget() function, and that first looks in specialHTMLTargets and if not found there do a document.querySelector() (but it doesn't know about Module['canvas']),

floooh commented 3 days ago

PS: there's also this detail in the WebGPU C API:

https://github.com/emscripten-core/emscripten/blob/b3edd4cdb6f8055f7b50e3cc406b8ec7b2232afb/system/include/webgpu/webgpu.h#L1136-L1140

...e.g. the WebGPU shim is also expecting a CSS selector string to find the canvas object and this also uses the findCanvasEventTarget() function under the hood:

https://github.com/emscripten-core/emscripten/blob/b3edd4cdb6f8055f7b50e3cc406b8ec7b2232afb/src/library_webgpu.js#L2494

floooh commented 3 days ago

...e.g. in the end it's all about making Emscripten's findCanvasEventTarget() function happy (which is now just an alias of findEventTarget()), since a lot of Emscripten shim code depends on that.

floooh commented 3 days ago

This is an interesting part in your PR:

    if (Module.canvas) {
        specialHTMLTargets["!canvas"] = Module.canvas;
        stringToUTF8("!canvas", canvas_selector_cstr, 8);
        return;
    }

E.g. don't expect specialHTMLTargets[] to be initialized outside the WASM, but instead expect a canvas object to be set in a special place (like Module['canvas']) and patch specialHTMLTarget as part of the sokol-app setup. I would reverse the canvas name thing and simply do this though:

    if (Module['canvas']) {
        const canvas_selector_str = UTF8ToString(canvas_selector_cstr);
        specialHTMLTargets[canvas_selector_str] = Module['canvas'];
    }

I could live with that too.

I'm using Module['canvas'] instead of Module.canvas because the Closure compiler pass usually tries to minify the Module.canvas form, which then causes trouble when using that property outside the Emscripten generated Javascript code.

floooh commented 3 days ago

Ok see:

https://github.com/floooh/sokol/pull/1159/commits/05190b6d4359eaa87b1e44f98e8460364f16fc24

(the only detail I don't quite like about that is the use of the deprecated Module['canvas'] property, but I guess it's fine since it provides an automatic backward compatibility path even when Emscripten is compiled with default options).

konsumer commented 3 days ago

...those functions take a selector string and then call Emscripten's findCanvasEventTarget() function, and that first looks in specialHTMLTargets and if not found there do a document.querySelector() (but it doesn't know about Module['canvas']),

Did you try it? As I said, my code sets the string selector for the emscripten functions, so in C, it works the same, and in js you can use Module.canvas. It should all work fine, allowing more options, not less. It's not that different from how it was, but you used another name (Module.sapp_emsc_target) and you mixed 3/4 strategies in different places, where I use 2 (1 for js and 1 for C.) Using canvas makes it so the options work automatically (I can skip setting the cached copy of js canvas, since it's already set, but I match the selector to it.)

...e.g. the WebGPU shim is also expecting a CSS selector string to find the canvas object and this also uses the findCanvasEventTarget() function under the hood:

Again, so do I.

I'm using Module['canvas'] instead of Module.canvas because the Closure compiler pass usually tries to minify the Module.canvas form, which then causes trouble when using that property outside the Emscripten generated Javascript code.

Are you sure it does that? Module['canvas'] is fine, but I did not have this problem, so I am confused about it.

I could live with that too.

This does not do the same thing. If you look at the other code, the legacy stuff that uses id, etc, that runs before this, it overrides it with other strings. I think a few console.logs will show what I mean, if you want to verify. Try setting Module.canvas outside C, and see what canvas_selector_str is there. It will be #canvas, which is incorrect.

Ok see:

As I said, I think you are misreading the deprecated behavior. It's not "Don't name your copy of the canvas Module.canvas" (several other libraries do it this way, and it's a good way to set the option, since it stays the same name all the way through any js code.) It's saying "emscritpen used to automatically hook #canvas up for you, to Module.canvas" which I agree is stupid. The current behavior of sokol is more like that legacy behavior, though.

I think the key misunderstanding about my solution is here. If you look at my code, it does this (in sapp_js_init) and here in _sapp_emsc_run:

if (Module.canvas) {
    specialHTMLTargets["!canvas"] = Module.canvas;
    stringToUTF8("!canvas", canvas_selector_cstr, 8);
    return;
}

// lookup and store canvas object by name
const canvas_selector = UTF8ToString(canvas_selector_cstr);
Module.canvas = document.querySelector(canvas_selector);

My code supports several user-config paths:

User sets canvas option in js emscritpen config, this is used, and selector is set to !canvas and !canvas is bound to Module.canvas, so it can be used in sokol's C and JS
User sets html5_canvas_selector in C, so that is resolved to Module.canvas. This is similar to original behavior, except they can use a selector instead of an id
You can still do preRun stuff, manually setting Module.canvas and/or html5_canvas_selector, and using specialHTMLTargets however you want.
Old behavior: User sets id in C, via html5_canvas_name (see here), so that is resolved to Module.canvas.

You definitely need to set Module.canvas to match whatever is in html5_canvas_selector because there is a mix in code of both strategies. There was before, I didn't add that. Instead of Module.canvas it just used to be some other name. Using the canvas name makes the config work simpler, and also in the old code nothing tied the option to the selector (it wasn't considered an option, more like a cached copy of canvas) so there were situations where they would not be the same element, which means it didn't fully work, if you tried to override anything.

Personally, I don't like any css-selectors/id being set in C, at all. I left support for it, so you get id (like previous) and query-selector (new thing, but simple enough.) If I were using SDL/raylib/etc, and needed this, it's pretty easy without anything in the C code, as long as canvas option works:

const instance = await setupEmscripten({
  canvas: document.querySelector('main > canvas') // or whatever you want here
})

To me, it feels like a deployment/implementation detail that should be set outside of sokol/C. Like when you target an opengl context on native, you don't set the video-driver that is used, or force users to have a specific kind of monitor or whatever. You just say "hey OS, give me a window that is XxY size". I think of emscritpen as just another target platform, where it should mostly do the right stuff, and let users override that, outside of the C code, if they want to. I kept support for it because I want all the old code to still work, but I disagree with that sort of usage, in the first place. I think Emscripten themselves came to the same conclusion, which is why that old behavior of automatically attaching to #canvas is deprecated.

Again, personally, I would say just use 1 selector all the way through for emscripten_* (like !canvas or whatever you want) but I'd still recommend starting with a ! to make it not a valid regular selector, otherwise (for example mycanvas actually targets an html-element like <mycanvas />.) Like, make it not even user-configurable, that is just what the emscripten-binding layer uses to tie Module.canvas to emscripten_*. User can select their canvas outside of C, and the same selector would be used in all those functions, internally. This would break the current behavior of passing the id, in user's C code, so I left that change out, but I don't think it's the right way to do it, since it's easy enough to do outside of C, and then your wasm isn't tied to a very specific HTML shape forever (or until you change the selector and recompile.)

konsumer commented 3 days ago

Is there a way to chat on discord or something? I am 203695042760671234 (.konsumer) That may be a better forum for this sort of back and forth. I am happy to step through my code. Again, I'm not currently using sokol, so I'm not really sure what the point is for me, but I really don't like being misunderstood, and I do think it would help sokol to be more useful to others.

I can let it rest, if we step through the code, and I feel like you understand what I was doing, and you are still like "I still don't like your way." I have a strong feeling that once you understand, you will agree with me, but I'm cool either way.

In the hypothetical situation that I decide to use sokol in my thing, and you did not agree with me, I could again, just fork the 1 header & change a few lines of code, or do it the more roundabout way you are proposing (once your PR is in.) It's not a huge deal, either way.

floooh commented 2 days ago

Is there a way to chat on discord or something?

No, tbh I'll just merge my PR and be done with it, we already wasted too much time on a trivial feature :)

One thing I'll need to fix is checking that Module['canvas'] is actually a JS object ~~because it's not unusual that index.html defines it as a function that returns a canvas (must be some even older deprecated behaviour)~~ (it's actually a self-executing function)

I'm quite convinced that Module['canvas'] is deprecated. Library shims in the Emscripten SDK like GLFW or SDL are not a good place to look at because those are pretty much abandondend by the Emscripten devs (Emscripten platform support for SDL has moved upstream directly into SDL).

floooh commented 2 days ago

Oki, dropping the sokol_app.h from this PR into your multicanvas example (https://github.com/konsumer/sokol-canvas-example-1154) works without changes. I'll just keep it at that.

konsumer commented 2 days ago

we already wasted too much time on a trivial feature :)

Agreed. Originally, I was trying to help, but I feel like the finer points of the differences are probably not important, and this much talk about a trivial thing feels like bike-shedding.

(it's actually a self-executing function)

Yeh, I think historically it could be a string, DOM-element or a function (and it would run it, if it was a function, or use it as an id, if it was a string.) As a self-running function, it could output string/DOM-element, or even return a function.

I had thought you did not use Module.canvas, but it seems to work, and I see it in updated README, so I think I misunderstood that part.

I'm quite convinced that Module['canvas'] is deprecated

I still disagree that offering canvas option in emscripten-setup is the deprecated part. The old behavior in emscripten, of automatically pulling #canvas and attaching to Module.canvas was definitely jenky. You are still using Module.canvas here, so I'm not sure I understand what you are saying.

Oki, dropping the sokol_app.h from this PR into your multicanvas example

That's great. I updated the demo. I also checked the canvases that get set to Module['canvas'] (in sapp_js_init) and they are correct (each web-component gets it's own canvas.) Works for me!

I notice that the selector stays #canvas, but that is fine because you overwrite it with specialHTMLTargets[target_selector_str] = Module['canvas']

floooh / sokol

sokol_app.h html5: cleanup canvas lookup handling (see #1154) #1159