From a83954571a47da986568a56e12f60d4634ca5c3d Mon Sep 17 00:00:00 2001 From: Kaz Wesley Date: Mon, 1 May 2023 07:29:59 -0700 Subject: [PATCH] Fix disappearing cached shapes (#6458) * Fix GL parameter * Don't leak GL Framebuffers * Resize: Don't rebuild screen-size-independent passes * Don't drop Texture while in use * Fix concurrency bug --- .../core/src/display/render/composer.rs | 20 ++++- .../ensogl/core/src/display/render/pass.rs | 11 ++- .../src/display/render/passes/cache_shapes.rs | 87 ++++++++++++------- .../core/src/system/gpu/data/texture/class.rs | 6 +- 4 files changed, 89 insertions(+), 35 deletions(-) diff --git a/lib/rust/ensogl/core/src/display/render/composer.rs b/lib/rust/ensogl/core/src/display/render/composer.rs index bdb95ff1f5f1..09134edeeffd 100644 --- a/lib/rust/ensogl/core/src/display/render/composer.rs +++ b/lib/rust/ensogl/core/src/display/render/composer.rs @@ -51,12 +51,28 @@ impl { self.init_passes(); } - /// Resize the composer and reinitialize all of its layers. + /// Resize the composer and reinitialize all of its screen-size-dependent layers. pub fn resize(&mut self, width: i32, height: i32, pixel_ratio: f32) { + if width == self.width && height == self.height && pixel_ratio == self.pixel_ratio { + // Some resize events are spurious; it is not necessary to reinitialize the passes if + // the size hasn't actually changed. + return; + } self.width = width; self.height = height; self.pixel_ratio = pixel_ratio; - self.init_passes(); + let ctx = &self.context; + let vars = &self.variables; + let defs = self.pipeline.passes_clone(); + let old_passes = self.passes.drain(..); + let passes = defs.into_iter().zip(old_passes).map(|(def, pass)| { + if def.is_screen_size_independent() { + pass + } else { + ComposerPass::new(ctx, vars, def, width, height, pixel_ratio) + } + }).collect_vec(); + self.passes = passes; } /// Initialize all pass definitions from the [`Pipeline`]. diff --git a/lib/rust/ensogl/core/src/display/render/pass.rs b/lib/rust/ensogl/core/src/display/render/pass.rs index 332a4cf8662a..62078713d785 100644 --- a/lib/rust/ensogl/core/src/display/render/pass.rs +++ b/lib/rust/ensogl/core/src/display/render/pass.rs @@ -20,6 +20,9 @@ use crate::system::gpu::data::texture::class::TextureOps; pub trait Definition: CloneBoxedForDefinition + Debug + 'static { fn initialize(&mut self, _instance: &Instance) {} fn run(&mut self, _instance: &Instance, update_status: UpdateStatus); + fn is_screen_size_independent(&self) -> bool { + false + } } clone_boxed!(Definition); @@ -170,7 +173,7 @@ impl OutputDefinition { /// Constructor of the RGBA u8 output with default texture parameters. It is the most popular /// option and you should use it to render colors with your passes. pub fn new_rgba(name: Name) -> Self { - let internal_format = texture::Rgba; + let internal_format = texture::Rgba8; let item_type = texture::item_type::u8; let texture_parameters = default(); OutputDefinition::new(name, internal_format, item_type, texture_parameters) @@ -217,3 +220,9 @@ impl Framebuffer { result } } + +impl Drop for Framebuffer { + fn drop(&mut self) { + self.context.delete_framebuffer(Some(&self.native)); + } +} diff --git a/lib/rust/ensogl/core/src/display/render/passes/cache_shapes.rs b/lib/rust/ensogl/core/src/display/render/passes/cache_shapes.rs index 537acbe2c98c..e854f68bbb5d 100644 --- a/lib/rust/ensogl/core/src/display/render/passes/cache_shapes.rs +++ b/lib/rust/ensogl/core/src/display/render/passes/cache_shapes.rs @@ -35,24 +35,31 @@ use crate::gui::component::AnyShapeView; #[derive(Clone, Derivative)] #[derivative(Debug)] pub struct CacheShapesPass { - scene: Scene, - framebuffer: Option, + scene: Scene, + framebuffer: Option, + texture: Option, #[derivative(Debug = "ignore")] - shapes_to_render: Vec>, + shapes_to_render: Vec>, /// Texture size in device pixels. texture_size_device: Vector2, - layer: Layer, + layer: Layer, + camera_ready: Rc>, + #[derivative(Debug = "ignore")] + display_object_update_handler: Option>, } impl CacheShapesPass { /// Constructor. pub fn new(scene: &Scene) -> Self { Self { - framebuffer: default(), - shapes_to_render: default(), - layer: Layer::new("Cached Shapes"), - scene: scene.clone_ref(), + framebuffer: default(), + texture: default(), + shapes_to_render: default(), + layer: Layer::new("Cached Shapes"), + scene: scene.clone_ref(), texture_size_device: default(), + camera_ready: default(), + display_object_update_handler: default(), } } } @@ -83,38 +90,60 @@ impl pass::Definition for CacheShapesPass { self.layer.camera().update(&self.scene); self.scene.display_object.update(&self.scene); self.layer.update(); + // The asynchronous update of the scene's display object initiated above will eventually + // set our layer's camera's transformation. Handle the camera update when all + // previously-initiated FRP events finish being processed. + let handle = frp::microtasks::next_microtask({ + let camera = self.layer.camera(); + let scene = self.scene.clone_ref(); + let camera_ready = Rc::clone(&self.camera_ready); + move || { + camera.update(&scene); + camera_ready.set(true); + } + }); + self.display_object_update_handler = Some(Rc::new(handle)); let output = pass::OutputDefinition::new_rgba("cached_shapes"); let texture = instance.new_texture(&output, self.texture_size_device.x, self.texture_size_device.y); self.framebuffer = Some(instance.new_framebuffer(&[&texture])); + self.texture = Some(texture); } fn run(&mut self, instance: &Instance, _update_status: UpdateStatus) { - let is_shader_compiled = - |shape: &mut Rc| shape.sprite().symbol.shader().program().is_some(); - let mut ready_to_render = self.shapes_to_render.drain_filter(is_shader_compiled).peekable(); - if ready_to_render.peek().is_some() { - if let Some(framebuffer) = self.framebuffer.as_ref() { - framebuffer.with_bound(|| { - instance.with_viewport( - self.texture_size_device.x, - self.texture_size_device.y, - || { - with_display_mode(DisplayModes::CachedShapesTexture, || { - with_context(|ctx| ctx.set_camera(&self.layer.camera())); - for shape in ready_to_render { - shape.sprite().symbol.render(); - } - }) - }, - ); - }); - } else { - reportable_error!("Impossible happened: The CacheShapesPass was run without initialized framebuffer."); + if self.camera_ready.get() { + let is_shader_compiled = |shape: &mut Rc| { + shape.sprite().symbol.shader().program().is_some() + }; + let mut ready_to_render = + self.shapes_to_render.drain_filter(is_shader_compiled).peekable(); + if ready_to_render.peek().is_some() { + if let Some(framebuffer) = self.framebuffer.as_ref() { + framebuffer.with_bound(|| { + instance.with_viewport( + self.texture_size_device.x, + self.texture_size_device.y, + || { + with_display_mode(DisplayModes::CachedShapesTexture, || { + with_context(|ctx| ctx.set_camera(&self.layer.camera())); + for shape in ready_to_render { + shape.sprite().symbol.render(); + } + }) + }, + ); + }); + } else { + reportable_error!("Impossible happened: The CacheShapesPass was run without initialized framebuffer."); + } } } } + + fn is_screen_size_independent(&self) -> bool { + true + } } fn with_display_mode(mode: DisplayModes, f: impl FnOnce() -> R) -> R { diff --git a/lib/rust/ensogl/core/src/system/gpu/data/texture/class.rs b/lib/rust/ensogl/core/src/system/gpu/data/texture/class.rs index 8c93b45fe987..2c76aaecf79d 100644 --- a/lib/rust/ensogl/core/src/system/gpu/data/texture/class.rs +++ b/lib/rust/ensogl/core/src/system/gpu/data/texture/class.rs @@ -58,9 +58,9 @@ impl Drop for TextureBindGuard { /// For more background see: https://developer.mozilla.org/en-US/docs/Web/API/WebGLRenderingContext/texParameter #[derive(Copy, Clone, Debug, Default)] pub struct Parameters { - /// Specifies the setting for the texture magnification filter (`Context::TEXTURE_MIN_FILTER`). + /// Specifies the setting for the texture minification filter (`Context::TEXTURE_MIN_FILTER`). pub min_filter: MinFilter, - /// Specifies the setting for the texture minification filter (`Context::TEXTURE_MAG_FILTER`). + /// Specifies the setting for the texture magnification filter (`Context::TEXTURE_MAG_FILTER`). pub mag_filter: MagFilter, /// Specifies the setting for the wrapping function for texture coordinate s /// (`Context::TEXTURE_WRAP_S`). @@ -75,7 +75,7 @@ impl Parameters { pub fn apply_parameters(self, context: &Context) { let target = Context::TEXTURE_2D; context.tex_parameteri(*target, *Context::TEXTURE_MIN_FILTER, *self.min_filter as i32); - context.tex_parameteri(*target, *Context::TEXTURE_MIN_FILTER, *self.mag_filter as i32); + context.tex_parameteri(*target, *Context::TEXTURE_MAG_FILTER, *self.mag_filter as i32); context.tex_parameteri(*target, *Context::TEXTURE_WRAP_S, *self.wrap_s as i32); context.tex_parameteri(*target, *Context::TEXTURE_WRAP_T, *self.wrap_t as i32); }