kvark / mint

Math Interoperability Types
MIT License
256 stars 20 forks source link

Add optional support for `bytemuck` #54

Closed rijenkii closed 4 years ago

rijenkii commented 4 years ago

bytemuck is a crate that allows simple casts between "plain old data" types. With this pull request, the following becomes possible:

use mint::Vector2;

fn main() {
    let data = &[
        Vector2::<u16>::from([1, 1]),
        Vector2::<u16>::from([1, 0]),
        Vector2::<u16>::from([0, 0]),
        Vector2::<u16>::from([0, 1]),
    ];
    let raw_data: &[u8] = bytemuck::cast_slice(data);
    assert_eq!(
        raw_data,
        &[
            1, 0, 1, 0,
            1, 0, 0, 0,
            0, 0, 0, 0,
            0, 0, 1, 0,
        ]
    );
}
kvark commented 4 years ago

Thank you for filing this! mint tries to be extremely minimal in order to be widely adopted for a specific use case of interoperability. There is a high bar, therefore, for adding new features. In this case, for example, we'd see more code, more testing surface, and more need to publish (i.e. what if bytemuck releases a new breaking version?).

What are you trying to do by having the dependency here? The assertion code wouldn't be too verbose if it used Vector2 constructors instead of the fixed-size arrays.

rijenkii commented 4 years ago

Well, in case of mint, bytemuck provides a simple way of converting structures and arrays of structures (such as Vector2) into flat byte arrays (and back), which is a pretty common situation when working with graphics APIs, and, AFAIK, there's no (safe) way of doing this with standard Rust.

If you think that this is feature is unnecessary, or if it will be too much of a hassle to support, you can close this pull request.

kvark commented 4 years ago

So the case you are thinking about is when you (as a library author) have a function that gets &[mint::Vector2<f32>] and you are passing it down to GPU, so you need to be able to convert them to a byte slide?

rijenkii commented 4 years ago

As an example, yes. Or another example, I have a Vertex struct, and that struct has mint::VectorX and mint::MatrixX fields. To convert a &[Vertex] to a &[u8] (to then pass that slice to the GPU) right now, I have to:

kvark commented 4 years ago

Thank you for an example. How would it look like if your PR got accepted?

rijenkii commented 4 years ago

The latter example would look something like this:

fn pass_bytes_to_gpu(_data: &[u8]) {}

#[derive(Copy, Clone)]
struct Vertex {
    pos: mint::Vector3<f32>,
    uv: mint::Vector2<f32>,
    norm: mint::Vector3<f32>,
}

unsafe impl bytemuck::Zeroable for Vertex {}
unsafe impl bytemuck::Pod for Vertex {}

fn main() {
    let buf_data: Vec<Vertex> = vec![...];

    pass_bytes_to_gpu(bytemuck::cast_slice(&buf_data));
}

A sidenote - this PR adds bytemuck as an optional dependency - to use it it is needed to enable bytemuck feature in Cargo.toml.

kvark commented 4 years ago

Hmm, this isn't clear to me. Isn't this code possible today without mint depending on bytemuck? How does the PR help the case? You should already be able to have exactly the same unsafe impl bytemuck::Xxx for Vertex today.

rijenkii commented 4 years ago

Uhh.. You're right.. I guess I've had a brainfart earlier today and did not check if it was even possible without this PR. Sorry for being a dum dum, I guess I'll close this PR.

kvark commented 4 years ago

No worries! The story of supporting arrays is definitely missing from mint, it's still a space that needs to be fixed, i.e. by introducing a safe cast_slice inside mint or something.

leod commented 1 year ago

bytemuck now provides derive macros for Zeroable and Pod. These macros require that the types of fields within the struct also implement Zeroable or Pod respectively. Thus, I think it would make sense to revisit this PR.

For example, the following does not compile:

use bytemuck::Zeroable;

#[derive(Zeroable)]
#[repr(C)]
struct MyVertex {
    pos: mint::Vector3<f32>,
}

Compiler error message:

error[E0277]: the trait bound `Vector3<f32>: Zeroable` is not satisfied
 --> src/lib.rs:6:10
  |
6 |     pos: mint::Vector3<f32>,
  |          ^^^^^^^^^^^^^^^^^^ the trait `Zeroable` is not implemented for `Vector3<f32>`
  |
[...]
note: required by a bound in `assert_impl`
 --> src/lib.rs:3:10
  |
3 | #[derive(Zeroable)]
  |          ^^^^^^^^ required by this bound in `assert_impl`
4 | #[repr(C)]
  | - required by a bound in this
  = note: this error originates in the derive macro `Zeroable` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0277`.
kvark commented 1 year ago

@leod yes, you are correct. Looks like we'll need the bytemuck integration after all.

leod commented 1 year ago

Although your point on minimizing the surface area of mint still stands.