GrandTheftWalrus / RuneLite-World-Heatmap

A plugin that lets you visualize the tiles of the Old School Runescape world map that you most often visit
BSD 2-Clause "Simplified" License
5 stars 3 forks source link

Improve directory structure and autosave feature #10

Closed JasperSurmont closed 2 months ago

JasperSurmont commented 2 months ago

This commit changes the directory structure as mentioned in #9. It shouldn't mess up any existing files, so users of the plugin should not directly be affected (except from getting used to the new structure).

The main reason that I'd like this structure is that using the autosave you can have a timeline of the different heatmaps throughout your career. However, since files are not overwritten anymore, they will take up more space the more saves you do (especially the images). Maybe an option could be made that sets a limit to the amount of images?

The proposed structure is as follows:

worldheatmap/
├─ Heatmap Files/
│  ├─ [userId1]/
│  │  ├─ 240617-2042.heatmaps
│  ├─ [userId2]/
│  │  ├─ 240611-2010.heatmaps
├─ Heatmap Images/
│  ├─ [userId1]/
│  │  ├─ 240616-2212TYPE_A.tif
│  │  ├─ 240616-2212TYPE_B.tif
│  ├─ [userId2]/
│  │  ├─ 240616-2100TYPE_A.tif

It now also saves explicit saves and autosaves in the same folders, as it will not overwrite anything anymore.

JasperSurmont commented 2 months ago

Also, the naming of the files is now abstracted to another class. This makes it less prone to small errors when you need to craft the filename every time separately.

GrandTheftWalrus commented 2 months ago

Sweet, I'll check this out either tomorrow after class or on Friday.

Also, I just remembered something upon seeing this pull request which I should've mentioned earlier. I only took a cursory gander at the code (which looks good) so I'm not sure if you've already addressed this, but: If the location at which the plugin saves and loads heatmap files is changed, then the save files of existing users won't be found unless there's a check performed for the old save location (which should move/remove them from the old location too). Abstracting the heatmap file logic to its own class makes this pretty easy though, so good idear.

JasperSurmont commented 2 months ago

I think I saw this, because it used to be stored in a different format?

I left that there for the time being.

JasperSurmont commented 2 months ago

Should be implemented and it worked on my (Linux) machine. Should probably be tested on Windows / Mac too

GrandTheftWalrus commented 2 months ago

@JasperSurmont Did you see the PR that I made to your fork? I would add ye as a reviewer but can't

JasperSurmont commented 2 months ago

@GrandTheftWalrus Sorry, I completely missed that. I'm not too familiar with PRs

JasperSurmont commented 2 months ago

Is this ready to be merged or am I missing something? @GrandTheftWalrus

GrandTheftWalrus commented 2 months ago

@JasperSurmont Oh yeah, it's good. I'm going to do some more thorough testing before I make the PR to the RuneLite plugin hub, but I forgot I could approve this PR meanwhile (now that my PR to your fork went through). lots of PR-ing.

JasperSurmont commented 2 months ago

@GrandTheftWalrus Okay perfect, thanks. Could you let me know when the PR to RL Hub is approved?

GrandTheftWalrus commented 2 months ago

@GrandTheftWalrus Okay perfect, thanks. Could you let me know when the PR to RL Hub is approved?

Sure cuh 💯🅱