Adding arc() method to Turtle

[DeltaManiac]

Fixes #82

[sunjay]

Hello! Thanks for adding this method! Also, congrats on getting issue/PR number #100! That's a big milestone for the project! 🎉

My apologies for not getting back to your issue comment. I saw it, thought of my response, and then must have forgotten to actually send it. 😅

Your implementation is a good start and I'd like to see how it behaves when it is used in a drawing. Could you add an example to the examples directory that uses this function to draw a couple of different arcs? In particular it would be good to try it with different pen thicknesses to see what happens. Try drawing parts of circles, entire circles, and then do bigger angles like 800 or something.

This will probably bring out some interesting edge cases and will help me verify the implementation.

I have some other review comments that I will add later when I have a chance to look at this PR in more detail. My response above should give you something to work on in the meantime.

Thanks again for adding this method! Let me know if you have any questions about any of this. 😄

[DeltaManiac]

Hey @sunjay !
Congrats on this being PR#100! :)

Ive added a simple example that draws the beats logo.

I did notice some issues while working with different pen sizes.

For example the below code

    let mut turtle = Turtle::new();
    turtle.drawing_mut().set_background_color("black");
    let _radius= 50.0;
    turtle.set_pen_size(100.0);
    turtle.set_pen_color("light blue");
    turtle.arc(_radius, None);

seems to generate this weird pattern.

I've notice this becomes more evident as the pen size approaches radius.

Radius = 2.0

    let mut turtle = Turtle::new();
    turtle.drawing_mut().set_background_color("black");
    let _radius= 2.0;
    turtle.set_pen_size(100.0);
    turtle.set_pen_color("light blue");
    turtle.arc(_radius, None);

Radius = 15.0

    let mut turtle = Turtle::new();
    turtle.drawing_mut().set_background_color("black");
    let _radius= 15.0;
    turtle.set_pen_size(100.0);
    turtle.set_pen_color("light blue");
    turtle.arc(_radius, None);

Radius = 25.0

    let mut turtle = Turtle::new();
    turtle.drawing_mut().set_background_color("black");
    let _radius= 25.0;
    turtle.set_pen_size(100.0);
    turtle.set_pen_color("light blue");
    turtle.arc(_radius, None);

Any ideas on what could be causing this behavior?

Cheers
Delta

[sunjay]

Hello! Thanks for continuing to work on this! 😄

Any ideas on what could be causing this behavior?

Yes! This is actually exactly why I wanted you to test with different line thicknesses. I had a suspicion that we would run into this issue. Check out this code in the Rust logo example:

turtle/examples/rust.rs

Lines 136 to 143 in 05a5fa1

	for _ in 0..120 {
	// By going forward and then backward, we avoid any "gaps" between the very small movements
	// To see why this is needed, try replacing this with a single movement of the difference
	// between the two movements.
	turtle.forward(1.0);
	turtle.backward(0.5);
	turtle.right(1.5);
	}

I suggest you try to replace those two movements in that code example with a single movement turtle.forward(0.5) to see what happens.

Explanation

The problem is that when turtle draws, it takes very small steps. If the pen thickness is very large, these steps become very wide but still only go forward a short distance. This ends up resulting in the turtle drawing several very thin lines perpendicular to itself. That's what you're seeing in the screenshots that you provided.

Potential Solution

The solution to this is to make our approach a little more sophisticated. Instead of drawing individual lines, we should probably use something like a CircleArc from the piston-graphics crate to draw a more complete shape.

Background

To give you an idea of what we would have to do, let's look at how turtle.forward is implemented.

This is the source code of that method:

turtle/src/turtle.rs

Lines 118 to 120 in 05a5fa1

	pub fn forward(&mut self, distance: Distance) {
	self.window.borrow_mut().forward(distance);
	}

This calls the method with the same name on TurtleWindow:

turtle/src/turtle_window.rs

Lines 127 to 159 in 05a5fa1

	/// Move the turtle forward by the given distance. To move backwards, use a negative distance.
	///
	/// The turtle's motion will be animated based on the speed
	pub fn forward(&mut self, distance: Distance) {
	if !distance.is_normal() {
	return;
	}
	let TurtleState {
	position: start,
	speed,
	heading,
	pen,
	..
	} = self.fetch_turtle();
	let movement = Point {
	x: distance * heading.cos(),
	y: distance * heading.sin(),
	};
	let end = start + movement;
	let speed = speed.to_movement(); // px per second
	// We take the absolute value because the time is always positive, even if distance is negative
	let total_millis = (distance / speed * 1000.).abs();
	let animation = MoveAnimation {
	path: Path { start, end, pen },
	timer: Timer::start(),
	total_millis,
	};
	self.play_animation(animation);
	}

This method calculates the start, end, and duration of the movement animation, starts a timer, and then begins to play that animation synchronously.

turtle/src/animation.rs

Lines 67 to 90 in 05a5fa1

	fn advance(&self, turtle: &mut TurtleState) -> AnimationStatus {
	use self::AnimationStatus::*;
	let MoveAnimation {
	ref path,
	ref timer,
	total_millis,
	} = *self;
	let elapsed = timer.elapsed_millis() as f64;
	if elapsed >= total_millis {
	turtle.position = path.end;
	Complete(Some(path.clone()))
	} else {
	// t is the total progress made in the animation so far
	let t = elapsed / total_millis;
	turtle.position = lerp(&path.start, &path.end, &t);
	Running(Some(Path {
	start: path.start,
	end: turtle.position,
	pen: path.pen.clone(),
	}))
	}
	}

The animation is driven by the advance method on the MoveAnimation struct which calculates the state of the animation at the current point that it should be at based on the timer that we passed in initially. This works because we linearly interpolate between the start and end positions that we calculated in TurtleWindow::forward. We do that using the lerp function provided in the interpolation crate.

If the animation is still running, we pass in a Path that will be used to render the temporary path that just exists for the purpose of animation. Once the animation is complete, we return a Path that the renderer should store and continue to render from then onwards.

turtle/src/turtle_window.rs

Lines 184 to 200 in 05a5fa1

	fn play_animation<A: Animation>(&mut self, animation: A) {
	loop {
	// We want to keep the lock for as little time as possible
	let status = self.with_turtle_mut(|turtle| animation.advance(turtle));
	match status {
	AnimationStatus::Running(path) => self.set_temporary_path(path),
	AnimationStatus::Complete(path) => {
	if let Some(path) = path {
	self.set_temporary_path(None);
	self.send_drawing_command(StorePath(path));
	}
	break;
	}
	}
	}
	}

These paths are sent to the renderer process to be drawn by the renderer via a drawing command:

turtle/src/query.rs

Lines 25 to 36 in 05a5fa1

	#[derive(Debug, Clone, Serialize, Deserialize)]
	pub enum StateUpdate {
	TurtleState(TurtleState),
	DrawingState(DrawingState),
	TemporaryPath(Option<Path>),
	}
	#[derive(Debug, Clone, Serialize, Deserialize)]
	pub enum DrawingCommand {
	/// When a path is finished being animated, it needs to be persisted in the renderer
	/// so it can be redrawn every frame
	StorePath(Path),

The drawing commands are processed in the following:

turtle/src/renderer.rs

Lines 153 to 166 in 05a5fa1

	StorePath(path) => {
	if self.fill_polygon.is_some() {
	let &mut (ref mut border, ref mut poly) = self.fill_polygon.as_mut().unwrap();
	border.push(path.clone());
	let Path { start, end, .. } = path;
	if poly.vertices.last().map_or(true, |&v| v != start) {
	poly.vertices.push(start);
	}
	poly.vertices.push(end);
	} else if path.pen.enabled {
	self.drawings.push(Drawing::Path(path));
	}
	}

Then, the temporary path and each stored path are rendered in the renderer:

turtle/src/renderer.rs

Lines 200 to 246 in 05a5fa1

	/// The main rendering route. Dispatches to other functions as needed.
	fn render(
	&self,
	c: context::Context,
	g: &mut G2d,
	center: Point,
	drawing: &DrawingState,
	temporary_path: &Option<Path>,
	turtle: &TurtleState,
	) {
	let background = drawing.background;
	clear(background.into(), g);
	for drawing in &self.drawings {
	match *drawing {
	Drawing::Path(ref path) => self.render_path(c, g, center, path),
	Drawing::Polygon(ref poly) => self.render_polygon(c, g, center, poly.fill_color, poly.vertices.iter()),
	}
	}
	if let Some(&(ref border, ref poly)) = self.fill_polygon.as_ref() {
	// If the temporary_path is not None, we need to add it to the polygon being
	// filled or else the polygon will fall one edge behind in the animation
	let extra = temporary_path.as_ref().map_or(Vec::new(), |&Path { start, end, .. }| {
	if poly.vertices.last().map_or(true, |&v| v != start) {
	vec![start, end]
	} else {
	vec![end]
	}
	});
	self.render_polygon(c, g, center, poly.fill_color, poly.vertices.iter().chain(extra.iter()));
	for path in border {
	if path.pen.enabled {
	self.render_path(c, g, center, path);
	}
	}
	}
	if let Some(ref path) = *temporary_path {
	if path.pen.enabled {
	self.render_path(c, g, center, path);
	}
	}
	self.render_shell(c, g, center, turtle);
	}

The result of all of this is the lines that show up on your screen!

Mentoring Instructions

To implement arc using CircleArc, we would probably have to do something similar to these steps. We would essentially be introducing a new drawing primitive like Path but specifically for drawing arcs. Along every step of the way, instead of drawing lines, we would need to draw part of an arc. We would probably need to introduce new drawing commands for arcs and even a new temporary_arc variable to mimic the temporary_path we have now. Instead of interpolating on the distance, we would interpolate on the angle in radians.

Looking at all of this, it is actually a pretty monumental task to get arc support setup! I'm so sorry I didn't see this sooner! It took your experimenting for me to realize that a more complicated solution is actually necessary.

If you would like to continue working on this, I can take you there step by step through a series of smaller PRs. This is too big to be done in a single contribution.

That being said, the size and scope of this task has suddenly gotten quite big so I totally understand if you don't have the time to work on it. :)

Let me know what you are willing to do (and have time for). Sorry again for not anticipating this sooner.

[DeltaManiac]

Hey @sunjay

Thanks for the detailed explanation about what causes the issue.

Regarding further course of action the changes necessary could be broken down into smaller issues which can be picked up as time permits.

[sunjay]

Thank you for being understanding! I will add more information to the original issue and close this PR for the time being. I really appreciate you investing your time and my apologies for not realizing the complexity of this issue sooner.