sunjay / turtle

Create Animated Drawings in Rust
http://turtle.rs
Mozilla Public License 2.0
559 stars 54 forks source link

Set_Speed = "Instant" produces incorrect output #234

Open bharathk005 opened 3 years ago

bharathk005 commented 3 years ago

While setting the turtle speed to "instant" I am seeing issues with the fill color. Tried with turtle.set_speed(Speed::instant()); turtle.set_speed("instant");

image

sunjay commented 3 years ago

Could you tell me which Rust version (rustc --version --verbose) and operating system you are on? Please also provide your Cargo.lock file and tell me the exact command you ran to reproduce this.

This is definitely odd because setting the speed has nothing to do with how fills are calculated. It just affects how quickly the points are recorded. Does this still happen if the speed is very high (e.g. at 25)?

bharathk005 commented 3 years ago

@sunjay rustc 1.50.0 (cb75ad5db 2021-02-10) windows cargo run --example rust (from the root directory) Attaching Cargo.lock

bharathk005 commented 3 years ago

Cargo.lock.txt

bharathk005 commented 3 years ago

@sunjay I missed mentioning that setting the speed to 25 does not reproduce the issue. Occurs only when speed is set to "instant".

sunjay commented 3 years ago

Another thing I'd be curious to test is if it still happens if you set the IPC channel crate version to 0.14. We recently updated in #235 and this could happen if that crate is sending messages in the wrong order. (Sending messages in the wrong order shouldn't technically be possible because we always wait for a drawing to finish before sending the next one, but it's worth testing anyway just in case.)

bharathk005 commented 3 years ago

IPC channel is already in 0.14. I think #235 was merged 2 days ago. I never pulled from upstream after that.

sunjay commented 3 years ago

Ah okay. We can rule that out then. For anyone looking to work on this: I suggest printing out the messages being sent to the renderer process and making sure they are the same regardless of the speed. I also suggest printing out the display list and comparing at different speeds. Note that you don't need to run the entire example to debug this, just up to a portion of the drawing that appears different at different speeds.

bharathk005 commented 3 years ago

I put some print messages in rust example also in the send method of ipc_protocol Could see that setting "instant" and 25 produces the same output.

image

image

sunjay commented 3 years ago

Right. That's good. Then the next place I'd look is whether the display lists are the same.

sathwikmatsa commented 2 years ago

The issue arises from the way MoveAnimation deals with Instant speed. More precisely, on line 126 below: https://github.com/sunjay/turtle/blob/c9918c4e963a8257fb91eda0e355bac6c0f625fa/src/renderer_server/animation.rs#L118-L127 we update fill polygon with old position (or current position before animation) instead of new or target position.

sample code to reproduce:

use turtle::Turtle;

fn main() {
    let mut turtle = Turtle::new();
    turtle.set_speed(x);

    turtle.begin_fill();
    for _ in 0..2 {
        turtle.forward(200.0);
        turtle.right(90.0);
    }
    turtle.end_fill();
}
x is not instant x is instant
Screenshot from 2021-07-17 11-26-22 Screenshot from 2021-07-17 11-23-09

display_list.polygon_push(poly_handle, target_pos) will fix the issue.