Skytils / SkytilsMod

Skytils is a Hypixel Skyblock mod! Be careful, malicious copies are distributed across GitHub. Confirm on discord.gg/skytils (807302538558308352)
https://hypixel.net/threads/3856202/
GNU Affero General Public License v3.0
1.12k stars 433 forks source link

Waypoint categories #204

Closed FluxCapacitor2 closed 2 years ago

FluxCapacitor2 commented 2 years ago

Allows waypoints to be grouped into categories.

I've done a fair bit of testing, but this PR is marked as a draft until I'm sure that this won't cause a bunch of game crashes.

Have a great day! Skytils on top 🚀 This resolves suggestions 2021, 2003, 1990, 1950, 1786, 1416, 1406, 1269, 1192, 1130, 1069, 1054, 952, 842

FluxCapacitor2 commented 2 years ago

Haven't had any crashes or unexpected behavior so I think it's ready for review. The only things to keep in mind are the edge case I mentioned before, and the desired behavior of sorting.

Currently, each category is sorted in place and the order of categories isn't changed. Another solution would be sorting each category's waypoints, and then re-ordering each category by their first waypoint in the sort.

Let me know which one sounds better, if you think it matters enough to change the implementation

FluxCapacitor2 commented 2 years ago

Here are some images of the GUIs: Waypoints GUI: image Waypoints GUI with collapsed categories: image Waypoint sharing GUI: image

I haven't found any issues or crashes yet but there are some behaviors that might not be intended:

My-Name-Is-Jeff commented 2 years ago
  • Renaming two categories to the same name and reopening the GUI will cause them to combine. I think the only way to change this is to change how it's stored to be in like an array format
  • Clicking the "New Waypoints" button on the bottom always adds a waypoint in the "Uncategorized" category, is this what we want? Should there be some sort of picker to choose which category to go in? Maybe the button can just be removed? If each category has its own button to add a new waypoint then there isn't really a reason to keep it.
  • Typing in the search field, clicking the "toggle visible" button, or changing the current sorting will expand all categories prior to performing that operation.
  • Clicking "Remove Visible" only removes elements from non-collapsed categories, even if those elements meet the search criteria. I think that's fine because they are the only ones that are visible. Maybe with the waypoint categories, there isn't a need for the visible buttons anymore?
  • Should expanding and collapsing waypoint groups be implemented on the waypoint sharing GUI? It currently is not.
  • Expanding and collapsing categories are not included in the PersistentSave's data, so all categories are expanded by default when the menu is opened. I think the only way to fix this is to make like a new file or completely restructure the current file.
FluxCapacitor2 commented 2 years ago

I think the only way to change this is to change how it's stored to be in like an array format

Do you think it would be better to keep the file as-is or restructure the file and add some code to migrate old waypoint saves to the new format?

Also, when waypoints are imported from the clipboard, they are expected to be in a very specific format, and I don't know if we can maintain backward-compatibility after changing the format (unless categories aren't exported). I could just take the easy way and add a warning next to categories that have duplicate names, but that doesn't address saving the expanded/collapsed state of categories (if that's a necessary addition)

My-Name-Is-Jeff commented 2 years ago

I think the only way to change this is to change how it's stored to be in like an array format

Do you think it would be better to keep the file as-is or restructure the file and add some code to migrate old waypoint saves to the new format?

Also, when waypoints are imported from the clipboard, they are expected to be in a very specific format, and I don't know if we can maintain backward-compatibility after changing the format (unless categories aren't exported). I could just take the easy way and add a warning next to categories that have duplicate names, but that doesn't address saving the expanded/collapsed state of categories (if that's a necessary addition)

I think it would probably be better to just migrate the old saves to a newer format as the original save did not plan for categories. (This would fix the naming issue) For importing you can probably just run it through the same conversion function.

Expanding/Collapsed state of categories is kind of a QoL thing for the GUI which would also be addressed by a newer format

FluxCapacitor2 commented 2 years ago

I kept the function for exporting waypoints with the old save format, should we give users the option to use this? Another possibility is, when exporting, using the old format but adding the category name as a key to each waypoint, so newer Skytils versions can use this to create categories, and older Skytils versions will just discard it. Something like:

{ // example of an intermediary format that could be used for exporting/importing but not used for saving
  "name": "Waypoint 1",
  "island": "dynamic",
  "category": "category 1",
  "x": 8,
  "y": 100,
  "z": 5,
  "enabled": true,
  "color": -65536,
  "addedAt": 1650775702895
}, ...

With the way it's being done right now, the exported string will look something like this, which is not compatible with older Skytils versions:

{
  "categories": [ // I keyed the `categories` instead of keeping it at the top level just in case another key would be added in the future
    {
      "name": "category 1",
      "island": "mining_3", // categories are now bound to one island
      "isExpanded": true, // and they have their own expanded state which is respected in the UI when loading
      "waypoints": [
        {
          "name": "waypoint 3",
          "x": 80,
          "y": 25,
          "z": 80,
          "enabled": true,
          "color": -65536,
          "addedAt": 1650834983541
        } // end waypoint
      ] // end waypoint list
    } // end category
  ] // end category list
}
FluxCapacitor2 commented 2 years ago

I have no idea why Codacy complained earlier that the addNewWaypoint method had too many parameters but it doesn't anymore! 🎉 image Also, sorry about committing twice. I meant to amend the original commit because I forgot to import the util that I made, but I forgot to stage the changes before committing.

The only thing we need to consider now is the exporting compatibility with older versions of the mod. The old code is still there but currently unused by the "Export Selected Waypoints" button.

My-Name-Is-Jeff commented 2 years ago

I think it's not worth supporting old versions now that Microsoft Defender falsely flags them

Sychic commented 2 years ago

Going to wait for #218 to be merged before reviewing.