-
Notifications
You must be signed in to change notification settings - Fork 12
Make shader registration more modular #42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
* Add lint rules. * Add `rustfmt.toml` for predictable formatting. * Update dependencies. * Add a shader prelude.
feb0678
to
8fc3f63
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for doing this! 🍻 A couple of things.
shaders/src/shared_data.rs
Outdated
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer enums with u32 reprs if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome thanks. This is addressed in ebbce16
.
shaders/src/shaders/mod.rs
Outdated
#[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
shaders/src/shaders/mod.rs
Outdated
|
||
pub const SHADER_DEFINITIONS: &[ShaderDefinition] = &[ | ||
$( | ||
$shader_name::SHADER_DEFINITION, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.