From 5223dae3988b10bd389189a8324d0fd44eed5c2e Mon Sep 17 00:00:00 2001 From: jprochazk <1665677+jprochazk@users.noreply.github.com> Date: Mon, 27 May 2024 20:30:20 +0200 Subject: [PATCH 1/6] don't `forget` raf closure --- crates/eframe/src/web/web_runner.rs | 49 ++++++++++++++++++++++------- 1 file changed, 37 insertions(+), 12 deletions(-) diff --git a/crates/eframe/src/web/web_runner.rs b/crates/eframe/src/web/web_runner.rs index f39fc0dcace6..fd8a7cf236e0 100644 --- a/crates/eframe/src/web/web_runner.rs +++ b/crates/eframe/src/web/web_runner.rs @@ -1,7 +1,4 @@ -use std::{ - cell::{Cell, RefCell}, - rc::Rc, -}; +use std::{cell::RefCell, rc::Rc}; use wasm_bindgen::prelude::*; @@ -28,8 +25,8 @@ pub struct WebRunner { /// the panic handler, since they aren't `Send`. events_to_unsubscribe: Rc>>, - /// Used in `destroy` to cancel a pending frame. - request_animation_frame_id: Cell>, + /// Current animation frame in flight. + frame: Rc>>, } impl WebRunner { @@ -47,7 +44,7 @@ impl WebRunner { panic_handler, runner: Rc::new(RefCell::new(None)), events_to_unsubscribe: Rc::new(RefCell::new(Default::default())), - request_animation_frame_id: Cell::new(None), + frame: Default::default(), } } @@ -115,9 +112,10 @@ impl WebRunner { pub fn destroy(&self) { self.unsubscribe_from_all_events(); - if let Some(id) = self.request_animation_frame_id.get() { + if let Some(frame) = self.frame.take() { let window = web_sys::window().unwrap(); - window.cancel_animation_frame(id).ok(); + window.cancel_animation_frame(frame.id).ok(); + drop(frame.closure); } if let Some(runner) = self.runner.replace(None) { @@ -193,20 +191,47 @@ impl WebRunner { } pub(crate) fn request_animation_frame(&self) -> Result<(), wasm_bindgen::JsValue> { + if self.frame.borrow().is_some() { + // there is already an animation frame in flight + return Ok(()); + } + let window = web_sys::window().unwrap(); let closure = Closure::once({ let runner_ref = self.clone(); - move || events::paint_and_schedule(&runner_ref) + move || { + // we can paint now, so clear the animation frame + // this drop the `closure` and allows another + // animation frame to be scheduled + let _ = runner_ref.frame.take(); + events::paint_and_schedule(&runner_ref) + } }); + let id = window.request_animation_frame(closure.as_ref().unchecked_ref())?; - self.request_animation_frame_id.set(Some(id)); - closure.forget(); // We must forget it, or else the callback is canceled on drop + self.frame + .borrow_mut() + .replace(AnimationFrameRequest { id, closure }); + Ok(()) } } // ---------------------------------------------------------------------------- +// https://rustwasm.github.io/wasm-bindgen/api/wasm_bindgen/closure/struct.Closure.html#using-fnonce-and-closureonce-with-requestanimationframe +struct AnimationFrameRequest { + /// Represents the ID of a frame in flight. + /// + /// This is only set between a call to `request_animation_frame` and the invocation of its callback, + /// which means that repeated calls to `request_animation_frame` will be ignored. + id: i32, + + /// The callback given to `request_animation_frame`, stored here both to prevent it + /// from being canceled, and from having to `.forget()` it. + closure: Closure Result<(), JsValue>>, +} + struct TargetEvent { target: web_sys::EventTarget, event_name: String, From a9084258027164158116e3c09d37b9f297f6396d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Proch=C3=A1zka?= Date: Mon, 27 May 2024 21:35:06 +0200 Subject: [PATCH 2/6] Update crates/eframe/src/web/web_runner.rs Co-authored-by: Emil Ernerfeldt --- crates/eframe/src/web/web_runner.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/eframe/src/web/web_runner.rs b/crates/eframe/src/web/web_runner.rs index fd8a7cf236e0..44e804f6c272 100644 --- a/crates/eframe/src/web/web_runner.rs +++ b/crates/eframe/src/web/web_runner.rs @@ -200,8 +200,8 @@ impl WebRunner { let closure = Closure::once({ let runner_ref = self.clone(); move || { - // we can paint now, so clear the animation frame - // this drop the `closure` and allows another + // We can paint now, so clear the animation frame. + // This drops the `closure` and allows another // animation frame to be scheduled let _ = runner_ref.frame.take(); events::paint_and_schedule(&runner_ref) From ea3eaf5e6e5cdd2a50a49687bf536b2e405193ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Proch=C3=A1zka?= Date: Mon, 27 May 2024 21:35:13 +0200 Subject: [PATCH 3/6] Update crates/eframe/src/web/web_runner.rs Co-authored-by: Emil Ernerfeldt --- crates/eframe/src/web/web_runner.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/eframe/src/web/web_runner.rs b/crates/eframe/src/web/web_runner.rs index 44e804f6c272..826b6ceab17f 100644 --- a/crates/eframe/src/web/web_runner.rs +++ b/crates/eframe/src/web/web_runner.rs @@ -203,7 +203,7 @@ impl WebRunner { // We can paint now, so clear the animation frame. // This drops the `closure` and allows another // animation frame to be scheduled - let _ = runner_ref.frame.take(); + runner_ref.frame = None; events::paint_and_schedule(&runner_ref) } }); From 143c832266db762809b730a4cd30a9168f56b65f Mon Sep 17 00:00:00 2001 From: jprochazk <1665677+jprochazk@users.noreply.github.com> Date: Mon, 27 May 2024 21:36:49 +0200 Subject: [PATCH 4/6] remove explicit drop --- crates/eframe/src/web/web_runner.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/eframe/src/web/web_runner.rs b/crates/eframe/src/web/web_runner.rs index 826b6ceab17f..9c928505e7d5 100644 --- a/crates/eframe/src/web/web_runner.rs +++ b/crates/eframe/src/web/web_runner.rs @@ -115,7 +115,6 @@ impl WebRunner { if let Some(frame) = self.frame.take() { let window = web_sys::window().unwrap(); window.cancel_animation_frame(frame.id).ok(); - drop(frame.closure); } if let Some(runner) = self.runner.replace(None) { @@ -209,9 +208,10 @@ impl WebRunner { }); let id = window.request_animation_frame(closure.as_ref().unchecked_ref())?; - self.frame - .borrow_mut() - .replace(AnimationFrameRequest { id, closure }); + self.frame.borrow_mut().replace(AnimationFrameRequest { + id, + _closure: closure, + }); Ok(()) } @@ -229,7 +229,7 @@ struct AnimationFrameRequest { /// The callback given to `request_animation_frame`, stored here both to prevent it /// from being canceled, and from having to `.forget()` it. - closure: Closure Result<(), JsValue>>, + _closure: Closure Result<(), JsValue>>, } struct TargetEvent { From f5c2dd855b35bd402fab850241c6a87697b90615 Mon Sep 17 00:00:00 2001 From: jprochazk <1665677+jprochazk@users.noreply.github.com> Date: Mon, 27 May 2024 21:40:56 +0200 Subject: [PATCH 5/6] use `.take()` --- crates/eframe/src/web/web_runner.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/eframe/src/web/web_runner.rs b/crates/eframe/src/web/web_runner.rs index 9c928505e7d5..383572ce3f7e 100644 --- a/crates/eframe/src/web/web_runner.rs +++ b/crates/eframe/src/web/web_runner.rs @@ -202,7 +202,7 @@ impl WebRunner { // We can paint now, so clear the animation frame. // This drops the `closure` and allows another // animation frame to be scheduled - runner_ref.frame = None; + let _ = runner_ref.frame.take(); events::paint_and_schedule(&runner_ref) } }); From fcd7cdc39cca08722826d0f54822d9e1627ca056 Mon Sep 17 00:00:00 2001 From: jprochazk <1665677+jprochazk@users.noreply.github.com> Date: Mon, 27 May 2024 21:47:12 +0200 Subject: [PATCH 6/6] docs --- crates/eframe/src/web/web_runner.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/crates/eframe/src/web/web_runner.rs b/crates/eframe/src/web/web_runner.rs index 8bfc899479e5..6a230a8e3292 100644 --- a/crates/eframe/src/web/web_runner.rs +++ b/crates/eframe/src/web/web_runner.rs @@ -199,6 +199,10 @@ impl WebRunner { Ok(()) } + /// Request an animation frame from the browser in which we can perform a paint. + /// + /// It is safe to call `request_animation_frame` multiple times in quick succession, + /// this function guarantees that only one animation frame is scheduled at a time. pub(crate) fn request_animation_frame(&self) -> Result<(), wasm_bindgen::JsValue> { if self.frame.borrow().is_some() { // there is already an animation frame in flight @@ -245,9 +249,6 @@ impl WebRunner { // https://rustwasm.github.io/wasm-bindgen/api/wasm_bindgen/closure/struct.Closure.html#using-fnonce-and-closureonce-with-requestanimationframe struct AnimationFrameRequest { /// Represents the ID of a frame in flight. - /// - /// This is only set between a call to `request_animation_frame` and the invocation of its callback, - /// which means that repeated calls to `request_animation_frame` will be ignored. id: i32, /// The callback given to `request_animation_frame`, stored here both to prevent it