From 95f17a6d22392603d05c637412f04cfd4095a8f0 Mon Sep 17 00:00:00 2001 From: Imbris Date: Fri, 1 Jul 2022 21:22:41 -0400 Subject: [PATCH] General cleanup, remove local dependency, switch to u16 from i32 in several places, avoid guillotiere types in atlas API (one spot still remains) --- Cargo.lock | 2 + voxygen/Cargo.toml | 3 +- voxygen/src/mesh/greedy.rs | 116 +++++++++++++------------ voxygen/src/mesh/segment.rs | 6 +- voxygen/src/mesh/terrain.rs | 3 +- voxygen/src/render/pipelines/figure.rs | 2 +- voxygen/src/scene/particle.rs | 2 +- voxygen/src/scene/terrain.rs | 2 +- 8 files changed, 71 insertions(+), 65 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c16286a6a6..f841529b1f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2421,6 +2421,8 @@ dependencies = [ [[package]] name = "guillotiere" version = "0.6.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b62d5865c036cb1393e23c50693df631d3f5d7bcca4c04fe4cc0fd592e74a782" dependencies = [ "euclid", "svg_fmt", diff --git a/voxygen/Cargo.toml b/voxygen/Cargo.toml index 82d123c7a5..070cf9c1d6 100644 --- a/voxygen/Cargo.toml +++ b/voxygen/Cargo.toml @@ -103,8 +103,7 @@ crossbeam-channel = "0.5" directories-next = "2.0" dot_vox = "4.0" enum-iterator = "0.7" -# guillotiere = "0.6.2" -guillotiere = { path = "../../guillotiere"} +guillotiere = "0.6.2" etagere = "0.2.7" hashbrown = {version = "0.11", features = ["rayon", "serde", "nightly"]} image = {version = "0.24", default-features = false, features = ["ico", "png"]} diff --git a/voxygen/src/mesh/greedy.rs b/voxygen/src/mesh/greedy.rs index 3dc8ee3f65..4daa25196f 100644 --- a/voxygen/src/mesh/greedy.rs +++ b/voxygen/src/mesh/greedy.rs @@ -87,29 +87,31 @@ pub trait AtlasAllocator { /// Creates a new instance of this atlas allocator taking into account the /// provided max size; - fn with_max_size(max_size: Vec2) -> Self; + fn with_max_size(max_size: Vec2) -> Self; /// Allocates a rectangle of the given size. // TODO: don't use guillotiere type here - fn allocate(&mut self, size: Vec2) -> Option; + fn allocate(&mut self, size: Vec2) -> Option; /// Retrieves the current size of the atlas being allocated from. - fn size(&self) -> Vec2; + fn size(&self) -> Vec2; /// Grows the size of the atlas to the provided size. - fn grow(&mut self, new_size: Vec2); + fn grow(&mut self, new_size: Vec2); } -fn guillotiere_size(size: Vec2) -> guillotiere::Size { guillotiere::Size::new(size.x, size.y) } +fn guillotiere_size>(size: Vec2) -> guillotiere::Size { + guillotiere::Size::new(size.x.into(), size.y.into()) +} impl AtlasAllocator for guillotiere::SimpleAtlasAllocator { - fn with_max_size(max_size: Vec2) -> Self { + fn with_max_size(max_size: Vec2) -> Self { // TODO: Collect information to see if we can choose a good value here. These // current values were optimized for sprites, but we are using a // different allocator for them so different values might be better // here. let large_size_threshold = 8; //256.min(min_max_dim / 2 + 1); - let small_size_threshold = !(3); //33.min(large_size_threshold / 2 + 1); + let small_size_threshold = 3; //33.min(large_size_threshold / 2 + 1); // (12, 3) 24.5 // (12, 2) 33.2 // (12, 4) 27.2 @@ -128,19 +130,22 @@ impl AtlasAllocator for guillotiere::SimpleAtlasAllocator { } /// Allocates a rectangle of the given size. - fn allocate(&mut self, size: Vec2) -> Option { + fn allocate(&mut self, size: Vec2) -> Option { self.allocate(guillotiere_size(size)) } /// Retrieves the current size of the atlas being allocated from. - fn size(&self) -> Vec2 { self.size().to_array().into() } + fn size(&self) -> Vec2 { + // NOTE: with_max_size / grow take a u16 so the size will never be larger than + // u16::MAX + Vec2::::from(self.size().to_array()).map(|e| e as u16) + } /// Grows the size of the atlas to the provided size. - fn grow(&mut self, new_size: Vec2) { self.grow(guillotiere_size(new_size)) } + fn grow(&mut self, new_size: Vec2) { self.grow(guillotiere_size(new_size)) } } pub struct GuillotiereTiled { - // TODO: Try BucketsAtlasAllocator // Each tile is Self::TILE_SIZE (unless max size is not aligned to this, in which case the // tiles that reach the max size are truncated below this value). allocator: guillotiere::SimpleAtlasAllocator, @@ -152,13 +157,18 @@ pub struct GuillotiereTiled { // Offset (in tiles) of current tile being allocated from (others returned `None` on last // allocation attempt) current: Option>, - // Efficiency history (total area, used area) - history: Vec<(i32, i32)>, + // Efficiency history for filled tiles (total area, used area) + // + // This is useful to examine packing efficiency. + history: Vec<(u32, u32)>, + used_in_current_tile: u32, } impl GuillotiereTiled { // We can potentially further optimize packing by deferring the allocations - // until all rectangles are available for packing. + // until all rectangles are available for packing. We could also cache this + // for sprites if we get to the point of having the rest of start up times + // fast enough for this to be helpful (e.g. for iterative work). // // Tested with sprites: // 64 1.63s 1.109 packing @@ -171,7 +181,8 @@ impl GuillotiereTiled { fn allocator_options() -> guillotiere::AllocatorOptions { // TODO: Collect information to see if we can choose a better value here (these - // values were picked before switching to this tiled implementation). + // values were picked before switching to this tiled implementation). I + // suspect these are still near optimal though. let large_size_threshold = 8; let small_size_threshold = 3; @@ -183,21 +194,21 @@ impl GuillotiereTiled { } fn next_tile(&mut self) { - tracing::error!("next tile"); if self.current.is_some() { prof_span!("stats"); - let free_space = self.allocator.free_space(); let size = self.allocator.size(); - let area = size.width * size.height; - let used = area - free_space; + // NOTE: TILE_SIZE is small enough that this won't overflow. + let area = size.width as u32 * size.height as u32; + let used = self.used_in_current_tile; self.history.push((area, used)); } self.current = if let Some(offset) = self.free_tiles.pop() { self.allocator.reset( - guillotiere_size(Vec2::broadcast(i32::from(Self::TILE_SIZE))), + guillotiere_size(Vec2::broadcast(Self::TILE_SIZE)), &Self::allocator_options(), ); + self.used_in_current_tile = 0; Some(offset) } else { None @@ -206,9 +217,9 @@ impl GuillotiereTiled { } impl AtlasAllocator for GuillotiereTiled { - fn with_max_size(max_size: Vec2) -> Self { - let size = guillotiere_size(Vec2::broadcast(i32::from(Self::TILE_SIZE))) - .min(guillotiere_size(max_size)); + fn with_max_size(max_size: Vec2) -> Self { + let size = + guillotiere_size(Vec2::broadcast(Self::TILE_SIZE)).min(guillotiere_size(max_size)); let allocator = guillotiere::SimpleAtlasAllocator::with_options(size, &Self::allocator_options()); @@ -219,27 +230,27 @@ impl AtlasAllocator for GuillotiereTiled { size: Vec2::new(1, 1), current: Some(Vec2::new(0, 0)), history: Vec::new(), + used_in_current_tile: 0, } } /// Allocates a rectangle of the given size. - fn allocate(&mut self, size: Vec2) -> Option { - //prof_span!("allocate_tiled"); - //tracing::info!("size {}", size); + fn allocate(&mut self, size: Vec2) -> Option { let size = guillotiere_size(size); while let Some(current) = self.current { match self.allocator.allocate(size) { Some(r) => { - // NOTE: The offset will always be smaller or equal to the `i32`s passed into + // NOTE: The offset will always be smaller or equal to the `u16`s passed into // `with_max_size`/`grow` so this won't overflow. - let offset = - guillotiere_size(current.map(|e| e as i32 * i32::from(Self::TILE_SIZE))); + let offset = guillotiere_size(current.map(|e| e as u16 * Self::TILE_SIZE)); let offset_rect = guillotiere::Rectangle { min: r.min.add_size(&offset), max: r.max.add_size(&offset), }; + // NOTE: `i32` -> `u32` conversion is fine since these will always be positive. + self.used_in_current_tile += size.width as u32 * size.height as u32; return Some(offset_rect); }, @@ -251,14 +262,14 @@ impl AtlasAllocator for GuillotiereTiled { } /// Retrieves the current size of the atlas being allocated from. - fn size(&self) -> Vec2 { - // NOTE: The size will always be smaller or equal to the `i32`s passed into + fn size(&self) -> Vec2 { + // NOTE: The size will always be smaller or equal to the `u16`s passed into // `with_max_size`/`grow` so this won't overflow. - self.size.map(|e| e as i32 * i32::from(Self::TILE_SIZE)) + self.size.map(|e| e as u16 * Self::TILE_SIZE) } /// Grows the size of the atlas to the provided size. - fn grow(&mut self, new_size: Vec2) { + fn grow(&mut self, new_size: Vec2) { if tracing::enabled!(tracing::Level::TRACE) { tracing::trace!( "Tile count: {}", @@ -274,12 +285,10 @@ impl AtlasAllocator for GuillotiereTiled { } let diff = (new_size - self.size()).map(|e| e.max(0)); - // NOTE: `e` should fit in usize since it is a postive i32 (and we aren't - // targeting 16-bit platforms, although this will probably hit the max - // texture size limit first regardless) - // NOTE: growing only occurs in increments of tiles size so any remaining size - // is ignored. - let diff_tiles = diff.map(|e| e as usize / usize::from(Self::TILE_SIZE)); + // NOTE: Growing only occurs in increments of TILE_SIZE so any remaining size is + // ignored. Max size is not known here so this must truncate instead of rounding + // up. + let diff_tiles = diff.map(|e| usize::from(e) / usize::from(Self::TILE_SIZE)); let old_size = self.size; self.size += diff_tiles; @@ -309,7 +318,7 @@ pub struct GreedyMesh<'a, Allocator: AtlasAllocator = guillotiere::SimpleAtlasAl //atlas: guillotiere::SimpleAtlasAllocator, atlas: Allocator, col_lights_size: Vec2, - max_size: Vec2, + max_size: Vec2, suspended: Vec>>, } @@ -319,7 +328,8 @@ impl<'a, Allocator: AtlasAllocator> GreedyMesh<'a, Allocator> { /// Takes as input the maximum allowable size of the texture atlas used to /// store the light/color data for this mesh. /// - /// NOTE: It is an error to pass any size > u16::MAX. + /// NOTE: It is an error to pass any size > u16::MAX (this is now enforced + /// by the type being `u16`). /// /// Even aside from the above limitation, this will not necessarily always /// be the same as the maximum atlas size supported by the hardware. @@ -328,10 +338,7 @@ impl<'a, Allocator: AtlasAllocator> GreedyMesh<'a, Allocator> { /// to have at least 2 bits of the normal; thus, it can only take up at /// most 30 bits total, meaning we are restricted to "only" at most 2^15 /// × 2^15 atlases even if the hardware supports larger ones. - // TODO: could we change i32 -> u16 here? - // TODO: use this: pub fn new(max_size: Vec2) -> Self { - pub fn new(max_size: guillotiere::Size) -> Self { - let max_size = Vec2::new(max_size.width, max_size.height); + pub fn new(max_size: Vec2) -> Self { span!(_guard, "new", "GreedyMesh::new"); let min_max_dim = max_size.reduce_min(); assert!( @@ -405,14 +412,13 @@ impl<'a, Allocator: AtlasAllocator> GreedyMesh<'a, Allocator> { col_lights_info } - // TODO: don't use guillotiere type here - pub fn max_size(&self) -> guillotiere::Size { guillotiere_size(self.max_size) } + pub fn max_size(&self) -> Vec2 { self.max_size } } fn greedy_mesh<'a, M: PartialEq, D: 'a, FL, FG, FO, FS, FP, FT, Allocator: AtlasAllocator>( atlas: &mut Allocator, col_lights_size: &mut Vec2, - max_size: Vec2, + max_size: Vec2, GreedyConfig { mut data, draw_delta, @@ -648,16 +654,16 @@ fn add_to_atlas( dim: Vec2, norm: Vec3, faces_forward: bool, - max_size: Vec2, + max_size: Vec2, cur_size: &mut Vec2, ) -> guillotiere::Rectangle { //prof_span!("add_to_atlas"); // TODO: Check this conversion. let atlas_rect = loop { - // NOTE: Conversion to i32 is safe because he x, y, and z dimensions for any + // NOTE: Conversion to u16 is safe because he x, y, and z dimensions for any // chunk index must fit in at least an i16 (lower for x and y, probably - // lower for z). - let res = atlas.allocate(Vec2::new(dim.x as i32 + 1, dim.y as i32 + 1)); + // lower for z) and at least x and y are not negative. + let res = atlas.allocate(Vec2::new(dim.x as u16 + 1, dim.y as u16 + 1)); if let Some(atlas_rect) = res { break atlas_rect; } @@ -688,9 +694,9 @@ fn add_to_atlas( }); atlas.grow(new_size); }; - // NOTE: Conversion is correct because our initial max size for the atlas was - // a u16 and we never grew the atlas, meaning all valid coordinates within the - // atlas also fit into a u16. + // NOTE: Conversion is correct because our initial max size for the atlas was a + // u16 and we never grew the atlas past the max size, 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 u16), cur_size.y.max(atlas_rect.max.y as u16), diff --git a/voxygen/src/mesh/segment.rs b/voxygen/src/mesh/segment.rs index 2a0625704f..068d38ad8f 100644 --- a/voxygen/src/mesh/segment.rs +++ b/voxygen/src/mesh/segment.rs @@ -35,7 +35,7 @@ where // in order to store the bone index. The two bits are instead taken out // of the atlas coordinates, which is why we "only" allow 1 << 15 per // coordinate instead of 1 << 16. - assert!(max_size.width.max(max_size.height) < 1 << 15); + assert!(max_size.reduce_max() < 1 << 15); let lower_bound = vol.lower_bound(); let upper_bound = vol.upper_bound(); @@ -130,7 +130,7 @@ where // in order to store the bone index. The two bits are instead taken out // of the atlas coordinates, which is why we "only" allow 1 << 15 per // coordinate instead of 1 << 16. - assert!(max_size.width.max(max_size.height) < 1 << 16); + assert!(u32::from(max_size.reduce_max()) < 1 << 16); let lower_bound = vol.lower_bound(); let upper_bound = vol.upper_bound(); @@ -253,7 +253,7 @@ where // in order to store the bone index. The two bits are instead taken out // of the atlas coordinates, which is why we "only" allow 1 << 15 per // coordinate instead of 1 << 16. - assert!(max_size.width.max(max_size.height) < 1 << 16); + assert!(u32::from(max_size.reduce_max()) < 1 << 16); let lower_bound = vol.lower_bound(); let upper_bound = vol.upper_bound(); diff --git a/voxygen/src/mesh/terrain.rs b/voxygen/src/mesh/terrain.rs index c76f5fc245..4adcb71295 100644 --- a/voxygen/src/mesh/terrain.rs +++ b/voxygen/src/mesh/terrain.rs @@ -351,8 +351,7 @@ pub fn generate_mesh<'a, V: RectRasterableVol + ReadVol + Debug + ' (start, end) }); - let max_size = - guillotiere::Size::new(i32::from(max_texture_size.x), i32::from(max_texture_size.y)); + let max_size = max_texture_size; assert!(z_end >= z_start); let greedy_size = Vec3::new(range.size().w - 2, range.size().h - 2, z_end - z_start + 1); // NOTE: Terrain sizes are limited to 32 x 32 x 16384 (to fit in 24 bits: 5 + 5 diff --git a/voxygen/src/render/pipelines/figure.rs b/voxygen/src/render/pipelines/figure.rs index 0ab2799b75..33e81c0de5 100644 --- a/voxygen/src/render/pipelines/figure.rs +++ b/voxygen/src/render/pipelines/figure.rs @@ -92,7 +92,7 @@ impl FigureModel { // in order to store the bone index. The two bits are instead taken out // of the atlas coordinates, which is why we "only" allow 1 << 15 per // coordinate instead of 1 << 16. - let max_size = guillotiere::Size::new((1 << 15) - 1, (1 << 15) - 1); + let max_size = Vec2::new((1 << 15) - 1, (1 << 15) - 1); GreedyMesh::new(max_size) } } diff --git a/voxygen/src/scene/particle.rs b/voxygen/src/scene/particle.rs index 75e06a4e05..ffc4de6ab5 100644 --- a/voxygen/src/scene/particle.rs +++ b/voxygen/src/scene/particle.rs @@ -1599,7 +1599,7 @@ fn default_cache(renderer: &mut Renderer) -> HashMap<&'static str, Model::load_expect("voxygen.voxel.sprite_manifest").cloned(); - let max_size = guillotiere::Size::new(max_texture_size as i32, max_texture_size as i32); + let max_size = Vec2::from(u16::try_from(max_texture_size).unwrap_or(u16::MAX)); let mut greedy = GreedyMesh::::new(max_size); let mut sprite_mesh = Mesh::new(); // NOTE: Tracks the start vertex of the next model to be meshed.