Fixed render loop

[kulicuu]

Description

Before this change, the entire render_gl function was called every frame, calling redundant operations, hence inefficient. After the changes, the render_gl function is called only once, and the f/g closure contains the logic to be performed every frame -- this is passed to request_animation_frame

Also removed the storage of the render_loop on self. It's not motivated for the demo. However, I will look to add that back in when I see a way to demonstrate it, might require some DOM interface for the user to modify.

Fixes #2758

Checklist

• [x ] I have reviewed my own code
• [x ] I have added tests

There are two log statements added -- one which shouldn't repeat every frame, and another that should.

[kulicuu]

Thanks!

• I tried using gloo-render but there was a type error. I'll try again on that.
• There is more cleanup I can do as you mention above with the legacy pattern involving that timestamp. Basically, the previous version called the Yew update every frame with that timestamp. Do need to eliminate that. The other thing I'm looking into was the storage from rendered function of a render_loop on self. I'll look for a use-case, and see if I can integrate that functionality into the fix. It may just be something vestigial to let drop out.
• The other issues -- gloo logging for example -- I'll work on and resubmit.

[github-actions[bot]]

Size Comparison

examples	master (KB)	pull request (KB)	diff (KB)	diff (%)
boids	169.331	169.342	+0.011	+0.006%
contexts	107.380	107.385	+0.005	+0.005%
counter	84.909	84.911	+0.002	+0.002%
counter_functional	85.536	85.536	0	0.000%
dyn_create_destroy_apps	87.838	87.840	+0.002	+0.002%
file_upload	99.571	99.572	+0.001	+0.001%
function_memory_game	162.698	162.706	+0.008	+0.005%
function_router	345.678	345.696	+0.019	+0.005%
function_todomvc	157.696	157.708	+0.012	+0.007%
futures	221.991	221.989	-0.002	-0.001%
game_of_life	105.042	105.044	+0.002	+0.002%
immutable	180.741	180.731	-0.010	-0.005%
inner_html	82.009	82.011	+0.002	+0.002%
js_callback	110.725	110.722	-0.003	-0.003%
keyed_list	193.481	193.481	0	0.000%
mount_point	84.695	84.693	-0.002	-0.002%
nested_list	112.006	112.003	-0.003	-0.003%
node_refs	91.852	91.852	0	0.000%
password_strength	1546.347	1546.343	-0.004	-0.000%
portals	95.432	95.438	+0.006	+0.006%
router	315.124	315.112	-0.012	-0.004%
simple_ssr	151.386	151.384	-0.002	-0.001%
ssr_router	391.255	391.181	-0.074	-0.019%
suspense	108.452	108.451	-0.001	-0.001%
timer	87.678	87.678	0	0.000%
todomvc	139.212	139.214	+0.002	+0.001%
two_apps	85.553	85.557	+0.004	+0.005%
web_worker_fib	150.535	150.537	+0.002	+0.001%
webgl	85.689	84.451	-1.238	-1.445%

⚠️ The following example has changed its size significantly:

examples	master (KB)	pull request (KB)	diff (KB)	diff (%)
webgl	85.689	84.451	-1.238	-1.445%

[kulicuu]

Gloo-render's request_animation_frame source:

It looks like they wrap the provided closure in a similar way to what I've done. Not exactly sure. I imagine it will be possible to provide it something that would work.

[kulicuu]

I'm thinking that even if I can supply the gloo-render raf function with an appropriate closure, that works, it will be less efficient. Every frame it's going to be running those logics, wrapping the supplied closure in the Rc, cloning, etc. With my version using web-sys, the closure is prepared just once. Fewer op cycles. In that sense, it may be better to stick to web-sys for this.
Thoughts?

[ranile]

Can you try the following patch? I got it to compile but haven't tested if it is functional.

Index: examples/webgl/src/main.rs
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/examples/webgl/src/main.rs b/examples/webgl/src/main.rs
--- a/examples/webgl/src/main.rs	(revision ba80b8d5abe3b07f0d02b68bc3437a9bd5a93664)
+++ b/examples/webgl/src/main.rs	(date 1656526514345)
@@ -1,10 +1,10 @@
-use web_sys::{HtmlCanvasElement, WebGlRenderingContext as GL, window};
+use web_sys::{HtmlCanvasElement, WebGlProgram, WebGlRenderingContext as GL, window};
 use yew::html::Scope;
 use yew::{html, Component, Context, Html, NodeRef};
 use wasm_bindgen::prelude::*;
 use wasm_bindgen::JsCast;
 
-use std::cell::RefCell;
+use std::cell::{Cell, RefCell};
 use std::rc::Rc;
 
 use gloo_console::log;
@@ -59,10 +59,10 @@
 
     fn render_gl(&mut self, link: &Scope<Self>) {
         log!("This shouldn't repeat every frame.");
-        
+
         let gl = self.gl.as_ref().expect("GL Context not initialized!");
 
-        let mut timestamp = 0.0;
+        let mut timestamp: Cell<f32> = Cell::new(0.0);
 
         let vert_code = include_str!("./basic.vert");
         let frag_code = include_str!("./basic.frag");
@@ -99,7 +99,7 @@
 
         // Attach the time as a uniform for the GL context.
         let time = gl.get_uniform_location(&shader_program, "u_time");
-        gl.uniform1f(time.as_ref(), timestamp as f32);
+        gl.uniform1f(time.as_ref(), timestamp.get());
 
         gl.draw_arrays(GL::TRIANGLES, 0, 6);
         let gl = gl.clone();
@@ -107,14 +107,16 @@
         let f = Rc::new(RefCell::new(None));
         let g = f.clone();
 
-        *g.borrow_mut() = Some(Closure::wrap(Box::new(move || {
+        fn handle_animation_frame(gl: Rc<GL>, timestamp: Cell<f32>, shader_program: WebGlProgram) {
             log!("This should repeat every frame.");
-            timestamp+= 20.0;
+            timestamp.set(timestamp.get() + 20.0);
             let time = gl.get_uniform_location(&shader_program, "u_time");
-            gl.uniform1f(time.as_ref(), timestamp as f32);
+            gl.uniform1f(time.as_ref(), timestamp.get());
             gl.draw_arrays(GL::TRIANGLES, 0, 6);
-            App::request_animation_frame(f.borrow().as_ref().unwrap());
-        }) as Box<dyn FnMut()>));
+            gloo_render::request_animation_frame(move |_| handle_animation_frame(gl, timestamp, shader_program));
+        }
+
+        gloo_render::request_animation_frame(move |_| handle_animation_frame(gl, timestamp, shader_program));
 
         App::request_animation_frame(g.borrow().as_ref().unwrap());
     }
Index: examples/webgl/Cargo.toml
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/examples/webgl/Cargo.toml b/examples/webgl/Cargo.toml
--- a/examples/webgl/Cargo.toml	(revision ba80b8d5abe3b07f0d02b68bc3437a9bd5a93664)
+++ b/examples/webgl/Cargo.toml	(date 1656526190118)
@@ -10,6 +10,7 @@
 wasm-bindgen = "0.2"
 yew = { path = "../../packages/yew", features = ["csr"] }
 gloo-console = "0.2.1"
+gloo-render = "0.1"
 
 [dependencies.web-sys]
 version = "0.3"

[kulicuu]

I'll need to squash commits. Now just looking for feedback on the request-animation-frame call. My take at the moment is it's more optimal to use the web-sys given what gloo-render's does internally, not efficient in a render loop.

[kulicuu]

Can you try the following patch? I got it to compile but haven't tested if it is functional.

Index: examples/webgl/src/main.rs
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/examples/webgl/src/main.rs b/examples/webgl/src/main.rs
--- a/examples/webgl/src/main.rs	(revision ba80b8d5abe3b07f0d02b68bc3437a9bd5a93664)
+++ b/examples/webgl/src/main.rs	(date 1656526514345)
@@ -1,10 +1,10 @@
-use web_sys::{HtmlCanvasElement, WebGlRenderingContext as GL, window};
+use web_sys::{HtmlCanvasElement, WebGlProgram, WebGlRenderingContext as GL, window};
 use yew::html::Scope;
 use yew::{html, Component, Context, Html, NodeRef};
 use wasm_bindgen::prelude::*;
 use wasm_bindgen::JsCast;
 
-use std::cell::RefCell;
+use std::cell::{Cell, RefCell};
 use std::rc::Rc;
 
 use gloo_console::log;
@@ -59,10 +59,10 @@
 
     fn render_gl(&mut self, link: &Scope<Self>) {
         log!("This shouldn't repeat every frame.");
-        
+
         let gl = self.gl.as_ref().expect("GL Context not initialized!");
 
-        let mut timestamp = 0.0;
+        let mut timestamp: Cell<f32> = Cell::new(0.0);
 
         let vert_code = include_str!("./basic.vert");
         let frag_code = include_str!("./basic.frag");
@@ -99,7 +99,7 @@
 
         // Attach the time as a uniform for the GL context.
         let time = gl.get_uniform_location(&shader_program, "u_time");
-        gl.uniform1f(time.as_ref(), timestamp as f32);
+        gl.uniform1f(time.as_ref(), timestamp.get());
 
         gl.draw_arrays(GL::TRIANGLES, 0, 6);
         let gl = gl.clone();
@@ -107,14 +107,16 @@
         let f = Rc::new(RefCell::new(None));
         let g = f.clone();
 
-        *g.borrow_mut() = Some(Closure::wrap(Box::new(move || {
+        fn handle_animation_frame(gl: Rc<GL>, timestamp: Cell<f32>, shader_program: WebGlProgram) {
             log!("This should repeat every frame.");
-            timestamp+= 20.0;
+            timestamp.set(timestamp.get() + 20.0);
             let time = gl.get_uniform_location(&shader_program, "u_time");
-            gl.uniform1f(time.as_ref(), timestamp as f32);
+            gl.uniform1f(time.as_ref(), timestamp.get());
             gl.draw_arrays(GL::TRIANGLES, 0, 6);
-            App::request_animation_frame(f.borrow().as_ref().unwrap());
-        }) as Box<dyn FnMut()>));
+            gloo_render::request_animation_frame(move |_| handle_animation_frame(gl, timestamp, shader_program));
+        }
+
+        gloo_render::request_animation_frame(move |_| handle_animation_frame(gl, timestamp, shader_program));
 
         App::request_animation_frame(g.borrow().as_ref().unwrap());
     }
Index: examples/webgl/Cargo.toml
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/examples/webgl/Cargo.toml b/examples/webgl/Cargo.toml
--- a/examples/webgl/Cargo.toml	(revision ba80b8d5abe3b07f0d02b68bc3437a9bd5a93664)
+++ b/examples/webgl/Cargo.toml	(date 1656526190118)
@@ -10,6 +10,7 @@
 wasm-bindgen = "0.2"
 yew = { path = "../../packages/yew", features = ["csr"] }
 gloo-console = "0.2.1"
+gloo-render = "0.1"
 
 [dependencies.web-sys]
 version = "0.3"

See comment above. It looks to me the gloo-render request-animation-frame is sub-optimal, especially in the render loop.

https://docs.rs/gloo-render/latest/src/gloo_render/lib.rs.html#41-65

You don't want those operations performed every frame, if you can possibly extract them, which is what I've done.

Versus web-sys, which will not have those ops -- direct bind.

[ranile]

I see. A comment explaining why we're doing it manually would do.

Also, please fix the CI errors (except for benchmark; that's unrelated)

[kulicuu]

The approach is nice, thanks for your efforts. Just a few thoughts of mine to address. And running cargo fmt (maybe you can turn on format-on-save in your editor?) should make CI happy afterwards.

I'm in the process of refactoring through an extension, which can be seen at https://github.com/kulicuu/yew_webgl2_game/blob/master/src/components/game.rs

There are some bad-form problems, you have hit on at least some of them, and I am intending to resolve all. Thank you for the feedback. I will respond by item to above specific comments. I'm looking at how to make the Yew/webgl example more complicated, with uniform-buffer generated transforms. I'm very long from webgl work, so just now getting reacquainted with web-sys/MDN api.

[kulicuu]

I'll try to wrap this up this week with something clean. I'm still learning the CI tools. The Cargo fmt issue I couldn't figure out what was causing it.

[WorldSEnder]

I'll try to wrap this up this week with something clean. I'm still learning the CI tools. The Cargo fmt issue I couldn't figure out what was causing it.

Any updates on this? cargo fmt complains partially about whitespace, which can be hard to read. Though running cargo fmt from the workspace directory should format all the files.

[ranile]

I went ahead and updated the PR so it can be merged.