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

graphics scaling error, deeper than turtle graphics? #47

Open
ricknun opened this issue Dec 7, 2024 · 7 comments
Open

graphics scaling error, deeper than turtle graphics? #47

ricknun opened this issue Dec 7, 2024 · 7 comments

Comments

@ricknun
Copy link

ricknun commented Dec 7, 2024

I am using a Circuit Playground Bluefruit with TFT Gizmo, adafruit-circuitpython-bundle-9.x-mpy-20241206.zip, and the "Turtle Gizmo Setup" code provided by John Park at https://learn.adafruit.com/turtle-graphics-gizmo/. See the attached file renamed from .txt to .py. From the code:

Issue: On a 240x240 pixel display with (0.0, 0.0) at the center, the
border pixels should include x,y values of 119.1 to 119.9 (actually
a slightly wider range, but at least those values). The following
code should make a visible square in both cases and does not. The
119.9 case is missing the right side of the square.

This was reduced from a more complicated program that required the
correct scaling of the 240x240 display including the border pixels.

turtle_scaling_issue.txt
20241207_131038

@FoamyGuy
Copy link
Contributor

FoamyGuy commented Dec 9, 2024

In order to conclude that it's an issue deeper than the turtle library I think it would need to be reproduced with only Bitmap and TileGrid. If you have, or discover, any reproducer code that is using directly those displayio objects rather than the Turtle library that exhibits issues please share and we can see if it needs an issue in the core.

With regards to the 119.9 value from the example, it's odd that the right edge and the bottom edge are different. Or I guess more generally that only 1 edge is different and not 2 edges.

My initial thought is that the 119.9 values should result in two of the lines being "drawn" outside of the bounds of the display and thus should not be visible.

Since the display size (240x240) is even there is no single exact middle pixel. I believe the code here is responsible for deciding upon the middle:

self._x = self._w // (2 * scale)
self._y = self._h // (2 * scale)
which is using integer division by 2 of the display width and height. So on a 240x240 display it will arrive at pixel (120,120).

If we start on pixel 120x120 and move 119.9 units we end up at the decimal location of 239.9 which gets rounded to 240 when its going to be plotted into the pixels of the Bitmap. The pixel index of 240 is outside of the Bitmap though because it's size is 240 and it's pixel indexes are zero based so they run 0-239. Because of this, in my mind it seems the right and bottom edges should both not have visible lines showing on the display from this code. The left and top edges should (and do) get shown because 120-119.9 = 0.1 which rounds to 0 i.e. the first row and column of pixels in the Bitmap.

Does that make sense to you @ricknun? Or have I misunderstood something perhaps?

I would need to trace more thoroughly through the code and add some debugging logs to try to understand why the bottom edge and right edge differ, that aspect of this does feel like a bug to me, but I think more likely within turtle library than deeper.

@ricknun
Copy link
Author

ricknun commented Dec 9, 2024

"...using integer division by 2 of the display width and height. So on a 240x240 display it will arrive at pixel (120,120)." That sounds like the problem. (I'm a retired software engineer with some pixel/graphics experience.) Pixels need to be thought of as large squares, not points. In the case of 240x240 pixels, the point (0.0, 0.0) should be at the corner where 4 pixels meet, with (+/- 119.5, +/- 119.5) being the centers of the 4 corner pixels. Then ceiling or floor (not round) should be used for all future point/line/circle/whatever float-to-integer conversions. What I think you describe maps the (0.0, 0.0) origin to THE CENTER of pixel (120,120) and that introduces just the kind of issue described here. It maps the origin point to sqrt(2)/2 ~= 0.707 pixels away from center. That results in longer distances from the origin being possible in 2 directions and shorter distances possible in the other two directions. I think we should first agree on the goal/requirements, then (what I am weak on) how to fix it.

Edit: Using floor or ceiling after floating point x and y math determines which of the 4 pixels that meet center-screen gets mapped to (0.0, 0.0). Since quadrant 1 (upper-right) is generally special in math and geometry, I think it makes sense to map (0.0, 0.0) to the upper-right of the 4 pixels that meet center-screen, pixel (120, 119). I think (check me) that this is done by x_pix = floor(120 + x) and y_pix = ceiling(119 - y). This maps both (0.0, 0.0) and (0.1, 0.1) into pixel (120, 119). (All this applies to the example of a 240x240 pixel display scaled to units of pixels.)

@ricknun
Copy link
Author

ricknun commented Dec 12, 2024

The proposed requirements are that points (+/- 119.5, +/- 119.5) map to the centers of the 4 corner pixels, and a 1-pixel dot at (0.0, 0.0) maps to pixel (120, 119). If that is done then connecting (+/- 119.9, +/- 119.9) will be a visible square and connecting (+/- 120.0, +/- 120.0) will be off-screen to the right and top of the display. I think that is correct behavior. Do you agree with the requirements? If not then why, and what are the requirements?

@FoamyGuy
Copy link
Contributor

Generally for libraries like this one which are CircuitPython python versions of existing CPython modules the goal is to match the behavior and API to the CPython version whenever possible. In some cases it's not possible to match all the way due to limitations of the microcontroller vs. larger computer or due to core APIs that result in differences like networking and display related things.

However with regards to how it handles the display size it's kind of tricky to compare to the CPython version. As far as I am aware in CPython turtle doesn't know about the display size. The closest analogous concept would be the window size in my mind, but it's behavior seems to differ even further away from how our display size works in circuitpython.

I ran this modified version of your test script in CPython and it results in the box being drawn with two edges off the display, and the other two edges being clearly not aligned to the edges of the window.

import math
import turtle
import time
import time
# Create a new turtle object
t = turtle.Turtle()

WIDTH, HEIGHT = 240, 240

screen = turtle.Screen()
screen.setup(WIDTH, HEIGHT)
screen.setworldcoordinates(0, 0, WIDTH//2, HEIGHT//2)

print(turtle.window_width(), turtle.window_height())

d = 119.9
print(d)
turtle.penup()
turtle.goto(-d, -d)
turtle.pendown()
turtle.goto(-d,  d)
turtle.goto( d,  d)
turtle.goto( d, -d)
turtle.goto(-d, -d)

# Keep the window open until manually closed
turtle.done()

image

I'm not really sure why it does that, I would expect something more similar to what we see in the CircuitPython version. But I haven't, and don't necessarily want to go down the potential rabbit hole on why the CPython one behaves how it does.

Since it doesn't seem easy to draw a direct comparison in behavior to the CPython one, it's left a bit more open to us how we want it to behave. I would not say that I have a strong opinion about mapping to the center of the 4 pixels vs. mapping specifically to one of them directly. In my mind I think it's unlikely to make much difference for the majority of people in the target audience for CircuitPython and the library. I think the biggest deciding factor to me would be how much extra code complexity and size it would require in order to account for this mapping to the center of 4 pixels. I'm not sure exactly how it would be done within the library code. If it's a relatively minimal amount of code and complexity to do so I think it is fine to consider. If it's a bit larger or requires the code become much more complex, personally I'd probably lean away from including in this library but it could always be forked and modified for anyone who wants it.

It's not a decision that is up to me unilaterally either, we hold weekly meetings open to the public on discord and have an "in the weeds" section where folks can raise in depth topics like proposed changes and things to discuss with the whole team. It's on Mondays at 2pm US Eastern time, though next week (12/16) is the last one of the year, the next couple of weeks we are skipping for holiday, but will return to the weekly schedule in January.

@ricknun
Copy link
Author

ricknun commented Dec 16, 2024

Please let's keep this issue and history here and not branch to Discord. If others should see this please point them here. I put more time into this and am more convinced that multiple issues need attention. The attached program (renamed to .py) shows that to within 1/50th of a pixel the x plot range is 240 pixels (good) and the y plot range is 241 pixels (bad). So not only is the (0.0, 0.0) origin misplaced to (0.5, -0.5), but vertical scaling is incorrect. Also (possibly should be a separate issue?) the program shows that changing turtle HEADING and then doing only goto() commands gives different plot results. See the file for details.
turtle_scaling_issues2.txt

@FoamyGuy
Copy link
Contributor

Please feel free to submit PR(s) to resolve any issues that you find and are able to find a solution to.

I won't be able to look much deeper into them myself for a while at least, but I can test any PRs that are submitted.

@ricknun
Copy link
Author

ricknun commented Dec 19, 2024

I won't be submitting any PRs -- I looked at the turtle code and find it too hard to understand. I am better at making it easy to replicate the problems, for example the files attached to this issue. Then the Adafruit authors that did the original coding can better fix the issues.

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

No branches or pull requests

2 participants