Skip to content

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

raldone01
Copy link

@raldone01 raldone01 commented Jun 7, 2025

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.

raldone01 added 3 commits June 7, 2025 14:15
* Add lint rules.
* Add `rustfmt.toml` for predictable formatting.
* Update dependencies.
* Add a shader prelude.
@raldone01 raldone01 force-pushed the changeset/shader_registration branch from feb0678 to 8fc3f63 Compare June 7, 2025 13:26
Copy link
Contributor

@LegNeato LegNeato left a 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.

// UI state
/// Boolean value indicating whether all shaders are rendered in a grid layout.
pub grid_mode: u32,
pub shader_to_show: u32,
Copy link
Contributor

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.

Copy link
Author

@raldone01 raldone01 Jun 8, 2025

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?

Copy link
Author

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?

Copy link

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.

Copy link

@fu5ha fu5ha Jun 9, 2025

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

Copy link
Author

@raldone01 raldone01 Jun 10, 2025

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.

#[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),
Copy link
Contributor

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?


pub const SHADER_DEFINITIONS: &[ShaderDefinition] = &[
$(
$shader_name::SHADER_DEFINITION,
Copy link
Contributor

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.

@raldone01
Copy link
Author

I have pushed a new commit where I did two shaders with a trait.
Would this be acceptable? (before I do the rest)

ShaderDefinition currently only contains the name but would get the functionality for more buffers, float sliders, int slider,... in the future.
I will probably make this into some sort of const builder that can encode all data from the cpu side and decode it on the gpu side.

@LegNeato
Copy link
Contributor

LegNeato commented Jun 8, 2025

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!)

@raldone01
Copy link
Author

raldone01 commented Jun 10, 2025

@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:

  • Using iced for a gui. Currently a bit of a pain because the latest master requires edition 2024 which we don't have. It's manageable though.
  • Give every shader its own pipline and entrypoint. That should improve performance and compile times.
  • Give shaders access to buffers and input guis.

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. shader_context contains all the buffers and inputs from the gui. This also opens the door to supporting all the other shadertoy features.

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants