Make shader registration more modular#42
Conversation
* Add lint rules. * Add `rustfmt.toml` for predictable formatting. * Update dependencies. * Add a shader prelude.
feb0678 to
8fc3f63
Compare
LegNeato
left a comment
There was a problem hiding this comment.
Thanks so much for doing this! 🍻 A couple of things.
| // UI state | ||
| /// Boolean value indicating whether all shaders are rendered in a grid layout. | ||
| pub grid_mode: u32, | ||
| pub shader_to_show: u32, |
There was a problem hiding this comment.
I'd prefer enums with u32 reprs if possible.
There was a problem hiding this comment.
#[repr(C, u32)]
#[derive(Copy, Clone, NoUninit, Zeroable)]
pub enum DisplayMode {
Grid,
SingleShader(u32),
}
#[repr(C)]
#[derive(Copy, Clone, NoUninit, Zeroable)]
#[allow(unused_attributes)]
pub struct ShaderConstants {
pub width: u32,
pub height: u32,
pub time: f32,
// UI state
pub display_mode: DisplayMode,
// Mouse state.
pub cursor_x: f32,
pub cursor_y: f32,
pub drag_start_x: f32,
pub drag_start_y: f32,
pub drag_end_x: f32,
pub drag_end_y: f32,
pub mouse_left_pressed: u32,
pub mouse_left_clicked: u32,
}Something like this right?
I am not very familiar with bytemuck and it seems to not support enums at first glance.
At least for the Pod guarantee.
Maybe a lesser one would suffice here?
I assume bytemuck is needed to ensure that the struct layout between spirv and cpu matches right?
There was a problem hiding this comment.
There is some activity on Lokathor/bytemuck#292. Maybe if it gets merged in the next week this will become possible by using NoUninit instead of Pod. @LegNeato do you know if the Pod guarantees are really necessary? What is important when sharing data with the gpu? How different is the layout of spirv and rust?
There was a problem hiding this comment.
I assume bytemuck is needed to ensure that the struct layout between spirv and cpu matches right?
Unlikely given that just having bytemuck traits would/does not guarantee that. Instead it's probably used to allow casting to a &[u8] when copying to a GPU buffer. That would only require NoUninit, not Pod or Zeroable, so would be possible when that PR lands.
Technically, being able to cast to &[u8] is not a requirement to copy data to the GPU safely, but it may practically be a requirement depending on how wgpu exposes interfaces to copy into buffers which I don't know off the top of my head.
There was a problem hiding this comment.
this is the call that makes bytemuck traits needed. Indeed NoUninit is the only requirement for bytemuck::bytes_of, and wgpu does enforce a fully-initialized byte slice here so we do indeed need to ensure NoUninit
| #[inline(always)] | ||
| pub fn render_shader(shader_index: u32, shader_input: &ShaderInput, shader_output: &mut ShaderResult) { | ||
| match_index!(shader_index; $( | ||
| $shader_name::shader_fn(shader_input, shader_output), |
There was a problem hiding this comment.
Not sure we should do this, it leads to a lot of copied and pasted shader_fn in the various models and is "stringly" typed outside of the type system. Can we create a trait they all implement or make this generic?
|
|
||
| pub const SHADER_DEFINITIONS: &[ShaderDefinition] = &[ | ||
| $( | ||
| $shader_name::SHADER_DEFINITION, |
There was a problem hiding this comment.
Similar with this, it seems like it should be a trait that is implemented rather than loosely relying on names to match.
|
I have pushed a new commit where I did two shaders with a trait.
|
|
Awesome, I like that better, thanks! If we can't do enums, maybe do a newtype with (inline) function calls that return enums / options? One of the strengths of rust over other shader langs is the type system, so it is not as idiomatic to use primitive values rather than semantic types (though there are still limitations in rust-gpu of course!) |
|
@LegNeato I would revert the Shader trait. It's a lot of work to rewrite everything and I have some grand changes in the works that would invalidate that grunt work anyways. I am working on the following:
Here is my current design draft for the combined shader/cpu side: pub mod phantom_star_2 {
use super::*;
define_shader!(pub struct ShaderDefinition {
name: "Phantom Star",
uniform_buffers: {
buffer_a: { type: BufferA, binding: 0 },
buffer_b: { type: BufferB, binding: 1 },
},
const_parameters: {
my_int_slider: ParameterIntSlider {
min: 0,
max: 100,
default: 50,
label: "My Int Slider",
description: "This is a slider for an integer value.",
step: 1,
},
},
});
#[derive(Zeroable)]
pub struct BufferA {
my_data: [f32; 3],
}
#[derive(Zeroable)]
pub struct BufferB {
my_other_data: [f32; 3],
}
fn main_image(
shader_input: &ShaderInput,
shader_context: &mut ShaderContext<'_>,
frag_color: &mut Vec4,
frag_coord: Vec2,
) {
}
}The macro generates the pipline and gui stuff for the cpu and the shader side should be obvious. What do you think about this? Would you be interested in these changes? (Making good progress over at https://github.com/raldone01/rust-gpu-shadertoys/tree/feat/individual_entrypoints) |
This makes the shader registration more modular.
It also allows the cpu to get information about the active shader such as the name.
This is the base for building more shader specific inputs/outputs.
The registration interface is not perfect yet.
In the future I want it to be trait I think.
For now I would leave it like that.
It is also very convenient to comment out the shaders one doesn't need to speed up compile times.
The grid is now also adaptive to the number of active shaders and attempts to keep the aspect ratio while leaving no empty cells.
I would revisit the inline attributes once there is some proper benchmarking.
This is the first changeset from #40.
It is ready for review.