Wasm support #11
Wasm support #11
Conversation
basis-universal/Cargo.toml
Outdated
| @@ -15,6 +15,8 @@ categories = ["game-development", "graphics", "api-bindings", "compression", "en | |||
| basis-universal-sys = { version = "0.1.0", path = "../basis-universal-sys" } | |||
| lazy_static = "1.4.0" | |||
| bitflags = "1.2.1" | |||
| basis-universal-wasm = { git = "https://github.com/expenses/basis-universal-rs-wasm" } | |||
There was a problem hiding this comment.
Can/should these dependencies be optional?
There was a problem hiding this comment.
Also we'd need basis-universal-wasm to be published with a license compatible with this crate (no copyleft, or copyleft-ish licenses like MPL). I noticed there are some other new licenses probably from js-sys as a dependency. The ones I saw are fine but will need to be allow-listed in deny.toml
There was a problem hiding this comment.
Can/should these dependencies be optional?
Yep, I'll do that.
| @@ -188,7 +188,10 @@ impl TranscoderTextureFormat { | |||
|
|
|||
| /// Returns true if the transcoder texture type is a compressed format. | |||
| pub fn is_compressed(self) -> bool { | |||
| unsafe { !sys::basis_transcoder_format_is_uncompressed(self.into()) } | |||
There was a problem hiding this comment.
I actually don't think it's good to rewrite the C code into rust. If upstream implementation changes, we would have no way of knowing and it would just be silently wrong.
There was a problem hiding this comment.
That's fair, I can have a go at writing bindings for these.
| output_rows_in_pixels.unwrap_or(0), | ||
| total_slice_blocks, | ||
| ) | ||
| if !self.is_compressed() { |
There was a problem hiding this comment.
I'd rather we call the C implementation that try to duplicate it for same reasons as in previous comment
| @@ -447,7 +465,18 @@ impl TranscoderBlockFormat { | |||
|
|
|||
| /// Returns true if the block format is a compressed format. | |||
| pub fn is_compressed(self) -> bool { | |||
| unsafe { !sys::basis_block_format_is_uncompressed(self.into()) } | |||
There was a problem hiding this comment.
I'd rather we call the C implementation that try to duplicate it for same reasons as in previous comment
|
Just wanted to bump this. Could you publish the wasm support crate? I don't want to take a dependency that isn't published because that would prevent me from publishing further versions of this crate. Also seems like there is still some code in enums.rs that was ported over from the C library, and I'd prefer we call into the C library instead of duplicate the logic (which requires us to maintain it and risks desyncing from the original C library's behavior) |
|
Hey, just checking in to say that I'm still working on this PR, I just got busy with other things in the meantime. |
|
So I've sorta gone off the idea of merging this. The codebase as it is works pretty well, and I think that merging the wasm bindings would make things messier and require compromises in the wasm code that I'm not willing to make. Personally, basis transcoding in wasm isn't really acceptably performant. A better solution would be to do transcoding in a fullscreen fragment shader or webgpu compute shader: BinomialLLC/basis_universal#120 Given that the basis universal codebase doesn't move at a super fast pace, I think the best solution is for me to just maintain a fork for the time being. |
|
Thanks for the update (and sorry I'm only now getting back to you.) Will close the PR but happy to revisit if you change your mind. |
Hi! I mentioned I started working on wasm support here: #10. I've been using this on a branch of this crate for a while but I thought I'd just now have a go at merging my changes. This PR isn't super ready yet but some aspect could be merged. I've rewritten some calls like
basis_transcoder_format_is_uncompressedandvalidate_output_buffer_sizein rust so there are less bindings to the basis-universal wasm binary that I have to implement.