-
Notifications
You must be signed in to change notification settings - Fork 50
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
Feature for web_sys::HtmlCanvasElement
or web_sys::OffscreenCanvas
#102
Comments
The current design also doesn't allow writing independent components that are added to a web application, since there's no guarantee that it's the only component using raw-window-handle, and thus the The only quick fix I can think of right now is to always use a random number for the For example, bevy always uses |
One more difficulty in adding a platform-specific dependency is that we'll be reintroducing a |
For what it's worth, the reasoning for the current API is discussed in #26 (comment) mentions some of the downsides of exposing web_sys types directly |
That's from before OffscreenCanvas was usable. Firefox enabled their implementation by default just in the last update, that's why things have changed now. Also, I'm not sure if anybody is still using stdweb. The last update to that crate was almost three years ago. |
I linked to the same thread in the winit issue that spawned this one. While I can appreciate the spirit and intention of those decisions, I have a hard time accepting the current situation which pushes the dependencies and complexities to downstream crates. There is going to be little-to-no sharing of implementations between them and that additional surface area increases the likelihood of interoperability bugs.
This is something I also forgot to mention, thanks for the reminder. I agree it appears most of the ecosystem has settled on |
Dropping stdweb sounds ok. Using web-sys types directly causes a bit of an issue for anyone using canvases that aren't created from web-sys though (not just stdweb - this includes Emscripten, passing a canvas created manually in JavaScript, etc.). I think using the Abi trait from web-sys would be problematic for those use cases. I think the point about tying raw-window-handle to a particular version of web-sys/wasm-bindgen is still relevant. I'm not sure of a good way around that, but maybe it's ok because major versions aren't expected to happen that often in practice. |
I can see the issue with emscripten, but how does creating it in JavaScript cause issues? You can simply pass it to a wasm function call and wasm-bindgen will wrap it into its own type. |
I see it as similar to the Emscripten use case. If you have a canvas somewhere in the DOM already, you can manually set up a raw window handle (using the data attribute) to have everything work fine today, without interacting with wasm-bindgen or web-sys in your own code. |
It might not be in the DOM though, or even a DOM element in the first place (OffscreenCanvas). A solution might be to make
On the emscripten target, only the first one would be supported. wgpu has two different function calls for |
Yeah exactly - I agree that the current situation doesn't work for canvases that aren't in the DOM, so we should change something here. I just wanted to mention some of the original use cases and why it avoided exposing web-sys types (e.g., major version pinning, external canvases not created through web-sys) so we don't forget about those if we change this API. |
Well, you could rely on the IntoWasmAbi and FromWasmAbi traits to just store a |
Are there any realistic alternatives to types that are known to be valid, without introducing SemVer hazards? Querying the DOM with a The benefit of using the web-sys types is that it removes a lot of potential errors and it doesn't have any issues with querying the DOM or ID collisions mentioned elsewhere. The downside as mentioned in #26 (comment) is that wasm-bindgen maintainers do not want to have If there truly is no safe way to type-erase |
I also want to address this specific comment from that thread:
It sounds nice in theory that one can avoid breaking changes entirely, but we've already seen at least 5 minor release versions here with various breaking changes. In fact, In other words, when (or rather if) |
I haven't had time to review the thread and try to read the links, but I can say this: Breaking changes are certainly to be avoided, but since we're a communication library, our hand really is forced any time the data we want to communicate changes. |
Implementation wise: We could probably just add a new variant |
It should probably be noted that |
There might be another solution for
The additional global "pollution" and not addressing the ID collision issue are minor annoyances, but without good support for web workers this would not support the Bevy use case. Looking at the example made by @anlumo it looks like the helper would have to be provided by the end user creating the That said, I am by no means qualified to recommend something like this. It really smells like another kludge to avoid a shared dependency. So, I'll leave this here as a FWIW. |
A collision-free id could be generated by One issue I have with this solution is that it requires the application developer to add some weird code to the codebase. Since the canvas is created outside of raw-window-handle and thus this Map doesn't exist yet, the developer would have to create this global object on their side with some specified name (so the crate using raw-window-handle can find it), generate the id and then insert the canvas. It would look something like this: const canvas = document.createElement('canvas');
document.body.appendChild(canvas);
const uuid = self.crypto.randomUUID();
if (window.rawWindowHandleCanvases) {
window.rawWindowHandleCanvases.set(uuid, canvas);
} else {
window.rawWindowHandleCanvases = new Map([[uuid, canvas]]);
}
// send uuid to Rust code here This is for the regular case without OffscreenCanvas and without Web Workers. This could also be done in Rust via web-sys of course. |
I worked out an interesting workaround for the shadow dom issue at construction time. https://github.com/Zageron/pixels-shadow-dom-example I have not tried yet, but I imagine I could:
Quite annoying compared to just passing the canvas in and having it work, but at least there is an option that will likely work. |
For the sub-goal of avoiding ID collisions, this could be done by querying This does not solve problems like confusion over which |
Not any weirder than the status quo. The RWH provider (like I think the only difference with using the global scope instead of the DOM is that it allows access to Still not arguing for this as a solution. Just pointing out that it does have advantages over the DOM. |
Following from rust-windowing/winit#2259, the
WebWindowHandle.id
is a flaky way to share canvas references between crates. As discussed in that thread, synchronizing the IDs is challenging (leading to duplicates) and it isn't possible to query canvas elements in a Shadow DOM or offscreen canvases at all.What consumers need is a way to pass the canvas reference through
HasRawWindowHandle
. AFAICT, that can be done with either an enum with variants forweb_sys::HtmlCanvasElement
andweb_sys::OffscreenCanvas
, or withwasm_bindgen::JsValue
. It looks like another option is theFromWasmAbi
andIntoWasmAbi
traits provided bywasm_bindgen
if the reference needs to cross the WASM ABI boundary for some reason.I think the best way forward in the short-term is a feature to enable the use of one of these reference types in addition to the
id: u32
. It is preferable to replace the id entirely but may not be realistic without a deprecation period. The feature also mitigates a potential downside that it adds a dependency onweb_sys
and/orwasm_bindgen
.Open to discussion and other alternatives, but the current design appears to be a significant shortcoming in web environments.
The text was updated successfully, but these errors were encountered: