Improve C# WASM runtime binding loading and logging#147
Conversation
kottochii
left a comment
kottochii
left a comment
There was a problem hiding this comment.
The changes are legitimate and solve the problem of quiet failures, exposing the root problems with method referencing.
I, however, would like to keep an issue in the backlog to resolve the missing bindings.
cameronhum
left a comment
cameronhum
left a comment
There was a problem hiding this comment.
The code works and does what it aims where it fixes JS binding for the C# wasm setup and adds a fallback for process events. I have checked it and as seen in the below image it works.
before I approve I want to check the reasoning for the implementation straight into a generative file (as a temporary fix is fine but this will be overwritten eventually due to the files nature) and if we are able to simplify the logs so it causes less console spam.
if you can fix/justify these I am happy to push to next review
|
|
||
| [JSImport("SplashKitBackendWASM.draw_circle", "main.js")] | ||
| public static partial void DrawCircle(int color, double x, double y, double radius); | ||
|
|
There was a problem hiding this comment.
Is this necessary to add? Since this is a generated file should this be excluded from this pr? i saw you said you implemented missing bindings is that related to "Compare SplashKit.cs with generated bindings" if so this is reasonable as a quick fix although should be fixed at the core (generation, potentially a new task)
There was a problem hiding this comment.
You’re right — I’ve removed the manual change from SplashKitBindings.Generated.cs since it is a generated file and would likely be overwritten later.
The original intent was to quickly validate whether the missing binding was related to generated bindings versus the runtime wiring itself. I’ve now kept the fix focused on the JS binding/runtime side only, and the missing bindings should instead be addressed properly through the generation process in a separate task.
| if (typeof window[name] === "function") { | ||
| bindingsFunctions[name] = window[name]; | ||
| } else { | ||
| console.warn(`[SplashKit WASM] Missing function: ${name}`); |
There was a problem hiding this comment.
much more intentional and doesn't rely on crashes to occur. good job. however this can be quite 'spammy' since it runs every single time a build occurs. can we put this into a single warning instead?
There was a problem hiding this comment.
Updated this so the missing bindings are now reported through a single warning instead of logging one warning per function.
I also moved the detailed list into a collapsed console group so the console stays cleaner during builds while still keeping the missing bindings easy to inspect for debugging.
cameronhum
left a comment
cameronhum
left a comment
There was a problem hiding this comment.
Checked after the changes and it addresses everything i mentioned. I approve it now to go onto the next review
4 participants
Description
This change improves the stability of the C# WebAssembly runtime in SplashKit Online by addressing issues in the JavaScript binding layer.
During testing, the runtime failed to initialise reliably due to the binding loader attempting to resolve all generated SplashKit functions using
eval(). Since many of these functions are not available in the browser runtime, this resulted in runtime crashes and prevented C# programs from executing correctly.The binding logic in
CSharpWasmExpo/main.jshas been updated to safely check for function availability before assignment. Missing functions are now skipped, and warnings are logged instead of causing failures. This allows the runtime to initialise successfully even when bindings are incomplete.Additionally, basic handling for
process_eventshas been introduced to improve debugging visibility when the function is not properly wired.This change resolves the runtime initialisation crash and provides clearer insight into missing bindings, supporting further investigation of incomplete API coverage in the C# runtime.
Fixes # (runtime initialisation crash caused by missing JS bindings)
Type of change
How Has This Been Tested?
The changes were tested locally using the SplashKit Online development environment.
Steps:
npm startExpected / Observed Results:
process_eventsis still not fully wired, confirming improved debugging visibilityTesting Checklist
Checklist