rust3ds / citro3d-rs

Rust bindings and safe wrappers for citro3d
https://rust3ds.github.io/citro3d-rs
14 stars 11 forks source link

Rewrite the maths & uniform API #43

Closed 0x00002a closed 9 months ago

0x00002a commented 10 months ago

So theres quite a lot here and I'm aware it might not fly as its a substantial change, though it is mostly non-breaking it does remove some functionality.

Matrix and Matrix3 are gone. My thought process is that there are not many use-cases for the 3x3 stuff since basically all you can use it for is CPU math which the rust ecosystem already has stuff for.

Matrix4's API has been made more explicit about the row order and has been made Copy, also it no longer gives out pointers to the inner as that's just asking for trouble. If unsafe code wants to cast the reference to a pointer that's its business.

Uniform is now an enum rather than a sealed trait. This gives us some nice things:

also I added the missing bindings and fixed the soundness issue where an overflow could occure

glam interop with From impl's for FVec, Matrix4, and Uniform (which lets you bind glam mat4's directly as uniforms), this should help with that CPU math

0x00002a commented 9 months ago

Hey, thanks for this! I think it does simplify things a lot and I think you're right that 3x3 CPU math can be done in plain Rust, or coerced into PICA types for GPU math.

One use case I'm not sure this would support (which maybe the Uniform trait would have) is a custom type that's larger than the default types, e.g.

* `struct BoolVec([bool; 4]);`

* `struct Mat5x5([[f32; 5]; 5]);`

* `struct CustomIVec([u8; 6])`;

I guess maybe the way to handle this kind of thing would be to have a helper function on the type itself, so you'd own a range of indices and call bind multiple times to bind one of these custom types?

So originally I had the float arm of Uniform use an &[FVec4] but I changed it to the fixed version for... reason? after reading the C code I think? There might be a limit of 4 in a row bindings but saying that it doesn't really make sense? so yeah might want to change that back to the &[FVec4] version instead to support this use case, could also switch all the arms to be &[..] (this stops us implementing From for non-references though and its kinda weird).

For stuff like "custom uniforms" the only uniform types that are supported by citro3d C API is bool, ivec, and fvec the fvec version of which has the 1,2,3,4 versions

ian-h-chamberlain commented 9 months ago

For stuff like "custom uniforms" the only uniform types that are supported by citro3d C API is bool, ivec, and fvec the fvec version of which has the 1,2,3,4 versions

Yeah, I guess that's true — I had been hoping it might be possible to implement truly custom uniform types but maybe that's not feasible without implementing everything down to the GPU register writes themselves (like https://github.com/devkitPro/citro3d/blob/master/source/uniforms.c#L54).

I guess let's consider this "out of scope" for now, maybe implementing custom registers is possible but if so it could probably just be another enum variant, so we can deal with that later on.

So originally I had the float arm of Uniform use an &[FVec4] but I changed it to the fixed version for... reason? after reading the C code I think? There might be a limit of 4 in a row bindings but saying that it doesn't really make sense? so yeah might want to change that back to the &[FVec4] version instead to support this use case, could also switch all the arms to be &[..] (this stops us implementing From for non-references though and its kinda weird).

From what I can tell, there isn't necessarily a limit since C3D_FVUnifMtxNx4 seems to allow any num as its input count, but I would be surprised if there are any common use cases for more that 4x4. I was trying to think if we could use Cow<[FVec]> or something, but I don't think that works with array types, only Vec, and we probably don't want to use a Vec for every owned uniform like that.

I think having separate enum variants is fine for now, we could always change it later if it seems like having 5x4 or whatever is useful.

0x00002a commented 9 months ago

For stuff like "custom uniforms" the only uniform types that are supported by citro3d C API is bool, ivec, and fvec the fvec version of which has the 1,2,3,4 versions

Yeah, I guess that's true — I had been hoping it might be possible to implement truly custom uniform types but maybe that's not feasible without implementing everything down to the GPU register writes themselves (like https://github.com/devkitPro/citro3d/blob/master/source/uniforms.c#L54).

Yeah I think at that point we'd want an entirely new rust-native library for using the GPU, which I've been considering writing for a bit now (given the amount of footguns involved in all the state and globals in citro3d) but that's too much time investment for me right now :p

ian-h-chamberlain commented 9 months ago

@0x00002a I think everything looks good to me, are you planning on making any additional changes or should I go ahead and merge this if it passes CI checks?

0x00002a commented 9 months ago

@0x00002a I think everything looks good to me, are you planning on making any additional changes or should I go ahead and merge this if it passes CI checks?

Yeah I'm good with this changeset