This fixes the crash, but it does not means the caching is done correctly. Under the assumption that trying to reload the graphical or logical ('code') tiles is something done only when loading a new level, or explicitly changing / reloading the texture, I reasoned that an imperceptible delay at those situations were a small price to pay to not fight with cache correctness, so I eliminated the cache.
Later in that PR I presented a proof of concept about how the cache strategy implemented in the code (using ctimes) fails in a simple and real use case. Maybe reloading if any of ctime or mtime changes could fix that case. But, it will work in similar scenarios involving source code control (checkout this, unstash that, etc) or decompression from archives (zip, rar, 7z, ...) ??
But your reluctance to eliminate the cache made me to explore the code to see how much it was called this code, and this is what I found:
function load_tiles_and_codes which is the one doing the cacheing, is called only whitin the same file, at only two locations
One location is class _app.__init__ , the class is instantiated only by func init_app, which is only called by func main. in the loop
so, init_app will be called as many times as exception Restart is called.
Exception Restart is defined in leveledit, and is raised when:
the 'New' dialog terminates
the 'Open' dialog terminates
File/Reload is selected (or his keybinding is selected)
So this invocation of func load_tiles_and_codes is, as expected, when loading / reloading or creating a level. I don't see a significant performance hit here.
the only other call to load_tiles_and_codes is more puzzling:
The call happens in func cmd_refreshtiles , which only appears in
top.connect(pygame.ACTIVEEVENT, cmd_refreshtiles,None)
where top is the top window.
Not much info about pygame.ACTIVEEVENT[0], but roughtly is called when the pygame window has some changes in focus state.
That is not frequent in real use cases, and furthermore all considered it does not make sense: If cache would be working right, the whole ACTIVEEVENT invocation would be "if theres some change in focus, check if the file(s) changed, If yes reload them"
To me it looks as a fast hack to reload tiles, instead of adding a menu item "reload tiles", possibly with the use case "if user edits a level and context switchs to a program to edit the tileset, then when going back to leveledit it will not need to remember to reload the tileset"
And, changing the body of cmd_refreshtiles (the one called when ACTIVEEVENT) to 'pass' and running leveledit, no matter what in focus change (minimize, maximize, alt tab, hoover mouse on non focused window, ... no bad efects are seen.
In any case, the call to cmd_refreshtiles is infrequent. so even without caching it will not impact usability.
In my opinion is not worth to put work to getting caching to behave correctly
Recap
With the code in master, at commit "Improves setup.py layout", jsbueno committed on 19 Apr 2019 starting leveledit as
the script crashes, as mentioned in Problem 1 of PR #46
Fixing the crash is trivial, just a matter to change line 151 in leveleditor from
to
and similar change at line 167, https://github.com/parogers/pgu/blob/8d378586e8cec82d54510dc2d921ee2c5617dd22/scripts/leveledit#L167
This fixes the crash, but it does not means the caching is done correctly. Under the assumption that trying to reload the graphical or logical ('code') tiles is something done only when loading a new level, or explicitly changing / reloading the texture, I reasoned that an imperceptible delay at those situations were a small price to pay to not fight with cache correctness, so I eliminated the cache.
Later in that PR I presented a proof of concept about how the cache strategy implemented in the code (using ctimes) fails in a simple and real use case. Maybe reloading if any of ctime or mtime changes could fix that case. But, it will work in similar scenarios involving source code control (checkout this, unstash that, etc) or decompression from archives (zip, rar, 7z, ...) ??
But your reluctance to eliminate the cache made me to explore the code to see how much it was called this code, and this is what I found:
class _app.__init__, the class is instantiated only by func init_app, which is only called by func main. in the loopso, init_app will be called as many times as exception Restart is called. Exception Restart is defined in leveledit, and is raised when:
the only other call to load_tiles_and_codes is more puzzling: The call happens in func cmd_refreshtiles , which only appears in top.connect(pygame.ACTIVEEVENT, cmd_refreshtiles,None) where top is the top window. Not much info about pygame.ACTIVEEVENT[0], but roughtly is called when the pygame window has some changes in focus state. That is not frequent in real use cases, and furthermore all considered it does not make sense: If cache would be working right, the whole ACTIVEEVENT invocation would be "if theres some change in focus, check if the file(s) changed, If yes reload them" To me it looks as a fast hack to reload tiles, instead of adding a menu item "reload tiles", possibly with the use case "if user edits a level and context switchs to a program to edit the tileset, then when going back to leveledit it will not need to remember to reload the tileset"
And, changing the body of cmd_refreshtiles (the one called when ACTIVEEVENT) to 'pass' and running leveledit, no matter what in focus change (minimize, maximize, alt tab, hoover mouse on non focused window, ... no bad efects are seen.
In any case, the call to cmd_refreshtiles is infrequent. so even without caching it will not impact usability.
In my opinion is not worth to put work to getting caching to behave correctly
[0] https://stackoverflow.com/questions/61488114/in-pygame-for-event-type-activeevent-where-is-the-documentation-on-what-the