-
Notifications
You must be signed in to change notification settings - Fork 502
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
more canvas options #1156
more canvas options #1156
Conversation
if (!Module.sapp_emsc_target) { | ||
console.log("sokol_app.h: invalid target:" + target_str); | ||
const canvas_selector = UTF8ToString(canvas_selector_cstr); | ||
Module.canvas = document.querySelector(canvas_selector); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't this overwrite the standard `Module['canvas'] that might have been set in JS code?
(apart from that Module.canvas
might cause problems with the Emscripten Closure pass which tends to minify JS property names, that's why the Emscripten JS shims use Module['canvas']
which is safe from minification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it returns first, after setting the selector to match. You can put this some other place (like more indirect, as I was doing before, returning a boolean, then acting on that) but I'm not sure I see the point. Also Module.canvas
is not a standard, like it's not doing anything with it before my change in init, but using canvas
name makes it look more like SDL or glfw (that use the same name.) That is part of the point of all this. If it already used a canvas
option in js, I would not need to make any changes to sokol. Seems like minification also does not effect this. I think the minififier correctly understands "this name is part of the external interface" just like when you do an export, it doesn't mangle the name. Can you show an example of it not working? In my testing, this all works as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be noted, the name is semi-arbitrary, but it makes sense to me to use the name that is used in other things (SDL, glfw, etc.) Externally, in js, if I set my selector to !canvas
and added Module.sapp_emsc_target
to special-selectors before loading, it would mostly work, except sokol mixes strategies a bit, in different places, using a selector, id, and the dom-reference. This PR unifies that all to point to 1 thing (the canvas, however you set that up.) In places where it uses a selector, it uses !canvas
, in places where it uses an id, it was moved to use selector, and in places where it uses sapp_emsc_target
it uses canvas
which all points to the same thing, now, which is the same as the js-side canvas
option. I think this actually improves things for people who are not using canvas
option, because it's all 1 strategy, instead of 3, or at least simplifies it for devs, because now you can be sure you already have a good way to get the "screen canvas", without having to do a dom-selection (but it supports it, if you need to.)
I think a further simplification would be to just drop grabbing the selector at all, anywhere other than init, and just always use !canvas
(in C) or Module.canvas
(in JS.) Like it could be setup 1 time to handle the original C-side selector stuff (id/css-selector/etc) and no other functions would need access to the original selector string (since it's always !canvas
.)
The purpose of this PR was to value surgical/minimal update over improving the structure, since I thought that was your focus, but making all the sokol functions that use selector params not use them is totally an option, too, and would be a bit clearer.
This switches all to use CSS-selector for canvas (but still support old id thing) but adds ability to use
canvas
option in setup, for advanced situations like web-components, or just having the same wasm run multiple times in a single page.Related: #1154