From 308cbbb998cfd03a7ae7101a2995818dc8370b25 Mon Sep 17 00:00:00 2001
From: Imbris <imbrisf@gmail.com>
Date: Thu, 13 May 2021 01:25:28 -0400
Subject: [PATCH] Address various TODOs introduced in wgpu transition
---
assets/voxygen/shaders/skybox-vert.glsl | 10 +++-
assets/voxygen/shaders/terrain-vert.glsl | 1 +
voxygen/src/mesh/greedy.rs | 10 ++--
voxygen/src/render/pipelines/mod.rs | 8 +--
voxygen/src/render/pipelines/shadow.rs | 4 +-
voxygen/src/render/pipelines/terrain.rs | 14 ++++-
voxygen/src/render/renderer.rs | 7 ++-
voxygen/src/render/renderer/drawer.rs | 4 --
voxygen/src/scene/terrain.rs | 73 ++++++++++--------------
9 files changed, 66 insertions(+), 65 deletions(-)
diff --git a/assets/voxygen/shaders/skybox-vert.glsl b/assets/voxygen/shaders/skybox-vert.glsl
index e3eaf45546..9f362fedb7 100644
--- a/assets/voxygen/shaders/skybox-vert.glsl
+++ b/assets/voxygen/shaders/skybox-vert.glsl
@@ -25,8 +25,16 @@ layout(location = 0) out vec3 f_pos;
void main() {
f_pos = v_pos;
- // TODO: is this todo below still valid? is cam_pos jittery
// TODO: Make this position-independent to avoid rounding error jittering
+ // NOTE: we may or may not want to use an infinite projection here
+ //
+ // Essentially: using any finite projection is likely wrong here if we want
+ // to project out to infinity, but since we want to perturb the skybox as we
+ // move and we have stars now, the "right" answer is heavily dependent on
+ // how we compute cloud position and stuff.
+ //
+ // Infinite projections of cubemaps are nice because they can be oriented
+ // but still extend infinitely far.
gl_Position =
all_mat *
vec4(v_pos + cam_pos.xyz, 1);
diff --git a/assets/voxygen/shaders/terrain-vert.glsl b/assets/voxygen/shaders/terrain-vert.glsl
index a1b04c2946..7fac4a0d2e 100644
--- a/assets/voxygen/shaders/terrain-vert.glsl
+++ b/assets/voxygen/shaders/terrain-vert.glsl
@@ -32,6 +32,7 @@ layout (std140, set = 2, binding = 0)
uniform u_locals {
vec3 model_offs;
float load_time;
+ // TODO: consider whether these need to be signed
ivec4 atlas_offs;
};
diff --git a/voxygen/src/mesh/greedy.rs b/voxygen/src/mesh/greedy.rs
index 839d443cd1..ef8f4f5014 100644
--- a/voxygen/src/mesh/greedy.rs
+++ b/voxygen/src/mesh/greedy.rs
@@ -82,7 +82,7 @@ pub type SuspendedMesh<'a> = dyn for<'r> FnOnce(&'r mut ColLightInfo) + 'a;
/// For an explanation of why we want this, see `SuspendedMesh`.
pub struct GreedyMesh<'a> {
atlas: guillotiere::SimpleAtlasAllocator,
- col_lights_size: Vec2<u32>,
+ col_lights_size: Vec2<u16>,
max_size: guillotiere::Size,
suspended: Vec<Box<SuspendedMesh<'a>>>,
}
@@ -190,7 +190,7 @@ impl<'a> GreedyMesh<'a> {
fn greedy_mesh<'a, M: PartialEq, D: 'a, FL, FG, FO, FS, FP, FT>(
atlas: &mut guillotiere::SimpleAtlasAllocator,
- col_lights_size: &mut Vec2<u32>,
+ col_lights_size: &mut Vec2<u16>,
max_size: guillotiere::Size,
GreedyConfig {
mut data,
@@ -428,7 +428,7 @@ fn add_to_atlas(
norm: Vec3<i16>,
faces_forward: bool,
max_size: guillotiere::Size,
- cur_size: &mut Vec2<u32>,
+ cur_size: &mut Vec2<u16>,
) -> guillotiere::Rectangle {
// TODO: Check this conversion.
let atlas_rect;
@@ -473,8 +473,8 @@ fn add_to_atlas(
// a u16 and we never grew the atlas, meaning all valid coordinates within the
// atlas also fit into a u16.
*cur_size = Vec2::new(
- cur_size.x.max(atlas_rect.max.x as u32),
- cur_size.y.max(atlas_rect.max.y as u32),
+ cur_size.x.max(atlas_rect.max.x as u16),
+ cur_size.y.max(atlas_rect.max.y as u16),
);
// NOTE: pos can be converted safely from usize to i32 because all legal block
diff --git a/voxygen/src/render/pipelines/mod.rs b/voxygen/src/render/pipelines/mod.rs
index bb55a288a0..846a4d7339 100644
--- a/voxygen/src/render/pipelines/mod.rs
+++ b/voxygen/src/render/pipelines/mod.rs
@@ -124,11 +124,11 @@ impl Globals {
shadow_planes.x,
shadow_planes.y,
],
+ // TODO: why do we accept values greater than the max?
light_shadow_count: [
- // TODO: why do we accept values greater than the max?
- (light_count % (MAX_POINT_LIGHT_COUNT + 1)) as u32,
- (shadow_count % (MAX_FIGURE_SHADOW_COUNT + 1)) as u32,
- (directed_light_count % (MAX_DIRECTED_LIGHT_COUNT + 1)) as u32,
+ usize::min(light_count, MAX_POINT_LIGHT_COUNT) as u32,
+ usize::min(shadow_count, MAX_FIGURE_SHADOW_COUNT) as u32,
+ usize::min(directed_light_count, MAX_DIRECTED_LIGHT_COUNT) as u32,
0,
],
shadow_proj_factors: [
diff --git a/voxygen/src/render/pipelines/shadow.rs b/voxygen/src/render/pipelines/shadow.rs
index cc3262b4d7..e51069c28d 100644
--- a/voxygen/src/render/pipelines/shadow.rs
+++ b/voxygen/src/render/pipelines/shadow.rs
@@ -82,8 +82,8 @@ pub fn create_col_lights(
let texture_info = wgpu::TextureDescriptor {
label: None,
size: wgpu::Extent3d {
- width: col_lights_size.x,
- height: col_lights_size.y,
+ width: u32::from(col_lights_size.x),
+ height: u32::from(col_lights_size.y),
depth_or_array_layers: 1,
},
mip_level_count: 1,
diff --git a/voxygen/src/render/pipelines/terrain.rs b/voxygen/src/render/pipelines/terrain.rs
index 6fc245e7d8..8a379f589d 100644
--- a/voxygen/src/render/pipelines/terrain.rs
+++ b/voxygen/src/render/pipelines/terrain.rs
@@ -141,12 +141,20 @@ impl VertexTrait for Vertex {
#[derive(Copy, Clone, Debug, Zeroable, Pod)]
// TODO: new function and private fields??
pub struct Locals {
- pub model_offs: [f32; 3],
- pub load_time: f32,
- pub atlas_offs: [i32; 4],
+ model_offs: [f32; 3],
+ load_time: f32,
+ atlas_offs: [i32; 4],
}
impl Locals {
+ pub fn new(model_offs: Vec3<f32>, atlas_offs: Vec2<u32>, load_time: f32) -> Self {
+ Self {
+ model_offs: model_offs.into_array(),
+ load_time,
+ atlas_offs: Vec4::new(atlas_offs.x as i32, atlas_offs.y as i32, 0, 0).into_array(),
+ }
+ }
+
pub fn default() -> Self {
Self {
model_offs: [0.0; 3],
diff --git a/voxygen/src/render/renderer.rs b/voxygen/src/render/renderer.rs
index 4602da5e1b..bc77931326 100644
--- a/voxygen/src/render/renderer.rs
+++ b/voxygen/src/render/renderer.rs
@@ -38,7 +38,7 @@ use vek::*;
/// A type representing data that can be converted to an immutable texture map
/// of ColLight data (used for texture atlases created during greedy meshing).
// TODO: revert to u16
-pub type ColLightInfo = (Vec<[u8; 4]>, Vec2<u32>);
+pub type ColLightInfo = (Vec<[u8; 4]>, Vec2<u16>);
const QUAD_INDEX_BUFFER_U16_START_VERT_LEN: u16 = 3000;
const QUAD_INDEX_BUFFER_U32_START_VERT_LEN: u32 = 3000;
@@ -678,7 +678,10 @@ impl Renderer {
.unwrap_or_else(|| (Vec2::new(1, 1), Vec2::new(1, 1)))
}
- // TODO: @Sharp what should this look like with wgpu?
+ // TODO: Seamless is potentially the default with wgpu but we need further
+ // investigation into whether this is actually turned on for the OpenGL
+ // backend
+ //
/// NOTE: Supported by Vulkan (by default), DirectX 10+ (it seems--it's hard
/// to find proof of this, but Direct3D 10 apparently does it by
/// default, and 11 definitely does, so I assume it's natively supported
diff --git a/voxygen/src/render/renderer/drawer.rs b/voxygen/src/render/renderer/drawer.rs
index 4e45488e13..ca8bb7e824 100644
--- a/voxygen/src/render/renderer/drawer.rs
+++ b/voxygen/src/render/renderer/drawer.rs
@@ -839,10 +839,6 @@ impl<'pass_ref, 'pass: 'pass_ref> PreparedUiDrawer<'pass_ref, 'pass> {
pub fn set_scissor(&mut self, scissor: Aabr<u16>) {
let Aabr { min, max } = scissor;
- // TODO: Got an invalid scissor panic from wgpu,
- // use this if you can reproduce
- // Note: might have been from changing monitors
- // dbg!(&scissor)
self.render_pass.set_scissor_rect(
min.x as u32,
min.y as u32,
diff --git a/voxygen/src/scene/terrain.rs b/voxygen/src/scene/terrain.rs
index 1aa6022f75..6c09250fad 100644
--- a/voxygen/src/scene/terrain.rs
+++ b/voxygen/src/scene/terrain.rs
@@ -1088,40 +1088,32 @@ impl<V: RectRasterableVol> Terrain<V> {
let atlas = &mut self.atlas;
let chunks = &mut self.chunks;
let col_lights = &mut self.col_lights;
- let allocation = atlas
- .allocate(guillotiere::Size::new(
- tex_size.x as i32, /* TODO: adjust ColLightInfo to avoid the
- * cast here? */
- tex_size.y as i32,
- ))
- .unwrap_or_else(|| {
- // Atlas allocation failure: try allocating a new texture and atlas.
- let (new_atlas, new_col_lights) = Self::make_atlas(renderer)
- .expect("Failed to create atlas texture");
+ let alloc_size =
+ guillotiere::Size::new(i32::from(tex_size.x), i32::from(tex_size.y));
- // We reset the atlas and clear allocations from existing chunks,
- // even though we haven't yet
- // checked whether the new allocation can fit in
- // the texture. This is reasonable because we don't have a fallback
- // if a single chunk can't fit in an empty atlas of maximum size.
- //
- // TODO: Consider attempting defragmentation first rather than just
- // always moving everything into the new chunk.
- chunks.iter_mut().for_each(|(_, chunk)| {
- chunk.col_lights_alloc = None;
- });
- *atlas = new_atlas;
- *col_lights = Arc::new(new_col_lights);
+ let allocation = atlas.allocate(alloc_size).unwrap_or_else(|| {
+ // Atlas allocation failure: try allocating a new texture and atlas.
+ let (new_atlas, new_col_lights) =
+ Self::make_atlas(renderer).expect("Failed to create atlas texture");
- atlas
- .allocate(guillotiere::Size::new(
- tex_size.x as i32, /* TODO: adjust ColLightInfo to avoid
- * the
- * cast here? */
- tex_size.y as i32,
- ))
- .expect("Chunk data does not fit in a texture of maximum size.")
+ // We reset the atlas and clear allocations from existing chunks,
+ // even though we haven't yet
+ // checked whether the new allocation can fit in
+ // the texture. This is reasonable because we don't have a fallback
+ // if a single chunk can't fit in an empty atlas of maximum size.
+ //
+ // TODO: Consider attempting defragmentation first rather than just
+ // always moving everything into the new chunk.
+ chunks.iter_mut().for_each(|(_, chunk)| {
+ chunk.col_lights_alloc = None;
});
+ *atlas = new_atlas;
+ *col_lights = Arc::new(new_col_lights);
+
+ atlas
+ .allocate(alloc_size)
+ .expect("Chunk data does not fit in a texture of maximum size.")
+ });
// NOTE: Cast is safe since the origin was a u16.
let atlas_offs = Vec2::new(
@@ -1131,7 +1123,7 @@ impl<V: RectRasterableVol> Terrain<V> {
renderer.update_texture(
&col_lights.texture,
atlas_offs.into_array(),
- tex_size.into_array(),
+ tex_size.map(|e| u32::from(e)).into_array(),
&tex,
);
@@ -1144,22 +1136,15 @@ impl<V: RectRasterableVol> Terrain<V> {
light_map: mesh.light_map,
glow_map: mesh.glow_map,
sprite_instances,
- locals: renderer.create_terrain_bound_locals(&[TerrainLocals {
- model_offs: Vec3::from(
+ locals: renderer.create_terrain_bound_locals(&[TerrainLocals::new(
+ Vec3::from(
response.pos.map2(VolGrid2d::<V>::chunk_size(), |e, sz| {
e as f32 * sz as f32
}),
- )
- .into_array(),
- atlas_offs: Vec4::new(
- atlas_offs.x as i32,
- atlas_offs.y as i32,
- 0,
- 0,
- )
- .into_array(),
+ ),
+ atlas_offs,
load_time,
- }]),
+ )]),
visible: Visibility {
in_range: false,
in_frustum: false,