nathanielwarner / flightgear-photoscenery

FlightGear - PhotoScenery Edition
GNU General Public License v2.0
21 stars 6 forks source link

Add option --clear-cache #16

Closed frougon closed 1 year ago

frougon commented 1 year ago

Hi Nathaniel,

Summary (commit message)

Using --clear-cache=PROVIDER clears the tile cache for PROVIDER. If PROVIDER is the special value ALL, then all provider-specific subdirs are cleared. The option may be used with none of --lat, --lon and --index if one only wants to clear the tile cache. It is also possible to combine the clearing and downloading operations in a single command, in which case cache clearing happens first.

Remarks

  1. I added the --clear_cache alias, however if you prefer to only use --this-style for new options and update the existing docs to replace --scenery_folder with --scenery-folder, etc., that would be quite fine with me.

  2. Currently, the keys of the URLS global variable (i.e., provider names) are directly used as subdir names within the tile cache dir. In case you expect future keys of URLS to be problematic as folder names (e.g., a key containing a slash), we should start to add some mapping from these keys to safe dir names (this may be considered as borderline over-engineering, though ;-)).

  3. I believe the other calls to exit() should be replaced with sys.exit() according to the documentation of the constants module. This is trivial, I can file a PR for this, of course.

Thanks!

nathanielwarner commented 1 year ago

Am I correct in understanding that the cache will always be empty once the orthophotos are successfully downloaded and stitched together? So this command would only be run in the case that the process got interrupted by a network error, and the user wants to get rid of the sub-tiles?

(Side note: it seems like the network errors on ArcGIS are increasingly common nowadays. I wonder if it's gonna get more and more unreliable with photoscenery support being enabled in stable FlightGear, and more people using it...)

nathanielwarner commented 1 year ago

I didn't know the issue with exit vs. sys.exit()! Feel free to do another PR for that after this one is resolved. And thanks for all the contributions by the way!

frougon commented 1 year ago

Am I correct in understanding that the cache will always be empty once the orthophotos are successfully downloaded and stitched together? So this command would only be run in the case that the process got interrupted by a network error, and the user wants to get rid of the sub-tiles?

Yes, that's about it. --clear-cache should be rarely needed, however I felt that --cache-dir would be somewhat incomplete without --clear-cache. Cached tiles take more than a few kilobytes, so if you aren't going to use FG in months or years, maybe you don't want to keep the remaining ones. Another use case would be if there is a reason to believe that some cached files would be corrupt due to a bug somewhere. Nothing very frequent, hopefully!

(In particular, --clear-cache should almost never encounter the tmptile-...png* files because they should always be deleted—except in extreme cases like when the Python process receives a SIGKILL, or in case of a power outage. The finally clause of line 357 wouldn't be run in such cases if one were downloading a tile, so it makes sense to have the regexp match these temporary files.)

(Side note: it seems like the network errors on ArcGIS are increasingly common nowadays. I wonder if it's gonna get more and more unreliable with photoscenery support being enabled in stable FlightGear, and more people using it...)

Yup... As said, I only started with this a few days ago and had a fair number of transient HTTP 502 errors... which is why I got to look at what happens in these cases. :)

frougon commented 1 year ago

While we're talking Python stuff, although I don't think there is anything “wrong” about it, I'm not sure why you do args = vars(parser.parse_args()). Without thar vars(), you could access option values with args.info_only instead of args['info_only'], etc., which is slightly less verbose.

Regards

nathanielwarner commented 1 year ago

Looks good! I've merged it now, and made the change to sys.exit() in the remaining places.

nathanielwarner commented 1 year ago

While we're talking Python stuff, although I don't think there is anything “wrong” about it, I'm not sure why you do args = vars(parser.parse_args()). Without thar vars(), you could access option values with args.info_only instead of args['info_only'], etc., which is slightly less verbose.

Regards

True enough! Though I don't think it makes any practical difference, unlike the issue with how you exit.

frougon commented 1 year ago

Right, there isn't any difference in behavior; it's simply that I usually try to stay under 78 or 80 columns, so the shorter syntax helps a little bit. Not a big deal, of course.

Thanks for the merges and happy new year! (with 45 minutes of advance from Paris local time) :-)

nathanielwarner commented 1 year ago

Right, there isn't any difference in behavior; it's simply that I usually try to stay under 78 or 80 columns, so the shorter syntax helps a little bit. Not a big deal, of course.

Thanks for the merges and happy new year! (with 45 minutes of advance from Paris local time) :-)

Happy new year to you as well from Boston!