google / clojure-turtle

A Clojure library that implements the Logo programming language in a Clojure context
Apache License 2.0
425 stars 41 forks source link

Bug with the (clean) function #20

Closed atrus159 closed 8 years ago

atrus159 commented 8 years ago

This may only manifest on my setup, so I need someone to confirm this, but when I use the (clean) function in the in the CLJC repl the drawing position is reset to HOME. The turtle icon remains in its position and will respond to commands normally, but the line drawn behind the turtle will be disconnected from the turtle, drawn as if I had used the (home) command. Below are screenshots:

1 2 3 4 5 6

I believe I did a clean download before running this so it should be in the most recent version

atrus159 commented 8 years ago

So I actually have a fix to this but involved me adjusting some code that I don't know how it works so I'm not sure if I should submit it yet. My initial attempt was to simply add a (setxy) command in (clean), storing the x and y positions at the time (clean) was called and using those. This did remove the disjoint behavior, but caused the turtles triangle sprite to remain behind whenever a move command was called. While investigating I found that there was an invocation of (clean) in the (draw-turt-state) function. When this (clean) was removed the program immediately worked as intended with no problems. I do not know why this fixed it or if I broke something else in the process, but if a more elegant solution can not be found this one is available.

echeran commented 8 years ago

Good catch! I've been able to follow your commands easily and verify the behavior. It is a regression -- in other words, I don't recall happening previously. (Although, I can't seem to find a clean call inside a fn named similar to draw-turt-state.) The change in behavior is due to the way the turtle commands are drawn. Before, each command issued to the turtle would (or wouldn't) create a new line (segment) to be drawn from one point to another. We would store all lines to be drawn in a state container, and clean would empty the collection stored in that state container. Lines were correctly made to handle the statefulness of the turtle because they were created by wherever the turtle was and where it was going.

The work related to issue #2 prompted re-thinking how lines are defined (so as to handle shape fills, and color more generally), which triggered a re-thinking of how stateful info related to turtles are stored, displayed at the REPL, and rendered to the screen.

My current thought about what it would take to fix this issue is to add another field to the Turtle record, maybe :start-from, indicating from which point that the rendering of turtle's currently stored commands should start. In the draw-turtle-commands fn, there is an implicit assumption that all rendering must start at (0,0) because the loop state uses @(new-turtle) as-is. Meanwhile, rendering of the turtle sprite is correct because get-turtle-sprite ignores the turtle commands and just uses the turtle for its current position, and starts from there using (setxy (:x turt) (:y turt)) before creating the rest of the commands for drawing the turtle sprite. So I think clean should reset :start-from for the turtle it's dealing with, draw-turtle-commands should incorporate an initial setxy somehow (perhaps refactoring the (:commands turt) into a function that does so), and then the setxy call from get-turtle-sprite could be removed.

atrus159 commented 8 years ago

Hi, two things. One, I misspoke, the function is called get-turtle-sprite "A helper function that draws the triangle that represents the turtle onto the screen" and it occurs right before the second block of comments. Second, using my hacked together version I have some samples of animation using the wait function and would like to know if and where I should put them.

On Wed, Mar 30, 2016 at 8:46 AM, Elango notifications@github.com wrote:

Good catch! I've been able to follow your commands easily and verify the behavior. It is a regression -- in other words, I don't recall happening previously. (Although, I can't seem to find a clean call inside a fn named similar to draw-turt-state.) The change in behavior is due to the way the turtle commands are drawn. Before, each command issued to the turtle would (or wouldn't) create a new line (segment) to be drawn from one point to another. We would store all lines to be drawn in a state container, and clean would empty the collection stored in that state container. Lines were correctly made to handle the statefulness of the turtle because they were created by wherever the turtle was and where it was going.

The work related to issue #2 https://github.com/google/clojure-turtle/pull/2 prompted re-thinking how lines are defined (so as to handle shape fills, and color more generally), which triggered a re-thinking of how stateful info related to turtles are stored, displayed at the REPL, and rendered to the screen.

My current thought about what it would take to fix this issue is to add another field to the Turtle record, maybe :start-from, indicating from which point that the rendering of turtle's currently stored commands should start. In the draw-turtle-commands fn, there is an implicit assumption that all rendering must start at (0,0) because the loop state uses @(new-turtle) as-is. Meanwhile, rendering of the turtle sprite is correct because get-turtle-sprite ignores the turtle commands and just uses the turtle for its current position, and starts from there using (setxy (:x turt) (:y turt)) before creating the rest of the commands for drawing the turtle sprite. So I think clean should reset :start-from for the turtle it's dealing with, draw-turtle-commands should incorporate an initial setxy somehow (perhaps refactoring the (:commands turt) into a function that does so ), and then the setxy call from get-turtle-sprite could be removed.

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub https://github.com/google/clojure-turtle/issues/20#issuecomment-203497021

echeran commented 8 years ago

The fix was implemented similar to what was discussed above. See PR #21 for details.