[Merged by Bors] - use bytemuck crate instead of Byteable trait #2183
Conversation
alice-i-cecile
added
the
C-Code-Quality
| // FIXME: `Bytes` trait doesn't specify the expected encoding format, | ||
| // which means types that implement it have to know what format is expected | ||
| // and can only implement one encoding at a time. | ||
| // TODO: Remove `Bytes` and `FromBytes` in favour of `crevice` crate. |
There was a problem hiding this comment.
We should make an issue for this (once this PR gets merged) that way we we're able to track the progress for this TODO.
There was a problem hiding this comment.
Sure, but It's not really part of this PR. There is some progress in #1931 and afaik cart has some private branch going on as well.
| @@ -17,6 +16,7 @@ use bevy_render::{ | |||
| }, | |||
| }; | |||
| use bevy_transform::prelude::*; | |||
| use bytemuck::{bytes_of, Pod, Zeroable}; | |||
There was a problem hiding this comment.
| use bytemuck::{bytes_of, Pod, Zeroable}; | |
| use bytemuck::{Pod, Zeroable}; | |
| use bevy_core::bytes_of; |
Same here with the reexported things.
There was a problem hiding this comment.
the reason for that is I have to use bytemuck directly because of derive macro. I didn't wanted to unnecessarily split the imports here, as the bevy_core imports are aliases anyway.
There was a problem hiding this comment.
Is the goal to eventually use bevy specific exports? If so we should use bevy_core when possible. Else, using bytemuck directly is ok :)
There was a problem hiding this comment.
I do have a general overarching requirement for the bevy crate to be standalone for "normal workflows". This includes derives that make types byte-convertible for use on the gpu. Short term I'm cool with lifting this requirement, but before the next release we'll need to sort out the best way to make that happen.
Worst case scenario we just fork the derive macro, but we can probably do better than that.
Additionally, I don't think Pod and Zeroable are particularly "user friendly" names for making something byte convertible. I also don't like that two derives are required. I understand why they are what they are, but I have a strong inclination to unify them and give them a friendly name like Byteable. I understand that this opens a can of worms (ex: how would it interact with downstream crates like crevice). But I like being UX-first in Bevy. We should design the interface that we want, then make it happen. The existing ecosystem (generally) shouldn't dictate our apis.
There was a problem hiding this comment.
I think we could have our own derive that implements both Pod and Zeroable, and still have our own trait that represents both, like
trait Byteable: bytemuck::Zeroable + bytemuck::Pod {}
impl<T> Byteable for T where T: bytemuck::Zeroable + bytemuck::Pod {}then have a derive macro that simply delegates to bytemuck derives, like that
#[derive(Byteable)]
#[repr(C)]
struct MyByteableData { .. }Same idea could apply to hypothetical UniformData or VertexData traits, that delegate to crevice's AsStd140 and AsStd430.
In order to implement that, we would either need to fork both derive crates, or persuade the maintainers to publish extra intermediate crate with derive implementation that we could import in our proc-macro crate.
But in reality, I think that use-cases for both of those derives are fairly advanced. You only really need them if you are implementing your own rendering code, which isn't likely to happen the first time you are using the engine. So maybe it's allright to require some extra dependencies when you want to use the macros. That would cooperate with the ecosystem a bit better, people would know to enable "bytemuck" or "crevice" feature in whatever lib they are integrating with in order to make it's types work in rendering.
There was a problem hiding this comment.
That would cooperate with the ecosystem a bit better, people would know to enable "bytemuck" or "crevice" feature in whatever lib they are integrating with in order to make it's types work in rendering.
I think that class of "ecosystem trait impl" would "just work" for a Byteable combo trait.
But in reality, I think that use-cases for both of those derives are fairly advanced.
I think as soon as people start building "real games" in Bevy, custom shaders (and passing data to them) will be much more commonplace. I want to streamline that user experience as much as possible. Also selfishly I spend a lot of time in that space when game-deving and I want it to feel good.
|
bors r+ |
This gets rid of multiple unsafe blocks that we had to maintain ourselves, and instead depends on library that's commonly used and supported by the ecosystem. We also get support for glam types for free. There is still some things to clear up with the `Bytes` trait, but that is a bit more substantial change and can be done separately. Also there are already separate efforts to use `crevice` crate, so I've just added that as a TODO.
|
Pull request successfully merged into main. Build succeeded: |
bors
bot
changed the title
This gets rid of multiple unsafe blocks that we had to maintain ourselves, and instead depends on library that's commonly used and supported by the ecosystem. We also get support for glam types for free. There is still some things to clear up with the `Bytes` trait, but that is a bit more substantial change and can be done separately. Also there are already separate efforts to use `crevice` crate, so I've just added that as a TODO.
This gets rid of multiple unsafe blocks that we had to maintain ourselves, and instead depends on library that's commonly used and supported by the ecosystem. We also get support for glam types for free.
There is still some things to clear up with the
Bytestrait, but that is a bit more substantial change and can be done separately. Also there are already separate efforts to usecrevicecrate, so I've just added that as a TODO.