Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added the arc method to turtle with two examples #189

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions examples/beats.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
extern crate turtle;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
extern crate turtle;

extern crate is no longer necessary in the Rust 2018 edition.

use turtle::{Point, Turtle};

fn main() {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think your dragon_circle example does a great job of demonstrating the arc method. Let's keep that and delete this beats example. I appreciate DeltaManiac's work on this, but it probably isn't appropriate to have a company logo as an example in a project like this.

let mut turtle = Turtle::new();
let init_pos: Point = turtle.position();

let _radius_outer = 250.0;
turtle.set_pen_color("#fe0000");
turtle.pen_up();
turtle.go_to((init_pos.x - (_radius_outer), init_pos.y));
turtle.pen_down();
turtle.set_fill_color("#fe0000");
turtle.begin_fill();
turtle.arc(_radius_outer, None, None);
turtle.end_fill();

let _radius_mid = 115.0;
turtle.pen_up();
turtle.go_to((init_pos.x.round() - (_radius_mid), init_pos.y));
turtle.pen_down();
turtle.set_fill_color("white");
turtle.set_pen_color("white");
turtle.begin_fill();
turtle.arc(_radius_mid, None, None);
turtle.end_fill();

let _radius_inner = 60.0;
turtle.pen_up();
turtle.go_to((init_pos.x - (_radius_inner), init_pos.y));
turtle.pen_down();
turtle.set_fill_color("#fe0000");
turtle.begin_fill();
turtle.arc(_radius_inner, None, None);
turtle.end_fill();

let _size = 25.0;
turtle.pen_up();
turtle.go_to((init_pos.x - (_radius_mid) + _size - 2.0, init_pos.y));
turtle.pen_down();
turtle.set_pen_size(_size);
turtle.forward(250.0);
}
29 changes: 29 additions & 0 deletions examples/dragon_circle.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
extern crate turtle;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
extern crate turtle;

extern crate is no longer necessary in the Rust 2018 edition.

use turtle::Turtle;

enum Turn {
Left,
Right,
}

// Shameless copy from https://gist.github.com/fogleman/006e4069348fa163b8ae
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Shameless copy from https://gist.github.com/fogleman/006e4069348fa163b8ae
//! Based on: https://gist.github.com/fogleman/006e4069348fa163b8ae

Let's move this comment to the top of the file.


fn turn(i: i64) -> Turn {
let left = (((i & -i) << 1) & i) != 0;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know you didn't create this method of drawing this picture, so this suggestion is optional: Could you try to explain this line a bit and why it works? Maybe link to an explanation if one can be found online.

if left {
return Turn::Left;
} else {
return Turn::Right;
}
}

fn main() {
let mut turtle = Turtle::new();
turtle.set_speed("instant");
for i in 1..100000 {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot from 2020-08-05 12-59-17

Looks like the image is getting cut off at the current window size. We should consider shifting the whole drawing up a bit (using pen_up, forward, and then pen_down before this loop). We may also want to decrease the number of iterations to something that fits inside the window and finishes in a reasonable time.

match turn(i) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we could avoid some unnecessary complexity by moving the body of turn into this function.

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

    for i in 1i64..100000 {
        let should_turn_left = (((i & -i) << 1) & i) != 0;
        if should_turn_left {
            turtle.arc(-4.0, Some(90.0), Some(36))
        } else {
            turtle.arc(4.0, Some(90.0), Some(36)),
        }
    }
}

This avoids the Turn enum entirely and turns the entire example into a single main function.

Turn::Left => turtle.arc(-4.0, Some(90.0), Some(36)),
Turn::Right => turtle.arc(4.0, Some(90.0), Some(36)),
}
}
}
53 changes: 40 additions & 13 deletions src/async_turtle.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
use std::f64::consts::PI;
use std::fmt::Debug;

use tokio::time;

use crate::radians::{self, Radians};
use crate::ipc_protocol::{ProtocolClient, RotationDirection};
use crate::radians::{self, Radians};
use crate::renderer_server::TurtleId;
use crate::{Turtle, Color, Point, Speed};
use crate::{Color, Point, Speed, Turtle};

/// Any distance value (positive or negative)
pub type Distance = f64;
Expand Down Expand Up @@ -58,8 +59,7 @@ impl AsyncTurtle {
// of many programs that use the turtle crate.
crate::start();

let client = ProtocolClient::new().await
.expect("unable to create renderer client");
let client = ProtocolClient::new().await.expect("unable to create renderer client");
Self::with_client(client).await
}

Expand All @@ -68,7 +68,7 @@ impl AsyncTurtle {
let id = client.create_turtle().await;
let angle_unit = AngleUnit::Degrees;

Self {client, id, angle_unit}
Self { client, id, angle_unit }
}

pub async fn forward(&mut self, distance: Distance) {
Expand All @@ -87,7 +87,9 @@ impl AsyncTurtle {

pub async fn left(&mut self, angle: Angle) {
let angle = self.angle_unit.to_radians(angle);
self.client.rotate_in_place(self.id, angle, RotationDirection::Counterclockwise).await
self.client
.rotate_in_place(self.id, angle, RotationDirection::Counterclockwise)
.await
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like you have rustfmt running in your editor. Could you please remove all the changes that aren't relevant to the PR?

}

pub async fn wait(&mut self, secs: f64) {
Expand All @@ -101,6 +103,27 @@ impl AsyncTurtle {
time::delay_for(time::Duration::from_millis((secs * 1000.0) as u64)).await
}

pub async fn arc(&mut self, radius: Distance, extent: Option<Angle>, steps: Option<i32>) {
let angle = extent.unwrap_or(360.0);

// Arc Length = radius * angle in radians
let angle_rad = 2.0 * PI * angle.abs() / 360.0;
let arc_length = radius.abs() * angle_rad;

let step_count = steps.unwrap_or(((arc_length / 4.0).round() + 1.0) as i32);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a comment to explain this code? (I realize that it's taken from #100, so just do the best you can)

let step_length = arc_length / step_count as f64;
let step_angle = angle / step_count as f64;

for _ in 0..step_count {
self.forward(step_length).await;
if radius < 0.0 {
self.left(step_angle).await;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have to be careful using left and right in methods like this because the user is allowed to change the unit those methods take using use_degrees() and use_radians() (see Angle). It's probably better to base this on the turn_towards method and use the Radians type as well as methods on self.client instead. This suggestion will probably affect your calculation of angle_rad as well. Let me know if you have any questions about that!

} else {
self.right(step_angle).await;
}
}
}

pub fn into_sync(self) -> Turtle {
self.into()
}
Expand All @@ -122,13 +145,13 @@ impl AsyncTurtle {
}

pub async fn set_x(&mut self, x: f64) {
let Point {x: _, y} = self.position().await;
self.go_to(Point {x, y}).await
let Point { x: _, y } = self.position().await;
self.go_to(Point { x, y }).await
}

pub async fn set_y(&mut self, y: f64) {
let Point {x, y: _} = self.position().await;
self.go_to(Point {x, y}).await
let Point { x, y: _ } = self.position().await;
self.go_to(Point { x, y }).await
}

pub async fn home(&mut self) {
Expand All @@ -155,7 +178,9 @@ impl AsyncTurtle {
// Formula from: https://stackoverflow.com/a/24234924/551904
let angle = angle - radians::TWO_PI * ((angle + radians::PI) / radians::TWO_PI).floor();

self.client.rotate_in_place(self.id, angle, RotationDirection::Counterclockwise).await
self.client
.rotate_in_place(self.id, angle, RotationDirection::Counterclockwise)
.await
}

pub fn is_using_degrees(&self) -> bool {
Expand Down Expand Up @@ -291,13 +316,15 @@ impl AsyncTurtle {
angle
};

self.client.rotate_in_place(self.id, angle, RotationDirection::Counterclockwise).await
self.client
.rotate_in_place(self.id, angle, RotationDirection::Counterclockwise)
.await
}

pub async fn wait_for_click(&mut self) {
use crate::{
event::{MouseButton::LeftButton, PressedState::Pressed},
Event::MouseButton,
event::{PressedState::Pressed, MouseButton::LeftButton},
};

loop {
Expand Down
52 changes: 48 additions & 4 deletions src/turtle.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
use std::fmt::Debug;

use crate::{Color, Point, Speed, Distance, Angle};
use crate::async_turtle::AsyncTurtle;
use crate::sync_runtime::block_on;
use crate::{Angle, Color, Distance, Point, Speed};

/// A turtle with a pen attached to its tail
///
Expand All @@ -26,7 +26,7 @@ impl Default for Turtle {

impl From<AsyncTurtle> for Turtle {
fn from(turtle: AsyncTurtle) -> Self {
Self {turtle}
Self { turtle }
}
}

Expand Down Expand Up @@ -199,6 +199,46 @@ impl Turtle {
block_on(self.turtle.wait(secs))
}

/// Instructs the turtle to draw an arc by the given angle for a given radius in the given step count from the current position of the turtle.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Instructs the turtle to draw an arc by the given angle for a given radius in the given step count from the current position of the turtle.
/// Draws a circle or an arc with the given radius from the current position of the turtle.

Updated based on the docs for the circle function in the Python turtle module

///
/// The `radius` parameter is a floation point number that represents the radius of the arc
/// that you want to draw. If a negative `radius` is passed its absolute value is
/// considered for drawing the arc.
Comment on lines +204 to +206
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// The `radius` parameter is a floation point number that represents the radius of the arc
/// that you want to draw. If a negative `radius` is passed its absolute value is
/// considered for drawing the arc.
/// The `radius` parameter represents the radius of the arc that you want to draw.
/// If a negative `radius` is passed its absolute value is considered for drawing the arc.

Fixed a typo and rewording a bit.

Are we sure that we should be taking the absolute value of the radius? The Python circle function actually allows for radius to be negative:

Draw the arc in counterclockwise direction if radius is positive, otherwise in clockwise direction.

///
/// The `extent` parameter is a floating point number that represents how much you want the
/// turtle to rotate. If the `extent` parameter is `None` then it is defaulted to 360. If
/// a negative `extent` is passed the arc is drawn in the opposite direction.
Comment on lines +208 to +210
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// The `extent` parameter is a floating point number that represents how much you want the
/// turtle to rotate. If the `extent` parameter is `None` then it is defaulted to 360. If
/// a negative `extent` is passed the arc is drawn in the opposite direction.
/// The `extent` parameter is an angle that represents how much of the circle to draw.
/// If `None`, the entire circle will be drawn. If negative, the arc will be drawn in
/// the opposite direction.

Instead of specifying 360, I used "the entire circle" since the actual default value should depend on what the current angle unit is set to.

///
/// the `steps` parameter is an integer that represent over how many steps you want the turtle to draw. If the `steps` parameter
/// is `None` then it is defaulted to the arc_length / 4.0.
Comment on lines +212 to +213
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// the `steps` parameter is an integer that represent over how many steps you want the turtle to draw. If the `steps` parameter
/// is `None` then it is defaulted to the arc_length / 4.0.
/// The `steps` parameter represents how many steps to take while drawing the arc. If
/// `None`, the value will be determined automatically based on the extent and radius
/// of the arc.

We don't want to commit to how we're calculating the steps, so I modified this line to be a bit more vague.

///
/// #Example
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// #Example
/// # Example

nit: whitespace

///
/// ```rust
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// ```rust
/// ```rust,no_run

Since there are no assertions in this example, it can probably be marked as no_run

/// # use turtle::*;
/// # let mut turtle = Turtle::new();
/// // Turtle will draw an arc of 90 in a clockwise direction
/// turtle.arc(100.0, Some(90.0), None);
///
/// // Turtle will draw an arc of 90 in a anti-clockwise direction
/// turtle.arc(-100.0, Some(90.0), None);
///
/// // Turtle will draw an arc of 90 in a clockwise direction
/// turtle.arc(-100.0, Some(-90.0), None);
///
/// //Turtle will draw an arc of 90 in a anti-clockwise direction
/// turtle.arc(-100.0, Some(90.0), None);
///
/// // Turtle will draw an arc of 90 in a clockwise direction in 10 steps
/// turtle.arc(100.0, Some(90.0), Some(10));
///
/// // Turtle will draw an arc of 90 in a clockwise direction in 100 steps
/// turtle.arc(100.0, Some(90.0), Some(100));
Comment on lines +220 to +236
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// // Turtle will draw an arc of 90 in a clockwise direction
/// turtle.arc(100.0, Some(90.0), None);
///
/// // Turtle will draw an arc of 90 in a anti-clockwise direction
/// turtle.arc(-100.0, Some(90.0), None);
///
/// // Turtle will draw an arc of 90 in a clockwise direction
/// turtle.arc(-100.0, Some(-90.0), None);
///
/// //Turtle will draw an arc of 90 in a anti-clockwise direction
/// turtle.arc(-100.0, Some(90.0), None);
///
/// // Turtle will draw an arc of 90 in a clockwise direction in 10 steps
/// turtle.arc(100.0, Some(90.0), Some(10));
///
/// // Turtle will draw an arc of 90 in a clockwise direction in 100 steps
/// turtle.arc(100.0, Some(90.0), Some(100));
/// // Both of these lines will cause the turtle to draw
/// // a 90 degree clockwise arc (by turning right)
/// turtle.arc(100.0, Some(90.0), None);
/// turtle.arc(-100.0, Some(-90.0), None);
///
/// // Both of these lines will cause the turtle to draw
/// // a 90 degree counterclockwise arc (by turning left)
/// turtle.arc(-100.0, Some(90.0), None);
/// turtle.arc(100.0, Some(-90.0), None);
///
/// // A 90 degree clockwise arc in 10 steps
/// turtle.arc(100.0, Some(90.0), Some(10));
/// // A 90 degree clockwise arc in 100 steps
/// turtle.arc(100.0, Some(90.0), Some(100));

When using an example to try to teach the subtleties of an API to someone, it's good to group related statements together so readers can see which small differences lead to the same outcome. I've changed this example to group some of the lines you added together so the user can see how the signs of the radius and extent affect the output.

Note: If your other documentation about us taking the absolute value of the radius is true, are these examples even accurate? I think this should be fine once we fix the code to no longer take the absolute value. We should definitely make sure we're accurate about the behaviour of our code in our examples!

/// ```
pub fn arc(&mut self, radius: Distance, extent: Option<Angle>, steps: Option<i32>) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub fn arc(&mut self, radius: Distance, extent: Option<Angle>, steps: Option<i32>) {
#[cfg(feature = "unstable")] //TODO: Should we use the builder pattern here instead?
pub fn arc(&mut self, radius: Distance, extent: Option<Angle>, steps: Option<i32>) {

Let's make this method unstable for now. I am still questioning whether we should use Option in the parameters or maybe use the builder pattern instead. If we do take an Option type, we might want to take Into<Option> instead so you can pass in the numbers directly without having to wrap them in Some.

block_on(self.turtle.arc(radius, extent, steps))
}

pub(crate) fn into_async(self) -> AsyncTurtle {
self.turtle
}
Expand Down Expand Up @@ -964,7 +1004,9 @@ mod tests {
}

#[test]
#[should_panic(expected = "Invalid color: Color { red: NaN, green: 0.0, blue: 0.0, alpha: 0.0 }. See the color module documentation for more information.")]
#[should_panic(
expected = "Invalid color: Color { red: NaN, green: 0.0, blue: 0.0, alpha: 0.0 }. See the color module documentation for more information."
)]
fn rejects_invalid_pen_color() {
let mut turtle = Turtle::new();
turtle.set_pen_color(Color {
Expand All @@ -976,7 +1018,9 @@ mod tests {
}

#[test]
#[should_panic(expected = "Invalid color: Color { red: NaN, green: 0.0, blue: 0.0, alpha: 0.0 }. See the color module documentation for more information.")]
#[should_panic(
expected = "Invalid color: Color { red: NaN, green: 0.0, blue: 0.0, alpha: 0.0 }. See the color module documentation for more information."
)]
fn rejects_invalid_fill_color() {
let mut turtle = Turtle::new();
turtle.set_fill_color(Color {
Expand Down