move_to(circle, location) is buggy #15

jelkner commented 2021年04月28日 21:48:17 +02:00

I have added test_move_to_circle.py to the tests directory. It has a loop that calls move_to on a circle, sending it to the same location three times. It shouldn't move, but it does.

Also Text is displaying in the wrong y location. The location (100, 500) used in this test program should render near the top of the window, but instead it renders near the bottom.

I have added test_move_to_circle.py to the tests directory. It has a loop that calls move_to on a circle, sending it to the same location three times. It shouldn't move, but it does. Also Text is displaying in the wrong y location. The location (100, 500) used in this test program should render near the top of the window, but instead it renders near the bottom.

caleb33 commented 2021年04月29日 04:34:10 +02:00

I've resolved the issue with where Text is positioned.
In test_move_to_circle.py did you mean to start the circle at (400, 300) and then have it move_to (500, 300)? From what you described, the test should try "moving" it to (400, 300), right?

I've resolved the issue with where Text is positioned. In test_move_to_circle.py did you mean to start the circle at (400, 300) and then have it `move_to` (500, 300)? From what you described, the test should try "moving" it to (400, 300), right?

jelkner commented 2021年04月29日 12:22:52 +02:00

No Caleb. The problem is that the circle shouldn't move, but it does. The purpose of the test was that a student of mine pointed out this odd behavior, which the test reproduced for me. Inside the loop, the text will change, but the circle should stay where it is, since the coordinates in the move_to call don't change. Instead, we saw the circle move.

No Caleb. The problem is that the circle *shouldn't move*, but it does. The purpose of the test was that a student of mine pointed out this odd behavior, which the test reproduced for me. Inside the loop, the text will change, but the circle should stay where it is, since the coordinates in the move_to call don't change. Instead, we saw the circle move.

RichardMartinez commented 2021年04月29日 17:54:28 +02:00

I think I found the problem. On line 365 of gasp.py, the move_by() function sets obj.pos to obj.coord_list[0] etc. But obj.coord_list refers to the corners of the bounding box of the circle, NOT its center. Since a circle's position is defined by it's center, the moved circle will be off from its intended position by exactly obj.radius. The fix would be to add obj.radius to each of the values in the tuple on line 365. This makes it work for circles, but would throw an error if obj is a box for example because there is no radius attribute. I changed move_by() and it seems to work fine for both boxes and circles using test_move_to_circle.py.

def move_by(obj, x, y):
 """Translate each x coordinate right and each y coordinate up"""
 _canvas.move(obj.id, x, -y)
 c = obj.coord_list
 obj.coord_list = [c[i] + x if i % 2 == 0 else c[i] - y for i in range(len(c))]
 obj.pos = (obj.coord_list[0], _canvas_ys - obj.coord_list[1])
 if type(obj) is Circle:
 obj.pos = (obj.pos[0] + obj.radius, obj.pos[1] + obj.radius)
 _root_window.tk.dooneevent(tkinter._tkinter.DONT_WAIT)

There might be a more elegant way to fix this issue (maybe within the circle class itself) but the actual issue that needs to be fixed is layed out here.

I think I found the problem. On [line 365](https://codeberg.org/GASP/GASP_Tkinter/src/branch/main/gasp/gasp.py#L365) of gasp.py, the `move_by()` function sets obj.pos to obj.coord_list[0] etc. But obj.coord_list refers to the corners of the bounding box of the circle, NOT its center. Since a circle's position is defined by it's center, the moved circle will be off from its intended position by exactly `obj.radius`. The fix would be to add `obj.radius` to each of the values in the tuple on line 365. This makes it work for circles, but would throw an error if obj is a box for example because there is no radius attribute. I changed `move_by()` and it seems to work fine for both boxes and circles using `test_move_to_circle.py`. ``` def move_by(obj, x, y): """Translate each x coordinate right and each y coordinate up""" _canvas.move(obj.id, x, -y) c = obj.coord_list obj.coord_list = [c[i] + x if i % 2 == 0 else c[i] - y for i in range(len(c))] obj.pos = (obj.coord_list[0], _canvas_ys - obj.coord_list[1]) if type(obj) is Circle: obj.pos = (obj.pos[0] + obj.radius, obj.pos[1] + obj.radius) _root_window.tk.dooneevent(tkinter._tkinter.DONT_WAIT) ``` There might be a more elegant way to fix this issue (maybe within the circle class itself) but the actual issue that needs to be fixed is layed out here.

fcrisafulli commented 2021年04月30日 22:17:49 +02:00

Im not sure if it was the main problem but c = obj.coord_list creates a pointer to the list and not a copy. If #obj.coord_list = [c[i] + x if i % 2 == 0 else c[i] - y for i in range(len(c))] was written with the thought that c was a copy and not a pointer there may be something extra happening that the original writer didnt intend.

def move_by(obj, x, y):
 """Translate each x coordinate right and each y coordinate up"""
 _canvas.move(obj.id, x, -y) # relative move
 obj.pos = (obj.pos[0]+x, obj.pos[1]+y)
 #c = obj.coord_list
 #obj.coord_list = [c[i] + x if i % 2 == 0 else c[i] - y for i in range(len(c))]
 #obj.pos = (obj.coord_list[0], _canvas_ys - obj.coord_list[1])
 _root_window.tk.dooneevent(tkinter._tkinter.DONT_WAIT)

I removed the code related to obj.coord_list because I didnt see it being utilized anywhere other than init functions making it was a redundant set of coordinates. And since move_by is a relative move it doesn't matter if the center coordinate is used or one of the corners of the boundary.

Im not sure if it was the main problem but ```c = obj.coord_list``` creates a pointer to the list and not a copy. If ```#obj.coord_list = [c[i] + x if i % 2 == 0 else c[i] - y for i in range(len(c))]``` was written with the thought that c was a copy and not a pointer there may be something extra happening that the original writer didnt intend. ``` def move_by(obj, x, y): """Translate each x coordinate right and each y coordinate up""" _canvas.move(obj.id, x, -y) # relative move obj.pos = (obj.pos[0]+x, obj.pos[1]+y) #c = obj.coord_list #obj.coord_list = [c[i] + x if i % 2 == 0 else c[i] - y for i in range(len(c))] #obj.pos = (obj.coord_list[0], _canvas_ys - obj.coord_list[1]) _root_window.tk.dooneevent(tkinter._tkinter.DONT_WAIT) ``` I removed the code related to obj.coord_list because I didnt see it being utilized anywhere other than __init__ functions making it was a redundant set of coordinates. And since move_by is a relative move it doesn't matter if the center coordinate is used or one of the corners of the boundary.