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

Added new hint: SDL_HINT_EMSCRIPTEN_CANVAS_ELEMENT #6134

Closed
wants to merge 1 commit into from
Closed

Added new hint: SDL_HINT_EMSCRIPTEN_CANVAS_ELEMENT #6134

wants to merge 1 commit into from

Conversation

sherpya
Copy link

@sherpya sherpya commented Aug 25, 2022

This helps emscripten to find the canvas HTML element

Description

This patch add the possibility to specify the canvas using a new hint SDL_HINT_EMSCRIPTEN_CANVAS_ELEMENT

Existing Issue(s)

fixes #5260

Non related, a strange side effect while testing the patch, calling SDL_CreateWindow with an overside dimension takes the path (I've added an fprintf before):

    /* Some platforms blow up if the windows are too large. Raise it later? */
    if ((w > 16384) || (h > 16384)) {
        SDL_SetError("Window is too large.");
        return NULL;
    }

but SDL_GetError() returns SDL not built with thread support, that is the output of SDL_CreateSemaphore() in src/thread/generic/SDL_syssem.c, I don't known who calls it, perhaps I may make a PR that prefixes the name of the function called in that file

The only possible caller looks like SDL_TimerInit(), but

#if !defined(__EMSCRIPTEN__) || !SDL_THREADS_DISABLED

that shouldn't be compiled in

@Daft-Freak
Copy link
Collaborator

I have almost the same patch: Daft-Freak@b0ed9e0. Though I went with ..._CANVAS_SELECTOR (not that it matters much).

It should probably be mentioned that everything rendering related is going to ignore this and use Module.canvas without something like Daft-Freak@cb6d810 / Daft-Freak@d3d0169.

(I was about to PR the first four commits from Daft-Freak/SDL@main...Daft-Freak:SDL:create-window-from-2 after being reminded about #5260.)

@@ -218,14 +218,19 @@ Emscripten_CreateWindow(_THIS, SDL_Window * window)
SDL_WindowData *wdata;
double scaled_w, scaled_h;
double css_w, css_h;
const char *canvasElement = SDL_GetHint(SDL_HINT_EMSCRIPTEN_CANVAS_ELEMENT);;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be canvas_element (or even canvas_selector), also there's an extra semicolon.

Copy link
Author

Choose a reason for hiding this comment

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

ok for the semicolon, I've updated the PR, but I've used canvasElement according to:
https://github.com/libsdl-org/SDL/blob/main/src/video/emscripten/SDL_emscriptenevents.c#L722

and anyway all the code calls it element, e.g. emscripten_set_canvas_element_size
The variable name is not really a problem for me, I can change it

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, that one is also wrong... most names are in snake_case now. (You could argue that that one is even more wrong as some of the options aren't even elements... But uh, it is also 8 year old code 😄 )

... and yeah there's a bit of inconsistent naming going on, mostly because it used to be an element id(+ some magic values) and is now a CSS selector (+ some other magic values).

Copy link
Author

Choose a reason for hiding this comment

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

so? @slouken @Daft-Freak going for canvas_element or canvas_selector?

Copy link
Author

Choose a reason for hiding this comment

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

and yes I handle directly specialHTMLTargets object because my canvas is in a shadow dom, emscripten wouldn't be able to find it without the shadow dom root

This helps emscripten to find the canvas HTML element
@slouken
Copy link
Collaborator

slouken commented Aug 26, 2022

I'm going to close this for now in favor of @Daft-Freak's work in progress.

@slouken slouken closed this Aug 26, 2022
@sherpya sherpya deleted the emscripten-canvas-hint branch August 26, 2022 14:08
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.

Allow customizing canvas on Emscripten
3 participants