search

parogers / pgu

Automatically exported from code.google.com/p/pgu
GNU Lesser General Public License v2.1
86 stars 36 forks source link

Fix leveledit py37 #46

Open ccanepa opened 4 years ago

ccanepa commented 4 years ago

This PR propose changes for multiple crashes in pgu/Scripts/leveledit

Testbed: win10 64 bits, python 3.3.7 64bits, pygame 1.9.6, pgu from github at commit commit 8d378586e8cec82d54510dc2d921ee2c5617dd22 (tag: 0.21, origin/master, origin/HEAD, master) Author: Joao S O Bueno Date: Fri Apr 19 02:48:28 2019 -0300

problem 1

Running leveledit

     cd D:\cla\dev2\pgu\examples
     python ../scripts/leveledit level.tga tiles.tga

It crashes with

Traceback (most recent call last):
  File "../scripts/leveledit", line 1387, in <module>
    main()
  File "../scripts/leveledit", line 1382, in main
    init_app()
  File "../scripts/leveledit", line 1288, in init_app
    app = _app()
  File "../scripts/leveledit", line 121, in __init__
    self.load_tiles_and_codes()
  File "../scripts/leveledit", line 151, in load_tiles_and_codes
    if newctime <= self.tiles_last_ctime:
TypeError: '<=' not supported between instances of 'int' and 'NoneType'

Analysis method _app.load_tiles_and_codes() in pgu/Scripts/leveledit tries to save some time when loading a tileset: it skips the load if the file ctime is <= than the ctime of the currently loaded tileset

Problem is, this fails when no tileset has been loaded, hence .tiles_last_ctime is None

The minimal fix would be to change if newctime <= self.tiles_last_ctime: to if self.tiles_last_ctime is not None and (newctime <= self.tiles_last_ctime):

But I think this strategy is wrong: what if you run some tool that changes the file contents and not its ctime? Loading time, even if perceptible, does not matter in an editor (for real cases :))

So I propose to get ride of this pattern which also repeats when loading the 'codes' part. The ctimes are only used for those tricks (I searched all the WD for the strings), so the checks are removed and the associated member variables are deleted.

This is the first commit in the PR, 'fix leveledit crash related to ctime'

problem 2

Trying to run after that patch gives

 cd D:\cla\dev2\pgu\examples
 python ../scripts/leveledit level.tga tiles.tga
Traceback (most recent call last):
  File "../scripts/leveledit", line 1369, in <module>
    main()
  File "../scripts/leveledit", line 1364, in main
    init_app()
  File "../scripts/leveledit", line 1313, in init_app
    e = app.vwrap = vwrap()
  File "../scripts/leveledit", line 381, in __init__
    self.vdraw = e = vdraw(width=w-sw,height=h-sw)
  File "../scripts/leveledit", line 436, in __init__
    for y in range(0,self.rect.w/inc):
TypeError: 'float' object cannot be interpreted as an integer

Analysis The relevant context is

        inc = 7
        for y in range(0,self.rect.w/inc):
            for x in range(0,self.rect.h/inc):
                s.fill(clrs[(x+y)%2],(x*inc,y*inc,inc,inc))

It is painting a chessboard of size self.rect, x,y being each tile corner. So integer division is desired, and it will be

        inc = 7
        for y in range(0,self.rect.w//inc):
            for x in range(0,self.rect.h//inc):
                s.fill(clrs[(x+y)%2],(x*inc,y*inc,inc,inc))

Whith this the app window shows, but crashes when trying to select the piramid tile in the top-right pane:

...
  File "..\pgu\gui\widget.py", line 323, in _event
    return self.event(e)
  File "..\pgu\gui\theme.py", line 354, in theme_event
    return func(sub)
  File "../scripts/leveledit", line 335, in event
    self.set(n)
  File "../scripts/leveledit", line 340, in set
    if n < 0 or n >= len(app.level.tiles) or app.level.tiles[n] == None: return
TypeError: list indices must be integers or slices, not float

Analysis The code involved is

    def event(self,e):
        if (e.type is MOUSEBUTTONDOWN and e.button == 1) or (e.type is MOUSEMOTION and e.buttons[0] == 1 and self.container.myfocus == self):
            w = app.tiles_w/app.tile_w
            x,y = e.pos[0]/app.tile_w,e.pos[1]/app.tile_h
            n = x+y*w
            self.set(n)
            ...
    def set(self,n):
        if n < 0 or n >= len(app.level.tiles) or app.level.tiles[n] == None: return
        app.tile = n

The tiles in the tileset are stored in the list app.level.tiles, the code in event wants to go from screen coords to list index, and it is clear that integer division is the correct way

The same happens clicking on a tile in the bottom-right panel, with same analysis and fix.

Both fixes in the second commit, "fix leveledit crash when click on right panels"

Problem 3

After the second patch, clicking on the Left crashes the app:

...
  File "../scripts/leveledit", line 512, in event
    if hasattr(self,a): getattr(self,a)(e)
  File "../scripts/leveledit", line 555, in tile_down
    self.tile_drag(e)
  File "../scripts/leveledit", line 566, in tile_drag
    app.level.tlayer[ty][tx] = app.tile
TypeError: list indices must be integers or slices, not float

The inmediate code context is:

    def tile_drag(self,e):
        pos = self.getpos(e)
        #r,g,b,a = app.view.get_at(pos)
        #r = app.tile
        #app.view.set_at(pos,(r,g,b))

        if pos == None: return
        tx,ty = pos
        app.mod(pygame.Rect(tx,ty,1,1))
        app.level.tlayer[ty][tx] = app.tile
        self.repaint()

It can be 'fixed' by intifyng tx, ty; but similar problems will pop. Maybe self.getpos(e) should return int, int ? Bactracking show this fragment in tilevid.py:

    def view_to_tile(self,pos):
        x,y = pos
        tiles = self.tiles
        tw,th = tiles[0].image.get_width(),tiles[0].image.get_height()
        return x  / tw, y // th

Here it is clear that x is missing an integer division

Changed this, test crash avoided, this is the 3rd commit, "fix leveledit crash when click in left panel"

(aside: in methods getpos and getpos2 , class vdraw in leveledit theres dead code after a return, I left that to not complicate the diff. This could be cleared at a later time if desired)

Problem 4

Start as alwayys, press Left Right Arrow key, crash:

...
  File "..\pgu\gui\theme.py", line 354, in theme_event
    return func(sub)
  File "../scripts/leveledit", line 289, in event
    cmd(value)
  File "../scripts/leveledit", line 869, in cmd_pick
    app.tpicker.set(n)
  File "../scripts/leveledit", line 340, in set
    if n < 0 or n >= len(app.level.tiles) or app.level.tiles[n] == None: return
TypeError: list indices must be integers or slices, not float

Analysis

The binding is defined at L 963,

keys = [
    ...
    (K_RIGHT,cmd_pick,(1,0)),

cmd_pick (around line 833) was called with value =(1, 0) and no shift or ctrl was pressed, so the code flows to the else, where it tries to advance one tile in the direction received as value. Needs int division.

With the int division the app does not crash when arrow, but going to right the selection box wrongly goes down one pixel each keypress.

Following the code, both cpicker.paint and tpicker.paint needs int division, and running with this change shows correct displacement with arrows in both right panels.

While looking at cmd_pick, the flow when an arrow is pressed with the shift key wants to slide the view by 1/8 of the view dimension in the relevant direction, and that calls for integer division to not set the level view origin to float value. Corrected.

This is the 4-th commit, "fix leveleditor crash when pressing arrows keys"

They are still other sections that need to use integer division and some other problems but i will stop here and wait for what happens with this PR

jsbueno commented 4 years ago

Ok - thank you for your analysis - you picked points that stopped working because they relied in Python 2 division behavior.

But why do your commits touch on the code dealing with tile-file loading, effectively removing a caching mechanism. Was that part involved in any of the above crashes?

ccanepa commented 4 years ago

Was that part involved in any of the above crashes? Yes, albeit it can be fixed without removing the cacheing.

I was biten by software that don't really reload a file if it think is not neccesary, and guessed wrong.

If you preffer, I can fix the other way.

ccanepa commented 4 years ago

Thinking about failure modes for the cacheing, I got a simple real use case when a user in windows makes a copy using Windows Explorer, edits the original, then deletes the edited and renames the copy to the original name.

It turns out that using ctime is just plain wrong in Windows, because

stat.ST_CTIME The “ctime” as reported by the operating system. On some systems (like Unix) is the time of the last metadata change, and, on others (like Windows), is the creation time (see platform documentation for details).

( from https://docs.python.org/3/library/stat.html )

using mtime seems better:

stat.ST_MTIME Time of last modification.

But it does not work as expected.

I wrote a litle script that uses load with cache to demo the problem with both times in Windows; 

#file xtime.py"
import datetime as dtt
import os
import stat

content = None
last_xtime = None
fname = "xtime_test.txt"

def load(mode):
    global content, last_xtime
    print("Loading...")
    newxtime = os.stat(fname)[mode]
    if last_xtime is not None and newxtime <= last_xtime:
        #nothing to do, so we return.
        return
    last_xtime = newxtime
    with open(fname) as f:
        content = f.read()

def show_info():
    print("content:", content)
    print("last_xtime:", last_xtime)
    clear_xtime = None if last_xtime is None else dtt.datetime.fromtimestamp(last_xtime)
    print("last_xtime as datetime:", clear_xtime)
    print("------------------------------------------------")

for mode_name, mode in [("ctime", stat.ST_CTIME), ("mtime", stat.ST_MTIME)]:
    print("================================================")
    print("CACHING USING:", mode_name)
    print("================================================")
    print()
    if os.path.isfile(fname):
        os.unlink(fname)

    content = None
    last_xtime = None
    print("expected content:", None)
    show_info()

    with open("xtime_test.txt", "w") as f:
        f.write("A")
    load(mode)
    print("expected content:", "A")
    show_info()

    s = """
    plase make a copy using explorer, in the same dir; thats is
      - open an explorer window in the directory the script lives
      - Rclick on file "xtime_test.txt", choose copy
      - Rclick in a free area and choose paste
    You should see a new file called similar to "xtime_test - copy.txt"

    When done, please press Return"""
    print(s)
    input()
    s = """
    Now edit the original file, "xtime_test.txt" so its contents are "B"

    When done, please press Return"""
    print(s)
    input()
    load(mode)
    print("expected content:", "B")
    show_info()

    s="""Now delete the original file, and edit the copy name in explorer
    so that it is called as the original, "xtime_test.txt"

    When done, please press Return"""
    print(s)
    input()
    load(mode)
    print("expected content:", "A")
    show_info()

    print()
    print("************************************************")
    print()

Which produces the output

D:\cla\dns_win10>py xtime.py
================================================
CACHING USING: ctime
================================================

expected content: None
content: None
last_xtime: None
last_xtime as datetime: None
------------------------------------------------
Loading...
expected content: A
content: A
last_xtime: 1590541981
last_xtime as datetime: 2020-05-26 22:13:01
------------------------------------------------

    plase make a copy using explorer, in the same dir; thats is
      - open an explorer window in the directory the script lives
      - Rclick on file "xtime_test.txt", choose copy
      - Rclick in a free area and choose paste
    You should see a new file called similar to "xtime_test - copy.txt"

    When done, please press Return

    Now edit the original file, "xtime_test.txt" so its contents are "B"

    When done, please press Return

Loading...
expected content: B                    <-- FAIL
content: A
last_xtime: 1590541981
last_xtime as datetime: 2020-05-26 22:13:01
------------------------------------------------
Now delete the original file, and edit the copy name in explorer
    so that it is called as the original, "xtime_test.txt"

    When done, please press Return

Loading...
expected content: A
content: A
last_xtime: 1590541981
last_xtime as datetime: 2020-05-26 22:13:01
------------------------------------------------

************************************************

================================================
CACHING USING: mtime
================================================

expected content: None
content: None
last_xtime: None
last_xtime as datetime: None
------------------------------------------------
Loading...
expected content: A
content: A
last_xtime: 1590545408
last_xtime as datetime: 2020-05-26 23:10:08
------------------------------------------------

    plase make a copy using explorer, in the same dir; thats is
      - open an explorer window in the directory the script lives
      - Rclick on file "xtime_test.txt", choose copy
      - Rclick in a free area and choose paste
    You should see a new file called similar to "xtime_test - copy.txt"

    When done, please press Return

    Now edit the original file, "xtime_test.txt" so its contents are "B"

    When done, please press Return

Loading...
expected content: B
content: B
last_xtime: 1590545453
last_xtime as datetime: 2020-05-26 23:10:53
------------------------------------------------
Now delete the original file, and edit the copy name in explorer
    so that it is called as the original, "xtime_test.txt"

    When done, please press Return

Loading...
expected content: A                  <-- FAIL
content: B
last_xtime: 1590545453
last_xtime as datetime: 2020-05-26 23:10:53
------------------------------------------------

************************************************

(the <-- FAIL were manually added)

ccanepa commented 4 years ago

Just FYI, I did some pases over pgu code in a new branch that follows the commits in this PR; I didn¡t want to pile too much changes in a PR; on the other side I didn't want to forget what I learned.

The changes are resticted to only integer division issues, plus one TAB-spaces mismatch, plus changing an open "wd" to "w" for loading one .ini file.

Theres one commit for each directory of interest, covering all python files in the parogers \pgu repo.

They are still issues, by example pressing TAB in leveledit will crash, If I remember correctly because changes in how iterators works.

I intend to fix this and other issues that may pop because I want to use pgu in some project of mine.

ccanepa commented 4 years ago

oh, the branch is https://github.com/ccanepa/pgu/tree/more_fixes

jsbueno commented 4 years ago

There is a lot of thing in there - the division errors are a trivial fix, and we should fix then ASAP.

The cache is not that trivial - it seems one on the danger of forcing a file reload each time the image is displayed - and is completely unrelated with the division problem.

If you prefer, I can copy your branch over, keep only the division fixes, and merge then into master with your name.

Otherwise, if you can fix it, just keep the division problem here, and let's see about the caching in another issue/pull request.

ccanepa commented 4 years ago

If you prefer, I can copy your branch over, keep only the division fixes, and merge then into master with your name.

Well, that would be the easier for me. If you volunteer, go ahead.

The cache is not that trivial - it seems one on the danger of forcing a file reload each time the image is displayed - and is completely unrelated with the division problem.

... and let's see about the caching in another issue/pull request.

Okay, I will create an issue for this.