asny / three-d

2D/3D renderer - makes it simple to draw stuff across platforms (including web)
MIT License
1.24k stars 105 forks source link

Efficient self access for `set_animation` #358

Closed wilfredjonathanjames closed 1 year ago

wilfredjonathanjames commented 1 year ago

Hi there, simple question. What's the recommended way to access a mesh in set_animation? I assume I'll have to put it in a Box or RefCell, or clone a reference, but there are no examples of this.

I'm trying to animate the height of a cube and peg the bottom in place, which I believe requires knowing the original position of the cube.

asny commented 1 year ago

I'm not entirely sure what you mean, set_animation is a method on a Mesh, so you should have access to the mesh to call that?

If you want to access the data for a Mesh, you'll have to look at the data you use to create the Mesh.

wilfredjonathanjames commented 1 year ago

Hi @asny, sorry if I confused you. I don't mean to access a mesh so that I can access set_animation, I mean accessing current mesh data inside of set_animation.

set_animation accepts a Fn, which captures any external variables it uses. Predictably when I try to use a mesh inside of a closure owned by the mesh itself, I get borrow checker errors.

Example code

```rust use three_d::*; pub fn main() { let window = Window::new(WindowSettings { title: "Shapes!".to_string(), max_size: Some((1280, 720)), ..Default::default() }) .unwrap(); let context = window.gl(); let mut camera = Camera::new_perspective( window.viewport(), vec3(5.0, 2.0, 2.5), vec3(0.0, 0.0, -0.5), vec3(0.0, 1.0, 0.0), degrees(45.0), 0.1, 1000.0, ); let mut cube = Gm::new( Mesh::new(&context, &CpuMesh::cube()), PhysicalMaterial::new_transparent( &context, &CpuMaterial { albedo: Color { r: 0, g: 255, b: 0, a: 200, }, ..Default::default() }, ), ); cube.set_transformation( Mat4::from_translation(vec3(0.0, 0.0, 0.0)) * Mat4::from_nonuniform_scale(0.2, 0.2, 0.2), ); cube.set_animation(|time| { // ERROR: cannot borrow `cube` as immutable because it is also borrowed as mutable let center = cube.transformation(); // << This predictably causes some issues Mat4::from_angle_x(radians(time * 0.5)) * Mat4::from_nonuniform_scale(1.0, 0.8 * time, 1.0) }); let light0 = DirectionalLight::new(&context, 1.0, Color::WHITE, &vec3(0.0, -0.5, -0.5)); let light1 = DirectionalLight::new(&context, 1.0, Color::WHITE, &vec3(0.0, 0.5, 0.5)); // ERROR: cannot move out of `cube` because it is borrowed window.render_loop(move |frame_input| { camera.set_viewport(frame_input.viewport); // HINT: move occurs due to use in closure cube.animate(0.001 * frame_input.accumulated_time as f32); frame_input .screen() .clear(ClearState::color_and_depth(1.0, 1.0, 1.0, 1.0, 1.0)) .render(&camera, cube.into_iter(), &[&light0, &light1]); FrameOutput::default() }); } ```

I've tried:

Keen to hear your thoughts.

wilfredjonathanjames commented 1 year ago

Here's an attempt to solve this with Arc and RwLock.

Example code

```rust use std::sync::{Arc, RwLock}; use three_d::*; pub fn main() { let window = Window::new(WindowSettings { title: "Shapes!".to_string(), max_size: Some((1280, 720)), ..Default::default() }) .unwrap(); let context = window.gl(); let mut camera = Camera::new_perspective( window.viewport(), vec3(5.0, 2.0, 2.5), vec3(0.0, 0.0, -0.5), vec3(0.0, 1.0, 0.0), degrees(45.0), 0.1, 1000.0, ); let mut cube = Arc::new(RwLock::new(Gm::new( Mesh::new(&context, &CpuMesh::cube()), PhysicalMaterial::new_transparent( &context, &CpuMaterial { albedo: Color { r: 0, g: 255, b: 0, a: 200, }, ..Default::default() }, ), ))); { let mut cube_deref = cube.write().unwrap(); cube_deref.set_transformation( Mat4::from_translation(vec3(0.0, 0.0, 0.0)) * Mat4::from_nonuniform_scale(0.2, 0.2, 0.2), ); let set_animation_cube = cube.clone(); cube_deref.set_animation(move |time| { let cube_deref = set_animation_cube.read().unwrap(); let center = cube_deref.transformation(); Mat4::from_angle_x(radians(time * 0.5)) * Mat4::from_nonuniform_scale(1.0, 0.8 * time, 1.0) }); } let light0 = DirectionalLight::new(&context, 1.0, Color::WHITE, &vec3(0.0, -0.5, -0.5)); let light1 = DirectionalLight::new(&context, 1.0, Color::WHITE, &vec3(0.0, 0.5, 0.5)); window.render_loop(move |frame_input| { let mut cube_deref = cube.write().unwrap(); camera.set_viewport(frame_input.viewport); cube_deref.animate(0.001 * frame_input.accumulated_time as f32); frame_input .screen() .clear(ClearState::color_and_depth(1.0, 1.0, 1.0, 1.0, 1.0)) .render(&camera, cube_deref.into_iter(), &[&light0, &light1]); FrameOutput::default() }); } ```

This panics with

thread 'main' panicked at 'rwlock read lock would result in deadlock',
/rustc/84c898d65adf2f39a5a98507f1fe0ce10a2b8dbc/library/std/src/sys/unix/locks/pthread_rwlock.rs:102:13

Probably because calling cube.animate(..) requires pulling a write lock from the RwLock wrapping cube, which panics when the cube.set_animate(..) Fn also tries to take a read lock.

asny commented 1 year ago

Ah, that makes much more sense, thanks for explaining. So an easy solution would be to pass the Mesh (or just the transformation of the mesh) into the animate function, so it would look like this: impl Fn(f32, &Self) -> Mat4 instead of impl Fn(f32) -> Mat4. I think that could make sense. However, before I make that change, I want to make sure that it's actually needed (in this case at least). So a couple of questions:

let transformation = cube.transformation().clone();
cube.set_animation(|time| {
   // Do something with the transformation
});
wilfredjonathanjames commented 1 year ago

Ah, I think I see what you're saying! Here's some code that is my best guess at what you mean. It:

I think I'm missing how to pin the bottom of the cube to 0 though. Am I meant to be able to set mesh transform origin using set_transformation? Is that what you mean by local space transformation?

Example code

```rust use three_d::*; pub fn main() { let window = Window::new(WindowSettings { title: "Shapes!".to_string(), max_size: Some((1280, 720)), ..Default::default() }) .unwrap(); let context = window.gl(); let mut camera = Camera::new_perspective( window.viewport(), vec3(5.0, 0.0, 0.0), vec3(0.0, 0.0, 0.0), vec3(0.0, 1.0, 0.0), degrees(60.0), 0.1, 1000.0, ); let mut cube = Gm::new( Mesh::new(&context, &CpuMesh::cube()), PhysicalMaterial::new_transparent( &context, &CpuMaterial { albedo: Color { r: 0, g: 255, b: 0, a: 200, }, ..Default::default() }, ), ); { cube.set_transformation( Mat4::from_translation(vec3(0.0, 0.1, 0.0)) * Mat4::from_scale(0.2), ); cube.set_animation(move |time| { Mat4::from_nonuniform_scale(1.0, f32::sin(0.8 * time) * 0.5 + 1.0, 1.0) }); } let light0 = DirectionalLight::new(&context, 1.0, Color::WHITE, &vec3(0.0, -0.5, -0.5)); let light1 = DirectionalLight::new(&context, 1.0, Color::WHITE, &vec3(0.0, 0.5, 0.5)); window.render_loop(move |frame_input| { camera.set_viewport(frame_input.viewport); cube.animate(0.01 * frame_input.accumulated_time as f32); frame_input .screen() .clear(ClearState::color_and_depth(1.0, 1.0, 1.0, 1.0, 1.0)) .render(&camera, cube.into_iter(), &[&light0, &light1]); FrameOutput::default() }); } ```

[Edit:]

Or do mean that I should reposition the mesh using set_transformation before the animate call, inside of render_loop? I think in that case handling the reposition inside of set_animation(Fn) would be cleaner. Concerns in one place etc.

asny commented 1 year ago

There's many ways to do this, but one could be to position and scale the mesh with bottom=0 on the CPU like this:

let mut cube = CpuMesh::cube();
cube.transform(&(Mat4::from_translation(vec3(0.0, 1.0, 0.0))
            * Mat4::from_scale(0.2)));
let mut cube = Gm::new(
    Mesh::new(&context, &cube),
    PhysicalMaterial::new_transparent(
        &context,
        &CpuMaterial {
            albedo: Color {
                r: 0,
                g: 255,
                b: 0,
                a: 200,
            },
            ..Default::default()
        },
    ),
);

Then set the animation

cube.set_animation(move |time| {
            Mat4::from_nonuniform_scale(1.0, f32::sin(0.8 * time) * 0.5 + 1.0, 1.0)
        });

and if you want to position the mesh somewhere in the world other than the origin, you can use set_transformation:

cube.set_transformation(
            Mat4::from_translation(vec3(10.0, 10.0, 10.0))
        );
wilfredjonathanjames commented 1 year ago

@asny Thanks for the code. I ran it, and still don't see the effect I'm after. This is what I got:

https://github.com/asny/three-d/assets/5732688/d1054a15-17d3-4fc7-9224-505942e7491a

And this is what I'm after:

https://github.com/asny/three-d/assets/5732688/3cb6180a-eade-4a83-93f4-c3ee8811f05f

Any other ideas?

asny commented 1 year ago

Yeah, I think I was wrong, it's

cube.transform(&(Mat4::from_scale(0.2) * Mat4::from_translation(vec3(0.0, 1.0, 0.0))));

instead of

cube.transform(&(Mat4::from_translation(vec3(0.0, 1.0, 0.0)) * Mat4::from_scale(0.2)));
wilfredjonathanjames commented 1 year ago

Hey that worked! Thanks! If I think of any other reasons why set_animation(Fn) would need access to the mesh I'll let you know, but this solves my problem :)