Address MR 2253 review comments.

voxygen/src/render/mod.rs

@@ -48,7 +48,7 @@ pub use self::{
     },
     renderer::{
         drawer::{
-            Drawer, FigureDrawer, FigureShadowDrawer, FirstPassDrawer, ParticleDrawer,
+            DebugDrawer, Drawer, FigureDrawer, FigureShadowDrawer, FirstPassDrawer, ParticleDrawer,
             PreparedUiDrawer, SecondPassDrawer, ShadowPassDrawer, SpriteDrawer, TerrainDrawer,
             TerrainShadowDrawer, ThirdPassDrawer, UiDrawer,
         },

voxygen/src/render/pipelines/debug.rs

@@ -24,8 +24,6 @@ impl Vertex {
 }
 
 impl VertexTrait for Vertex {
-    //const QUADS_INDEX: Option<wgpu::IndexFormat> =
-    // Some(wgpu::IndexFormat::Uint32);
     const QUADS_INDEX: Option<wgpu::IndexFormat> = None;
     const STRIDE: wgpu::BufferAddress = mem::size_of::<Self>() as wgpu::BufferAddress;
 }
@@ -42,30 +40,6 @@ pub struct Locals {
 
 pub type BoundLocals = Bound<Consts<Locals>>;
 
-/*gfx_defines! {
-    vertex Vertex {
-        pos: [f32; 3] = "v_pos",
-    }
-
-    constant Locals {
-        // pos is [f32; 4] instead of [f32; 3] so that Locals's size is a multiple of 8 bytes
-        // (which is required by gfx), the last component is ignored by the shader
-        pos: [f32; 4] = "w_pos",
-        color: [f32; 4] = "w_color",
-    }
-
-    pipeline pipe {
-        vbuf: gfx::VertexBuffer<Vertex> = (),
-
-        locals: gfx::ConstantBuffer<Locals> = "u_locals",
-        globals: gfx::ConstantBuffer<Globals> = "u_globals",
-
-        tgt_color: gfx::BlendTarget<TgtColorFmt> = ("tgt_color", gfx::state::ColorMask::all(), gfx::preset::blend::ALPHA),
-        //tgt_depth: gfx::DepthTarget<TgtDepthStencilFmt> = gfx::preset::depth::LESS_EQUAL_TEST,
-        tgt_depth: gfx::DepthTarget<TgtDepthStencilFmt> = gfx::preset::depth::PASS_TEST,
-    }
-}*/
-
 impl From<Vec3<f32>> for Vertex {
     fn from(pos: Vec3<f32>) -> Vertex {
         Vertex {

voxygen/src/render/renderer/drawer.rs

@@ -540,10 +540,7 @@ impl<'pass> FirstPassDrawer<'pass> {
         render_pass.set_pipeline(&self.pipelines.debug.pipeline);
         set_quad_index_buffer::<debug::Vertex>(&mut render_pass, &self.borrow);
 
-        DebugDrawer {
+        DebugDrawer { render_pass }
-            render_pass,
-            globals: self.globals,
-        }
     }
 
     pub fn draw_lod_terrain<'data: 'pass>(&mut self, model: &'data Model<lod_terrain::Vertex>) {
@@ -615,7 +612,6 @@ impl<'pass> FirstPassDrawer<'pass> {
 
 pub struct DebugDrawer<'pass_ref, 'pass: 'pass_ref> {
     render_pass: Scope<'pass_ref, wgpu::RenderPass<'pass>>,
-    globals: &'pass GlobalsBindGroup,
 }
 
 impl<'pass_ref, 'pass: 'pass_ref> DebugDrawer<'pass_ref, 'pass> {
@@ -624,8 +620,6 @@ impl<'pass_ref, 'pass: 'pass_ref> DebugDrawer<'pass_ref, 'pass> {
         model: &'data Model<debug::Vertex>,
         locals: &'data debug::BoundLocals,
     ) {
-        self.render_pass
-            .set_bind_group(0, &self.globals.bind_group, &[]);
         self.render_pass.set_bind_group(1, &locals.bind_group, &[]);
         self.render_pass.set_vertex_buffer(0, model.buf().slice(..));
         self.render_pass.draw(0..model.len() as u32, 0..1);

voxygen/src/scene/debug.rs

@@ -1,7 +1,9 @@
 use crate::render::{
-    Bound, Consts, DebugLocals, DebugVertex, FirstPassDrawer, Mesh, Model, Quad, Renderer, Tri,
+    Bound, Consts, DebugDrawer, DebugLocals, DebugVertex, Mesh, Model, Quad, Renderer, Tri,
 };
+use common::util::srgba_to_linear;
 use hashbrown::{HashMap, HashSet};
+use tracing::warn;
 use vek::*;
 
 #[derive(Debug)]
@@ -13,7 +15,6 @@ pub enum DebugShape {
 impl DebugShape {
     pub fn mesh(&self) -> Mesh<DebugVertex> {
         use core::f32::consts::PI;
-        use DebugShape::*;
         let mut mesh = Mesh::new();
         let tri = |x: Vec3<f32>, y: Vec3<f32>, z: Vec3<f32>| {
             Tri::<DebugVertex>::new(x.into(), y.into(), z.into())
@@ -22,11 +23,11 @@ impl DebugShape {
             Quad::<DebugVertex>::new(x.into(), y.into(), z.into(), w.into())
         };
         match self {
-            Line([a, b]) => {
+            DebugShape::Line([a, b]) => {
                 let h = Vec3::new(0.0, 1.0, 0.0);
                 mesh.push_quad(quad(*a, a + h, b + h, *b));
             },
-            Cylinder { radius, height } => {
+            DebugShape::Cylinder { radius, height } => {
                 const SUBDIVISIONS: usize = 16;
                 for i in 0..SUBDIVISIONS {
                     let angle = |j: usize| (j as f32 / SUBDIVISIONS as f32) * 2.0 * PI;
@@ -49,16 +50,14 @@ impl DebugShape {
 }
 
 #[derive(Copy, Clone, Debug, Hash, PartialEq, Eq)]
-pub struct DebugShapeId(usize);
+pub struct DebugShapeId(u64);
 
 pub struct Debug {
     next_shape_id: DebugShapeId,
     pending_shapes: HashMap<DebugShapeId, DebugShape>,
     pending_locals: HashMap<DebugShapeId, ([f32; 4], [f32; 4])>,
     pending_deletes: HashSet<DebugShapeId>,
-    models: HashMap<DebugShapeId, Model<DebugVertex>>,
+    models: HashMap<DebugShapeId, (Model<DebugVertex>, Bound<Consts<DebugLocals>>)>,
-    //locals: HashMap<DebugShapeId, Consts<DebugLocals>>,
-    locals: HashMap<DebugShapeId, Bound<Consts<DebugLocals>>>,
 }
 
 impl Debug {
@@ -69,7 +68,6 @@ impl Debug {
             pending_locals: HashMap::new(),
             pending_deletes: HashSet::new(),
             models: HashMap::new(),
-            locals: HashMap::new(),
         }
     }
 
@@ -88,41 +86,42 @@ impl Debug {
 
     pub fn maintain(&mut self, renderer: &mut Renderer) {
         for (id, shape) in self.pending_shapes.drain() {
-            self.models
+            if let Some(model) = renderer.create_model(&shape.mesh()) {
-                .insert(id, renderer.create_model(&shape.mesh()).unwrap());
+                let locals = renderer.create_debug_bound_locals(&[DebugLocals {
-            /*self.locals.insert(
-                id,
-                renderer.create_consts(&[DebugLocals {
                     pos: [0.0; 4],
                     color: [1.0, 0.0, 0.0, 1.0],
-                }]),
+                }]);
-            );*/
+                self.models.insert(id, (model, locals));
+            } else {
+                warn!(
+                    "Failed to create model for debug shape {:?}: {:?}",
+                    id, shape
+                );
+            }
         }
         for (id, (pos, color)) in self.pending_locals.drain() {
-            // TODO: what are the efficiency ramifications of creating the constants each
+            if let Some((_, locals)) = self.models.get_mut(&id) {
-            // time instead of caching them and binding them? UI seems to
+                let lc = srgba_to_linear(color.into());
-            // recreate them each time they change?
+                let new_locals = [DebugLocals {
-            /*if let Some(locals) = self.locals.get_mut(&id) {
+                    pos,
-                let new_locals = [DebugLocals { pos, color }];
+                    color: [lc.r, lc.g, lc.b, lc.a],
+                }];
                 renderer.update_consts(locals, &new_locals);
-                renderer.create_debug_bound_locals(new_locals);
+            } else {
-            }*/
+                warn!(
-            let new_locals = [DebugLocals { pos, color }];
+                    "Tried to update locals for nonexistent debug shape {:?}",
-            self.locals
+                    id
-                .insert(id, renderer.create_debug_bound_locals(&new_locals));
+                );
+            }
         }
         for id in self.pending_deletes.drain() {
             self.models.remove(&id);
-            self.locals.remove(&id);
         }
     }
 
-    pub fn render<'a>(&'a self, drawer: &mut FirstPassDrawer<'a>) {
+    pub fn render<'a>(&'a self, drawer: &mut DebugDrawer<'_, 'a>) {
-        let mut debug_drawer = drawer.draw_debug();
+        for (model, locals) in self.models.values() {
-        for (id, model) in self.models.iter() {
+            drawer.draw(model, locals);
-            if let Some(locals) = self.locals.get(id) {
-                debug_drawer.draw(model, locals);
-            }
         }
     }
 }

voxygen/src/scene/mod.rs

@@ -36,6 +36,7 @@ use common::{
 use common_base::span;
 use common_state::State;
 use comp::item::Reagent;
+use hashbrown::HashMap;
 use num::traits::{Float, FloatConst};
 use specs::{Entity as EcsEntity, Join, WorldExt};
 use vek::*;
@@ -1118,7 +1119,56 @@ impl Scene {
                 .render(&mut first_pass.draw_particles(), scene_data);
 
             // Render debug shapes
-            self.debug.render(&mut first_pass);
+            self.debug.render(&mut first_pass.draw_debug());
         }
     }
+
+    pub fn maintain_debug_hitboxes(
+        &mut self,
+        client: &Client,
+        settings: &Settings,
+        hitboxes: &mut HashMap<specs::Entity, DebugShapeId>,
+    ) {
+        let ecs = client.state().ecs();
+        let mut current_entities = hashbrown::HashSet::new();
+        if settings.interface.toggle_hitboxes {
+            let positions = ecs.read_component::<comp::Pos>();
+            let colliders = ecs.read_component::<comp::Collider>();
+            let groups = ecs.read_component::<comp::Group>();
+            for (entity, pos, collider, group) in
+                (&ecs.entities(), &positions, &colliders, groups.maybe()).join()
+            {
+                if let comp::Collider::Box {
+                    radius,
+                    z_min,
+                    z_max,
+                } = collider
+                {
+                    current_entities.insert(entity);
+                    let shape_id = hitboxes.entry(entity).or_insert_with(|| {
+                        self.debug.add_shape(DebugShape::Cylinder {
+                            radius: *radius,
+                            height: *z_max - *z_min,
+                        })
+                    });
+                    let hb_pos = [pos.0.x, pos.0.y, pos.0.z + *z_min, 0.0];
+                    let color = if group == Some(&comp::group::ENEMY) {
+                        [1.0, 0.0, 0.0, 0.5]
+                    } else if group == Some(&comp::group::NPC) {
+                        [0.0, 0.0, 1.0, 0.5]
+                    } else {
+                        [0.0, 1.0, 0.0, 0.5]
+                    };
+                    self.debug.set_pos_and_color(*shape_id, hb_pos, color);
+                }
+            }
+        }
+        hitboxes.retain(|k, v| {
+            let keep = current_entities.contains(k);
+            if !keep {
+                self.debug.remove_shape(*v);
+            }
+            keep
+        });
+    }
 }

voxygen/src/session/mod.rs

@@ -39,7 +39,7 @@ use crate::{
     key_state::KeyState,
     menu::char_selection::CharSelectionState,
     render::Renderer,
-    scene::{camera, terrain::Interaction, CameraMode, DebugShape, DebugShapeId, Scene, SceneData},
+    scene::{camera, terrain::Interaction, CameraMode, DebugShapeId, Scene, SceneData},
     settings::Settings,
     window::{AnalogGameInput, Event, GameInput},
     Direction, Error, GlobalState, PlayState, PlayStateResult,
@@ -142,55 +142,8 @@ impl SessionState {
         span!(_guard, "tick", "Session::tick");
 
         let mut client = self.client.borrow_mut();
-        {
+        self.scene
-            let ecs = client.state().ecs();
+            .maintain_debug_hitboxes(&client, &global_state.settings, &mut self.hitboxes);
-            let mut current_entities = hashbrown::HashSet::new();
-            let scene = &mut self.scene;
-            let hitboxes = &mut self.hitboxes;
-            if global_state.settings.interface.toggle_hitboxes {
-                let positions = ecs.read_component::<Pos>();
-                let colliders = ecs.read_component::<comp::Collider>();
-                let groups = ecs.read_component::<comp::Group>();
-                for (entity, pos, collider, group) in
-                    (&ecs.entities(), &positions, &colliders, groups.maybe()).join()
-                {
-                    if let comp::Collider::Box {
-                        radius,
-                        z_min,
-                        z_max,
-                    } = collider
-                    {
-                        current_entities.insert(entity);
-                        let shape_id = hitboxes.entry(entity).or_insert_with(|| {
-                            scene.debug.add_shape(DebugShape::Cylinder {
-                                radius: *radius,
-                                height: *z_max - *z_min,
-                            })
-                        });
-                        let hb_pos = [pos.0.x, pos.0.y, pos.0.z + *z_min, 0.0];
-                        let color = if group == Some(&comp::group::ENEMY) {
-                            [1.0, 0.0, 0.0, 0.5]
-                        } else if group == Some(&comp::group::NPC) {
-                            [0.0, 0.0, 1.0, 0.5]
-                        } else {
-                            [0.0, 1.0, 0.0, 0.5]
-                        };
-                        scene.debug.set_pos_and_color(*shape_id, hb_pos, color);
-                    }
-                }
-            }
-            let mut to_remove = Vec::new();
-            hitboxes.retain(|k, v| {
-                let keep = current_entities.contains(k);
-                if !keep {
-                    to_remove.push(*v);
-                }
-                keep
-            });
-            for shape_id in to_remove.into_iter() {
-                scene.debug.remove_shape(shape_id);
-            }
-        }
         for event in client.tick(self.inputs.clone(), dt, crate::ecs::sys::add_local_systems)? {
             match event {
                 client::Event::Chat(m) => {