From 9cdfb6a4aadb7ee6e6e7cdabfe887affcd1eeb4b Mon Sep 17 00:00:00 2001 From: Imbris Date: Mon, 14 Nov 2022 21:30:28 -0500 Subject: [PATCH] Resolve some remaining TODOs for ui image processing changes * Explicitly assert that neither of the requested dimensions for an image are 0. (I think this used to fail later on anyway) * Don't show the UI alpha premultiply pass in GPU timings in the HUD debug info display since it only very transiently appears (since this doesn't run every frame). --- voxygen/src/hud/mod.rs | 12 +++++++----- voxygen/src/render/mod.rs | 2 +- voxygen/src/render/renderer/drawer.rs | 9 +++++---- voxygen/src/ui/graphic/mod.rs | 21 ++++++++++++++------- 4 files changed, 27 insertions(+), 17 deletions(-) diff --git a/voxygen/src/hud/mod.rs b/voxygen/src/hud/mod.rs index 375a8f0a73..8bced4a057 100644 --- a/voxygen/src/hud/mod.rs +++ b/voxygen/src/hud/mod.rs @@ -2769,11 +2769,13 @@ impl Hud { } for (i, timing) in gpu_timings.iter().enumerate() { - let timings_text = &format!( - "{:16}{:.3} ms", - &format!("{}:", timing.1), - timing.2 * 1000.0, - ); + let label = timing.1; + // We skip displaying these since they aren't present every frame. + if label.starts_with(crate::render::UI_PREMULTIPLY_PASS) { + continue; + } + let timings_text = + &format!("{:16}{:.3} ms", &format!("{label}:"), timing.2 * 1000.0,); let timings_widget = Text::new(timings_text) .color(TEXT_COLOR) .down(V_PAD) diff --git a/voxygen/src/render/mod.rs b/voxygen/src/render/mod.rs index c607f633f0..d77cf8806a 100644 --- a/voxygen/src/render/mod.rs +++ b/voxygen/src/render/mod.rs @@ -53,7 +53,7 @@ pub use self::{ DebugDrawer, DebugShadowDrawer, Drawer, FigureDrawer, FigureShadowDrawer, FirstPassDrawer, ParticleDrawer, PreparedUiDrawer, ShadowPassDrawer, SpriteDrawer, TerrainDrawer, TerrainShadowDrawer, ThirdPassDrawer, TrailDrawer, - TransparentPassDrawer, UiDrawer, VolumetricPassDrawer, + TransparentPassDrawer, UiDrawer, VolumetricPassDrawer, UI_PREMULTIPLY_PASS, }, AltIndices, ColLightInfo, CullingMode, Renderer, }, diff --git a/voxygen/src/render/renderer/drawer.rs b/voxygen/src/render/renderer/drawer.rs index 2dc8a86a41..c350e99235 100644 --- a/voxygen/src/render/renderer/drawer.rs +++ b/voxygen/src/render/renderer/drawer.rs @@ -20,6 +20,9 @@ use wgpu_profiler::scope::{ManualOwningScope, OwningScope, Scope}; #[cfg(feature = "egui-ui")] use {common_base::span, egui_wgpu_backend::ScreenDescriptor, egui_winit_platform::Platform}; +/// Gpu timing label prefix associated with the UI alpha premultiplication pass. +pub const UI_PREMULTIPLY_PASS: &str = "ui_premultiply_pass"; + // Currently available pipelines enum Pipelines<'frame> { Interface(&'frame super::InterfacePipelines), @@ -447,10 +450,8 @@ impl<'frame> Drawer<'frame> { for (i, (target_texture, uploads)) in targets.into_iter().enumerate() { prof_span!("ui premultiply pass"); - let profile_name = format!("ui_premultiply_pass {}", i); - let label = format!("ui premultiply pass {}", i); - // S-TODO: a GPU profile scope on each of the passes here may be a bit too fine - // grained. + let profile_name = format!("{UI_PREMULTIPLY_PASS} {i}"); + let label = format!("ui premultiply pass {i}"); let mut render_pass = encoder.scoped_render_pass(&profile_name, device, &wgpu::RenderPassDescriptor { label: Some(&label), diff --git a/voxygen/src/ui/graphic/mod.rs b/voxygen/src/ui/graphic/mod.rs index 9bed1fad55..53fd4b48f7 100644 --- a/voxygen/src/ui/graphic/mod.rs +++ b/voxygen/src/ui/graphic/mod.rs @@ -214,8 +214,6 @@ impl TextureRequirements { } } - // TODO: what if requested size is 0? Do we currently panic on this case and - // expect caller not to ask for 0 size? (if so document that) fn to_key_and_tex_parameters( self, graphic_id: Id, @@ -387,6 +385,10 @@ impl GraphicCache { /// Source rectangle should be from 0 to 1, and represents a bounding box /// for the source image of the graphic. + /// + /// # Panics + /// + /// Panics if one of the lengths in requested_dims is zero. pub fn cache_res( &mut self, renderer: &mut Renderer, @@ -399,6 +401,7 @@ impl GraphicCache { source: Aabr, rotation: Rotation, ) -> Option<((Aabr, Vec2), TexId)> { + assert!(requested_dims.map(|e| e != 0).reduce_and()); let requested_dims_upright = match rotation { // The image is stored on the GPU with no rotation, so we need to swap the dimensions // here to get the resolution that the image will be displayed at but re-oriented into @@ -433,15 +436,13 @@ impl GraphicCache { }; // Apply all transformations. // TODO: Verify rotation is being applied correctly. - let transformed_aabr = |aabr| { + let transformed_aabr_and_scale = |aabr| { let scaled = scaled_aabr(aabr); // Calculate how many displayed pixels there are for each pixel in the source // image. We need this to calculate where to sample in the shader to // retain crisp pixel borders when scaling the image. - // S-TODO: A bit hacky inserting this here, just to get things working initially let scale = requested_dims_upright.map2( Vec2::from(scaled.size()), - // S-TODO div by zero potential? If so, is NaN an issue in that case? |screen_pixels, sample_pixels: f64| screen_pixels as f32 / sample_pixels as f32, ); let transformed = rotated_aabr(scaled); @@ -500,7 +501,10 @@ impl GraphicCache { details.set_valid(); } - return Some((transformed_aabr(aabr.map(|e| e as f64)), TexId(idx))); + return Some(( + transformed_aabr_and_scale(aabr.map(|e| e as f64)), + TexId(idx), + )); }, Entry::Vacant(details) => details, }; @@ -592,7 +596,10 @@ impl GraphicCache { // Insert into cached map details.insert(location); - Some((transformed_aabr(aabr.map(|e| e as f64)), TexId(idx))) + Some(( + transformed_aabr_and_scale(aabr.map(|e| e as f64)), + TexId(idx), + )) } }