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

[python] Turtle module #748

Open
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@boricj
Contributor

boricj commented Oct 27, 2018

This is a native implementation of the classic Python turtle module, complete with a cute lil' turtle icon. It's actually a fully native implementation in order to improve performance rendering. This currently adds a little bit over 2 KiB of Flash to the firmware.

from turtle import *
for i in range(255):
  gray=255-i
  color(gray,int(gray*0.75),int(gray*0.25))
  forward(i*0.1)
  left(10)

image

There are lots of improvements to do before this can be merged:

  • Add more turtle functions
  • Add alternative, shorter function names
  • Add toolbox menu for turtle module
  • Use a clock to control turtle speed in real-time (needs #626 merged first)
  • Allow interruption of turtle drawing (needs clock first)
  • Fix build dependency on turtle icon
  • (maybe?) Improve turtle logo rendering

Fixes #156.

@boricj

This comment has been minimized.

Contributor

boricj commented Oct 30, 2018

I've added the turtle module to the Python toolbox and some extra functions. It's very usable now, even if without speed-dialing menus like TI-83s it's a bit tedious to select things in the toolbox...

I've decided to leave out a lot of things from the standard module in order to keep the implementation simple. Nevertheless, I've kept most short function name substitutes implemented but not shown inside the toolbox in order to (hopefully) increase compatibility a bit with samples on the net for the full-blown CPython turtle module. I've left out a couple of "advanced" functions out of the toolbox to try and keep the toolbox at a reasonable size without confusing novice users.

I also added two new Python sample scripts, a spiral and a koch fractal. The turtle implementation could use some comments, but otherwise the code should be in fairly good shape for an initial review (except for the clock bit to control turtle speed).

@Ecco

This comment has been minimized.

Contributor

Ecco commented Nov 3, 2018

Wow, definitely something we need to merge! Thank you very much for this awesome contribution @boricj ! We'll try to review this as soon as possible 😄

@boricj

This comment has been minimized.

Contributor

boricj commented Nov 3, 2018

Thanks @Ecco!

Now that I look back on this, I do wonder if improving the Python runtime environment so that we can run the turtle module on top of it would be a better solution. After all, there's really nothing mandating a native implementation over a bytecode MicroPython module here, except for the lack of a decent Python platform API... Besides, calculator game programmers would very much prefer this option for obvious reasons.

@boricj

This comment has been minimized.

Contributor

boricj commented Nov 5, 2018

I've translated most of this native implementation to Python and it is indeed a much better solution. There's much less glue to maintain, the turtle implementation is looking far less scary in Python and the ability to interrupt a drawing comes "for free" since we're inside the MicroPython environment.

The downsize is that the sheer size of the turtle module written in Python won't fit in the 16 KiB MicroPython heap currently allocated. I need to figure out how to cross-compile the module to frozen MicroPython bytecode within the epsilon build system to make it fit.

@Ecco

This comment has been minimized.

Contributor

Ecco commented Nov 6, 2018

calculator game programmers would very much prefer this option for obvious reasons

Sorry, but they're really not obvious to me. Would you mind explaining why?

I've translated most of this native implementation to Python and it is indeed a much better solution.

I'd love to see some benchmarks. Not only on speed, but also on memory usage (RAM + Flash). I'd assume the C++ version would win, but who knows 😄 Also, I don't find the implementation scary, and pretty much everything in Epsilon is C++ anyway…

@Ecco

A small preliminary review 😄

}
mp_obj_t turtle_goto(size_t n_args, const mp_obj_t *args) {
float newx, newy;

This comment has been minimized.

@Ecco

Ecco Nov 6, 2018

Contributor

Use mp_float_t which is a dynamic type. MicroPythons floats can either be single or double precision floats, and in our case they are actually doubles.

Suggested change Beta
float newx, newy;
mp_float_t newx, newy;

This comment has been minimized.

@Ecco

Ecco Nov 6, 2018

Contributor

Also, we might want to remove single-precision floats altogether at some point, so unless performance is critical I suggest using doubles when possible.

if (t_dot) {
delete[] t_dot;
}
t_dot = new uint8_t[size*size];

This comment has been minimized.

@Ecco

Ecco Nov 6, 2018

Contributor

We're on a malloc diet. Please avoid heap allocations.

@@ -14,7 +14,7 @@ void KDContext::fillRect(KDRect rect, KDColor color) {
}
/* Note: we support the case where workingBuffer IS equal to pixels */
void KDContext::fillRectWithPixels(KDRect rect, const KDColor * pixels, KDColor * workingBuffer) {
void KDContext::fillRectWithPixels(KDRect rect, const KDColor * pixels, KDColor * workingBuffer, KDColor * prevPixels) {

This comment has been minimized.

@Ecco

Ecco Nov 6, 2018

Contributor

I'm rather uncomfortable modifying this, and I also have a hard time figuring out what you're trying to achieve here 😄 Would you mind explaining so we can see if there's a better solution?

This comment has been minimized.

@boricj

boricj Nov 6, 2018

Contributor

In order to move the turtle across the screen, I need to erase the turtle icon by blitting back the pixels under it, so I need a way to fetch those pixels that's more efficient than brute-forcing getPixel(). This is a very dirty hack to write-back the pixels under the turtle, but I've changed it to a more conventional fetchPixels(KDRect rect, KDColor * pixels) in my current Python rewriting (not online yet).

This comment has been minimized.

@Ecco

Ecco Nov 6, 2018

Contributor

I see 😄I think such an API could make sense in KDContext (a wrapper around pullRect and absoluteFillRect).

@boricj

This comment has been minimized.

Contributor

boricj commented Nov 6, 2018

calculator game programmers would very much prefer this option for obvious reasons

Sorry, but they're really not obvious to me. Would you mind explaining why?

One upside to a turtle implementation in Python is that the platform API must be improved to accommodate it (more drawing primitives inside kandinsky and a stripped-down time module). As a side effect it will expose these improvements to user scripts, even if we don't officially acknowledge their existence in the toolbox. Besides supporting the turtle, these new functions will be useful mostly for calculator game programmers (the only missing piece would be a non-blocking getKey(), but that's another story).

I've translated most of this native implementation to Python and it is indeed a much better solution.

I'd love to see some benchmarks. Not only on speed, but also on memory usage (RAM + Flash). I'd assume the C++ version would win, but who knows 😄 Also, I don't find the implementation scary, and pretty much everything in Epsilon is C++ anyway…

Except for speed(0) (max speed), I do not expect speed to be an issue since the turtle will spend most of its time inside time.sleep_ms() and drawing will still be performed by native code. I'm not sure how memory usage will turn out, especially since there are a bunch of different technical solutions for storing the turtle icons with different trade-offs characteristics, but I doubt similarly optimized solutions will exhibit a large delta.

"Scary" is perhaps not the most adequate word here, but having the bulk of the code written in plain Python does result in a much prettier implementation overall and conveniently solves a bunch of issues for free. I'll make a pull request with that version as soon as possible.

@Ecco

This comment has been minimized.

Contributor

Ecco commented Nov 6, 2018

more drawing primitives inside kandinsky and a stripped-down time module

I see. Note that it's something we could do anyway 😄

the only missing piece would be a non-blocking getKey(), but that's another story

Indeed, but that would be a rather short story, since there's already a C++ API for this: Ion::Keyboard::scan().

I doubt similarly optimized solutions will exhibit a large delta.

There's one specific area I think the C++ version would be better: it could bypass Python-Heap allocation altogether, leaving more heap for end users.

@boricj

This comment has been minimized.

Contributor

boricj commented Nov 6, 2018

the only missing piece would be a non-blocking getKey(), but that's another story

Indeed, but that would be a rather short story, since there's already a C++ API for this: Ion::Keyboard::scan().

Unless I'm mistaken, Ion::Keyboard::scan() is currently a placeholder on the Emscripten platform since the keys won't work in the hardware test on the online simulator. It should be fairly easy to fix, but it's not quite a story as short as you think 😄

@Ecco

This comment has been minimized.

Contributor

Ecco commented Nov 6, 2018

Unless I'm mistaken, Ion::Keyboard::scan() is currently a placeholder on the Emscripten platform

Indeed, you're a 100% right! We even have a lengthy comment on the matter 😄

@Ecco

This comment has been minimized.

Contributor

Ecco commented Nov 6, 2018

The more I'm thinking about this PR, the more I think I'd like the C++ version more than the Python one. It can be a great thing to actually compare both the C++ and the Python version, but all things equal I think I'd be more likely to eventually merge the C++ one: simpler build system, works like everything else, etc…

@boricj

This comment has been minimized.

Contributor

boricj commented Nov 6, 2018

I've opened a new pull request with the Python approach (#752). I think I'll close this pull request once feature parity has been reached as the other approach requires far less glue to work.

@boricj

This comment has been minimized.

Contributor

boricj commented Nov 6, 2018

The more I'm thinking about this PR, the more I think I'd like the C++ version more than the Python one. It can be a great thing to actually compare both the C++ and the Python version, but all things equal I think I'd be more likely to eventually merge the C++ one: simpler build system, works like everything else, etc…

It boils down to a matter of personal taste, but I think the native approach requires way too much domain-specific glue. The Python approach does require a bit of work to make the MicroPython cross-compiler fit in the epsilon build system, but it's not that bad and the side-effects are more useful (mechanism for additional bytecode modules in place for the future, generic improvements in kandinsky usable by everyone, keyboard interruption handled for free).

The native approach will have the edge on raw performance and memory consumption, but it's far more complicated to get right and it yields no improvements outside of the turtle module itself. Perhaps a hybrid approach (where drawing responsibilities are done by native code but the brain of the turtle is hosted by Python code) could bring the best of the two worlds...

@LeSeulArtichaut

This comment has been minimized.

LeSeulArtichaut commented Nov 7, 2018

Wouldn’t you adapt the color if the turtle icon, depending on the active color (just like in the official module, if I’m not wrong) ? Else, I’m not sure the icon currently used have to be changed, if I can give my opinions without having tested it.
Good luck on your work !

@boricj

This comment has been minimized.

Contributor

boricj commented Nov 7, 2018

I could, but that's most useful when you have several turtles on screen, which this implementation will not support. The icon does have 8 different directions to represent headings, but the graphics are just placeholders for now subject to change.

@LeSeulArtichaut

This comment has been minimized.

LeSeulArtichaut commented Nov 7, 2018

Strangely I get an error while building (similar in both "v1" and v2)
v1 : python/port/modturtle_impl.cpp:78:19: error: variable length array of non-POD element type 'KDColor'
v2 : python/port/mod_turtle_impl.cpp:46:24: error: variable length array of non-POD element type 'KDColor'
Have I missed something ? This is possible !
Thought, about my gcc compiler :

Apple LLVM version 6.1.0 (clang-602.0.53) (based on LLVM 3.6.0svn)
Target: x86_64-apple-darwin14.4.0
Thread model: posix
@boricj

This comment has been minimized.

Contributor

boricj commented Nov 8, 2018

Sounds like your compiler handles PODs the old, pre-C++11 way. Ensure -std=c++11 is set with a verbose build (V=1) and, if so, upgrade your toolchain 'cause it's buggy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment