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

Implement #82: Add an arc method #206

Merged
merged 9 commits into from
Oct 11, 2020
Merged

Implement #82: Add an arc method #206

merged 9 commits into from
Oct 11, 2020

Conversation

PaulDance
Copy link
Contributor

@PaulDance PaulDance commented Oct 6, 2020

This is part of #82 by following the mentoring instructions. However, only the part 1 is covered in the current state, hence the draft. Part 2 will be implemented when the intermediate review is finished and adequate instructions completed.

Steps of the issue that have been covered:

  • Step 1: Add methods on Turtle called arc_left and arc_right that block on corresponding methods on AsyncTurtle.
  • Step 2: Add arc_left and arc_right asynchronous methods on AsyncTurtle that await on a new circular_arc method on ProtocolClient.
  • Step 3: Add the circular_arc method on ProtocolClient.
    • Signature changed from issue: added the necessary TurtleId.
    • Implementation with inscribed fixed-sized regular polygon approximation
    • using the move_forward and rotate_in_place methods.
    • No changes to Radians.
  • Documentation
    • Major points should be covered.
    • Detailed examples with assertions.
  • New flower example illustrating arcs
  • using different pen sizes.
  • Abnormal cases are ignored: new tests cover that in turtle::tests::ignores_nan_inf_zero.
  • The methods are marked as "unstable".
  • Tests run fine.

The documentation looks like this for now:

image
image
image
image

As a general comment, the overall structure should be satisfying, it is quite simple after all. However, the current implementation interestingly performs very poorly: it is imprecise, very slow, laggy and gets worse over time. That could be caused by a mistake or just by the simplistic algorithm used that generates hundreds of IPC requests. Either way, it is definitely not the intended final state.

They block on corresponding AsyncTurtle methods.

Signed-off-by: Paul Mabileau <[email protected]>
They call the circular_arc method of ProtocolClient with adapted values.

Signed-off-by: Paul Mabileau <[email protected]>
Classical implementation for now: splits the arc in many forward
movements and rotations.

Signed-off-by: Paul Mabileau <[email protected]>
They go in the `turtle::tests::ignores_nan_inf_zero` function and check
that all the possible combinations of those do not make the turtle move.

Signed-off-by: Paul Mabileau <[email protected]>
It should provide good explanations of both methods with dedicated
and thourough examples.

Signed-off-by: Paul Mabileau <[email protected]>
It draws a simple flower only using arcs, rotations and fills.

Signed-off-by: Paul Mabileau <[email protected]>
@codecov
Copy link

codecov bot commented Oct 6, 2020

Codecov Report

Merging #206 into master will increase coverage by 0.70%.
The diff coverage is 85.24%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #206      +/-   ##
==========================================
+ Coverage   60.81%   61.51%   +0.70%     
==========================================
  Files          40       40              
  Lines        2636     2697      +61     
==========================================
+ Hits         1603     1659      +56     
- Misses       1033     1038       +5     
Impacted Files Coverage Δ
src/async_turtle.rs 77.77% <62.50%> (-1.79%) ⬇️
src/ipc_protocol/protocol.rs 55.33% <66.66%> (+0.51%) ⬆️
src/turtle.rs 92.56% <100.00%> (+2.27%) ⬆️
src/point.rs 58.82% <0.00%> (+2.94%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 960049a...20fef67. Read the comment docs.

Copy link
Owner

@sunjay sunjay left a comment

Choose a reason for hiding this comment

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

Great work @PaulDance! I've left some comments for you to address. This should be good to merge after those are resolved. 🎉

We'll do further work in future PRs but I just want to get this merged as quickly as possible since it's been open for a while. 😄

As a general comment, the overall structure should be satisfying, it is quite simple after all. However, the current implementation interestingly performs very poorly: it is imprecise, very slow, laggy and gets worse over time. That could be caused by a mistake or just by the simplistic algorithm used that generates hundreds of IPC requests. Either way, it is definitely not the intended final state.

You are 100% correct that this is slow because it "generates hundreds of IPC requests". That's totally expected for now. We'll fix that in Part 2!

examples/flower.rs Show resolved Hide resolved
examples/flower.rs Show resolved Hide resolved
examples/flower.rs Outdated Show resolved Hide resolved
src/ipc_protocol/protocol.rs Outdated Show resolved Hide resolved
src/turtle.rs Outdated Show resolved Hide resolved
src/turtle.rs Outdated Show resolved Hide resolved
src/turtle.rs Outdated Show resolved Hide resolved
src/turtle.rs Outdated Show resolved Hide resolved
src/turtle.rs Outdated Show resolved Hide resolved
src/turtle.rs Show resolved Hide resolved
@sunjay
Copy link
Owner

sunjay commented Oct 10, 2020

Please also rebase this branch when you get a chance. :)

PaulDance and others added 2 commits October 11, 2020 22:00
Add a compile-time error indicating the need to use `features unstable`.
Rephrase some documentation and tweak minor things here and there.

Co-authored-by: Sunjay Varma <[email protected]>
@PaulDance PaulDance marked this pull request as ready for review October 11, 2020 20:02
@PaulDance PaulDance requested a review from sunjay October 11, 2020 20:06
Copy link
Owner

@sunjay sunjay left a comment

Choose a reason for hiding this comment

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

Great work @PaulDance! Appreciate you quickly resolving all my review comments. It should be ready to go!

@sunjay sunjay merged commit bdac20c into sunjay:master Oct 11, 2020
@PaulDance PaulDance deleted the arc-method branch October 11, 2020 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants