Add a couple of thin validations to avoid footguns

After spending several hours last week getting tripped up on very silly
issues I have decided to add validations that hope to protect myself
from bad GLSL uniform alignment and bind groups not being reboud on
resize if they rely on resources which get recreated on resize.

The first of which is a much harder thing to validate from my
perspective, so in the hopes of not adding too much overhead to playing
with the uniforms I have decided to require each to implement an
arbitrary trait, `AlignedGLSL`, that defines the `validate_alignment`
function whenever a uniform is registered. At the moment all these
functions do is `assert_eq!` between a constant value I manually input
and the output of `std::mem::size_of` for each uniform. At the very
least, this should keep alignment at my forethought when editing these
uniforms and catch *most* cases of updating the structs without
considering alignment. The edge case which could be painful is reworking
the struct into a malaligned state but same overall size.

The second validation which is bit more well done in terms of actual
validation is for `surface_bound` bind groups as I call them. As long as
I remember to include any new `surface_bound` bind groups in the
relevant `bind_group` calls there ought to be a very obvious panicking
if a resize happens and a previously bound `surface_bound_bind_group`
manages to not get rebound.

atlas/Cargo.toml

@@ -14,4 +14,4 @@ bytemuck = "1.4"
 
 [dependencies.wgpu]
 version = "0.12"
-features = ["spirv"]
+features = ["spirv"]

atlas/src/rendering/init/texture.rs

@@ -165,5 +165,7 @@ fn init_bind_group_for_textured_pass<'a>(
         label: Some(label),
     });
 
-    let _handle = ctx.wrangler.add_bind_group(sampler_bind_group, label);
+    let _handle = ctx
+        .wrangler
+        .add_surface_bound_bind_group(sampler_bind_group, label);
 }

atlas/src/rendering/init/water.rs

@@ -322,7 +322,7 @@ pub fn init_water_resources<'a>(ctx: &mut Context<'a>, label: &'a str) {
     );
     let _handle = ctx
         .wrangler
-        .add_bind_group(texture_sampler_bind_group, WATER_TEXTURE_SAMPLER_UNIFORM);
+        .add_surface_bound_bind_group(texture_sampler_bind_group, WATER_TEXTURE_SAMPLER_UNIFORM);
 }
 
 pub fn init_water_pass<'a>(ctx: &mut Context<'a>) -> PassHandle<'a> {

atlas/src/rendering/mod.rs

@@ -157,13 +157,13 @@ where
     handle
 }
 
-pub fn register_texture<'a>(
+fn texture_bind_group<'a>(
     ctx: &mut Context<'a>,
-    texture: Texture,
+    texture: &Texture,
     label: &'a str,
     layout_label: &'a str,
     sampler_override: Option<&wgpu::Sampler>,
-) -> (BindGroupHandle<'a>, TextureHandle<'a>) {
+) -> wgpu::BindGroup {
     let bglh = ctx
         .wrangler
         .handle_to_bind_group_layout(layout_label)
@@ -189,13 +189,40 @@ pub fn register_texture<'a>(
         ],
         label: Some(&format!("{} Sampler Bind Group", label)),
     });
+    bind_group
+}
+
+pub fn register_texture<'a>(
+    ctx: &mut Context<'a>,
+    texture: Texture,
+    label: &'a str,
+    layout_label: &'a str,
+    sampler_override: Option<&wgpu::Sampler>,
+) -> (BindGroupHandle<'a>, TextureHandle<'a>) {
+    let bind_group = texture_bind_group(ctx, &texture, label, layout_label, sampler_override);
 
     (
         ctx.wrangler.add_or_swap_bind_group(bind_group, label),
         ctx.wrangler.add_or_swap_texture(texture, label),
     )
 }
 
+pub fn register_surface_bound_texture<'a>(
+    ctx: &mut Context<'a>,
+    texture: Texture,
+    label: &'a str,
+    layout_label: &'a str,
+    sampler_override: Option<&wgpu::Sampler>,
+) -> (BindGroupHandle<'a>, TextureHandle<'a>) {
+    let bind_group = texture_bind_group(ctx, &texture, label, layout_label, sampler_override);
+
+    (
+        ctx.wrangler
+            .add_or_swap_surface_bound_bind_group(bind_group, label),
+        ctx.wrangler.add_or_swap_texture(texture, label),
+    )
+}
+
 pub fn recreate_water_sampler_bind_group(ctx: &mut Context) {
     use init::*;
     let texture_sampler_bind_group_layout = ctx
@@ -230,7 +257,8 @@ pub fn recreate_water_sampler_bind_group(ctx: &mut Context) {
         label: Some(WATER_TEXTURE_SAMPLER_UNIFORM),
     });
 
-    let _handle = ctx
-        .wrangler
-        .add_or_swap_bind_group(texture_sampler_bind_group, WATER_TEXTURE_SAMPLER_UNIFORM);
+    let _handle = ctx.wrangler.add_or_swap_surface_bound_bind_group(
+        texture_sampler_bind_group,
+        WATER_TEXTURE_SAMPLER_UNIFORM,
+    );
 }

atlas/src/rendering/uniforms.rs

@@ -2,7 +2,15 @@ use pantheon::graphics::prelude::*;
 use pantheon::math::prelude::*;
 use wgpu::util::DeviceExt;
 
-pub trait Uniforms: 'static + Sized + Send + Sync + std::fmt::Debug {
+pub trait AlignedGLSL {
+    /// This doesn't do any enforcment of alignment because I have no idea how to do that that
+    /// wouldn't just require manual entry anyways, just requires me to keep updating this function
+    /// if the size of the struct changes
+    /// https://learnopengl.com/Advanced-OpenGL/Advanced-GLSL
+    fn validate_alignment(&self);
+}
+
+pub trait Uniforms: 'static + Sized + Send + Sync + std::fmt::Debug + AlignedGLSL {
     fn as_bytes(&self) -> &[u8] {
         unsafe {
             let data_ptr: *const Self = self;
@@ -23,6 +31,8 @@ pub trait Uniforms: 'static + Sized + Send + Sync + std::fmt::Debug {
         layout_label: &'a str,
         label: &'a str,
     ) -> (BindGroupHandle<'a>, BufferHandle<'a>) {
+        self.validate_alignment();
+
         let bind_group_layout_handle = ctx
             .wrangler
             .handle_to_bind_group_layout(&layout_label)
@@ -56,16 +66,18 @@ pub trait Uniforms: 'static + Sized + Send + Sync + std::fmt::Debug {
     }
 }
 
+type Padding32 = u32;
+type Padding64 = u64;
+
 impl Uniforms for CameraUniforms {}
 #[derive(Debug, Clone, Copy)]
 #[repr(C)]
 pub struct CameraUniforms {
     pub view: Mat4,
     pub projection: Mat4,
     pub position: Vec3,
-    _pad0: u32,
+    _pad0: Padding32,
     pub planes: Vec2,
-    _pad1: u64,
 }
 
 impl CameraUniforms {
@@ -76,21 +88,31 @@ impl CameraUniforms {
             position,
             planes,
             _pad0: 0,
-            _pad1: 0,
         }
     }
 }
+impl AlignedGLSL for CameraUniforms {
+    fn validate_alignment(&self) {
+        assert_eq!(64 + 64 + 12 + 4 + 8, std::mem::size_of::<Self>());
+    }
+}
 
 impl Uniforms for GlobalLightUniforms {}
 #[derive(Debug, Clone, Copy)]
 #[repr(C)]
+
 pub struct GlobalLightUniforms {
     pub direction: Vec3,
-    _pad0: u32,
+    _pad0: Padding32,
     pub color: Vec3,
-    _pad1: u32,
+    _pad1: Padding32,
     pub bias: Vec2,
-    _pad2: u64,
+    _pad2: Padding64,
+}
+impl AlignedGLSL for GlobalLightUniforms {
+    fn validate_alignment(&self) {
+        assert_eq!(2 + 4 + 12 + 4 + 8 + 8, std::mem::size_of::<Self>());
+    }
 }
 
 impl GlobalLightUniforms {
@@ -112,6 +134,11 @@ impl Uniforms for StaticEntityUniforms {}
 pub struct StaticEntityUniforms {
     pub model_matrix: Mat4,
 }
+impl AlignedGLSL for StaticEntityUniforms {
+    fn validate_alignment(&self) {
+        assert_eq!(64, std::mem::size_of::<Self>());
+    }
+}
 
 impl StaticEntityUniforms {
     pub fn new(model_matrix: Mat4) -> Self {

game-client/assets/shaders/shaded.vert

@@ -13,6 +13,7 @@ layout(set=0, binding=0) uniform Camera {
     mat4 view;
     mat4 projection;
     vec3 position;
+    vec2 planes;
 } camera;
 
 layout(set=0, binding=1) uniform GlobalLight {

game-client/src/main.rs

@@ -276,30 +276,32 @@ impl<'a> EventHandler<'a> for State<'a> {
             .swap_texture(&self.handles.depth_texture, depth_texture);
         */
 
-        rendering::register_texture(
+        ctx.wrangler.start_resize();
+
+        rendering::register_surface_bound_texture(
             ctx,
             depth_texture,
             "depth",
             "basic_textured",
             Some(&Texture::surface_texture_sampler(&ctx.device)),
         );
 
-        rendering::register_texture(
+        rendering::register_surface_bound_texture(
             ctx,
             refraction_depth_texture,
             "refraction_depth",
             "basic_textured",
             Some(&Texture::surface_texture_sampler(&ctx.device)),
         );
 
-        rendering::register_texture(
+        rendering::register_surface_bound_texture(
             ctx,
             reflection_texture,
             self.handles.reflection_texture.label,
             "basic_textured",
             None,
         );
-        rendering::register_texture(
+        rendering::register_surface_bound_texture(
             ctx,
             refraction_texture,
             self.handles.refraction_texture.label,
@@ -308,6 +310,8 @@ impl<'a> EventHandler<'a> for State<'a> {
         );
 
         rendering::recreate_water_sampler_bind_group(ctx);
+
+        ctx.wrangler.validate_resize();
     }
 
     fn key_mods_changed(&mut self, _ctx: &mut Context, _modifiers_state: ModifiersState) {}
@@ -397,7 +401,7 @@ async fn main() {
     let message: Message<GameMessage> = Message::new(GameMessage::SyncWorld);
     network_client.send(message).await.unwrap();
 
-    let shader_path = std::path::PathBuf::from("game_client/assets/shaders");
+    let shader_path = std::path::PathBuf::from("game-client/assets/shaders");
     let (mut ctx, event_loop) = Context::new(Color::new(135, 206, 235).into(), shader_path);
 
     // @NOTE this has to be 0 unless we want out camera to be paramertized against the water's

pantheon/src/graphics/wrangler.rs

@@ -7,16 +7,20 @@ use crate::shader::ShaderContext;
 pub struct RenderWrangler<'a> {
     pub passes: Vec<Pass<'a>>,
 
-    //@TODO swap LabeledEntry for PhantomData enforced types
     pub bind_group_layouts: Vec<LabeledEntry<'a, wgpu::BindGroupLayout>>,
     pub bind_groups: Vec<LabeledEntry<'a, wgpu::BindGroup>>,
     pub vertex_buffers: Vec<LabeledEntry<'a, wgpu::Buffer>>,
     pub vertex_buffer_cursors: Vec<LabeledEntry<'a, wgpu::BufferAddress>>,
     pub index_buffers: Vec<LabeledEntry<'a, wgpu::Buffer>>,
     pub index_buffer_cursors: Vec<LabeledEntry<'a, wgpu::BufferAddress>>,
+    // @TODO @SPEED this should probably just be broken down as per
+    // https://github.com/gfx-rs/wgpu/wiki/Do%27s-and-Dont%27s#do-group-resource-bindings-by-the-change-frequency-start-from-the-lowest
     pub uniform_buffers: Vec<LabeledEntry<'a, wgpu::Buffer>>,
     pub textures: Vec<LabeledEntry<'a, Texture>>,
     pub draw_calls: Vec<LabeledEntry<'a, DrawCall<'a>>>,
+    // validation stuff
+    surface_bound_bind_group_count: usize,
+    surface_bound_bind_group_cursor: usize,
 }
 impl<'a> RenderWrangler<'a> {
     pub fn new() -> Self {
@@ -31,6 +35,8 @@ impl<'a> RenderWrangler<'a> {
             uniform_buffers: Vec::new(),
             textures: Vec::new(),
             draw_calls: Vec::new(),
+            surface_bound_bind_group_count: 0,
+            surface_bound_bind_group_cursor: 0,
         }
     }
 
@@ -135,6 +141,76 @@ impl<'a> RenderWrangler<'a> {
         }
     }
 
+    pub fn start_resize(&mut self) {
+        self.surface_bound_bind_group_cursor = 0;
+    }
+
+    pub fn validate_resize(&self) {
+        if self.surface_bound_bind_group_count != self.surface_bound_bind_group_cursor {
+            panic!(
+                "[Wrangler.validate_resize] Expected {} surface bound bind groups rebounded after resize, counted {}",
+                self.surface_bound_bind_group_count, self.surface_bound_bind_group_cursor
+            );
+        }
+    }
+
+    /// use this for any bind group that needs to be recreated on resize, e.g. depth texutres /
+    /// color attachments for additional passes
+    pub fn add_surface_bound_bind_group(
+        &mut self,
+        bind_group: wgpu::BindGroup,
+        label: &'a str,
+    ) -> BindGroupHandle<'a> {
+        #[cfg(debug_assertions)]
+        {
+            self.surface_bound_bind_group_count += 1;
+            self.surface_bound_bind_group_cursor += 1;
+        }
+
+        let idx = self.bind_groups.len();
+        self.bind_groups.push(LabeledEntry {
+            label,
+            entry: bind_group,
+        });
+        BindGroupHandle {
+            label,
+            idx,
+            marker: PhantomData,
+        }
+    }
+
+    pub fn add_or_swap_surface_bound_bind_group(
+        &mut self,
+        bind_group: wgpu::BindGroup,
+        label: &'a str,
+    ) -> BindGroupHandle<'a> {
+        if let Some(handle) = self.handle_to_bind_group(label) {
+            #[cfg(debug_assertions)]
+            {
+                self.surface_bound_bind_group_cursor += 1;
+            }
+            self.bind_groups[handle.idx].entry = bind_group;
+
+            handle
+        } else {
+            #[cfg(debug_assertions)]
+            {
+                self.surface_bound_bind_group_count += 1;
+                self.surface_bound_bind_group_cursor += 1;
+            }
+            let idx = self.bind_groups.len();
+            self.bind_groups.push(LabeledEntry {
+                label,
+                entry: bind_group,
+            });
+            BindGroupHandle {
+                label,
+                idx,
+                marker: PhantomData,
+            }
+        }
+    }
+
     /// Returns first matching label, no guarentee of being the only/expected one
     pub fn handle_to_bind_group(&self, label: &'a str) -> Option<BindGroupHandle<'a>> {
         let idx = self

pantheon/src/math/vec3.rs

@@ -2,6 +2,7 @@ use crate::math::Vec4;
 use std::ops::{Add, AddAssign, Mul, MulAssign, Sub, SubAssign};
 
 // @TODO think about vector equality, may need to implement floating point epsilon comparisons
+#[repr(C)]
 #[derive(Clone, Copy, Debug, PartialEq)]
 pub struct Vec3 {
     pub x: f32,