Artistry-mods / amethyst-shield

Creative Commons Zero v1.0 Universal
0 stars 3 forks source link

Problems of Auto Updater #4

Closed Skidamek closed 2 months ago

Skidamek commented 3 months ago

Hey, as you can see i really like auto updaters. Although it should be configurable, and disabled by default like i wrote comment there. You never know what issues it can cause... I don't mind little nudge that new version is available but the process of auto update should be disabled by default.

E.g. from my analyze it will likely always break with my mod AutoModpack. Due to fact that on client AutoModpack contains mods in separate folder other than default ~/mods/. Here is why. This mod will try to update and we end up in mod mismatch with server.

  1. AutoModpack loads older version on client
  2. When this mod loads it updates to newer version
  3. Client is still on older version until reboot Now restart.
  4. AutoModpack sees duplicate of this mod inside default ~/mods/ and so it removes it and forces to restart the game with modpack's version of the mod (older one)
  5. And we are in loop

That's from AutoModpack perspective but also similar thing can happen without it.

  1. Client loads older version
  2. This mod updates itself, but is still on older version until reboot
  3. Client joins their minecraft server (which isn't updated yet) Now client reboot.
  4. Client is on possibly mismatched version with server's one
HerrChaos commented 3 months ago

Ello, first of all. You are right. And second I will make it configurable but not disabled by default. The update might come tomorrow or even a bit later but it will come. Thx for your feedback.

HerrChaos commented 3 months ago

A config was even planned but this version of the auto update is rather experimental. Btw if you have any other questions or ideas or requests please contact me on DC since I check that more often. Dc: herrchaos

Skidamek commented 3 months ago

Ello, first of all. You are right. And second I will make it configurable but not disabled by default.

That's progress great thanks but please consider different implementation. I think mods which aren't designed to make update, do auto updates without first even asking if you want to update is objectively wrong.

I don't mind if by default there would be check if newer version exists and then in Minecraft GUI screen with a button to update itself without manually doing anything. (If you do that please add in config option to disable this screen/whole check)

Also to maintain compatibility with different launchers like feather, quest or mods like automdpack or mod sets by settingdust or even just fabric flag --fabric.addMods you shouldn't assume that the mod is in standard mods folder.

In best case scenario mod will update and completely nothing will happen and even update won't load.

In worst case scenario it will break someone modpack.

Please just make sure client won't get automatically mismatched with sever they're playing on just because they relauched the game and server haven't time to do that yet

HerrChaos commented 3 months ago

So first of all I can make a toggle to make a screen appear. And second maybe I just don't get your points but the auto update only updates mods which are using the lib, which u will eventually release when it's finished, so no mod which does not want to update will update. I am also not assuming that the default mod dir is the mod dir I am using the fabric loader in the code to tell me where the mods are being loaded from. Which ensures compatibility with client. The only one it tested is the modrinth client and that worked.

It sounds like the auto mod pack updater seems to have a lot of problems with it. I would be down to make a check for the mod pack updater and not update if it is present and maybe give the user a little "we've not updated this mod because it might cause issues" pop-up. I just need the mod Id from said mod.

I can't make sure the desync does not happen but I will try my best.

Oh and also I am not a big fan of the screen since it interruptes the player normally. So I would leave it off by default because you are talking about "edge" cases and just for them I don't want to make the normal user experience worse. But I will make a toggle for sure.

Skidamek commented 3 months ago

Is it an "edge" case?

  1. Client loads older version
  2. This mod updates itself, but is still on older version until reboot
  3. Client joins their minecraft server (which isn't updated yet) Now client reboot.
  4. Client is on possibly mismatched version with server's one

And yes, you are assuming that the only folder from which mods are loaded is the standard ~/mods/ directory. It always returns just standard mods directory.

And that's why if mod is loaded from not standard directory this check will always be true and so on every boot, update happens. (You can check that on modrinth launcher as well by loading amethyst-shield mod via this fabric flag from not standard mods directory)

And no, it does not cause problems for AutoModpack, it probably will cause bigger for Mod Sets. AutoModpack knows how to handle that, and deletes the updated mod, but its just frustrating because it requires a restart of entire client. (Making update configurable already solves that problem, it just will require adding to modpack, config of this mod for clients with disabled updater)

What I think the best solution would look like is to get path of currently loaded amethyst-shield mod by its mod id. Like this, then generate hash sha512 or sha1 from it and use this modrinth api call to determinate if current version is the latest and if no, download the latest for our minecraft & loader version.

HerrChaos commented 3 months ago

You clearly are way more experienced in this topic than I am. Since this is my second time using an API and I am also pretty new to modding. I thought that the fabric thing always works. I will be looking into this solution later. Tbh I assumed a lot of things about how stuff works, that's a bad approach. Is "skidam" your DC? If yes may I ask you for advice or help with this on there if I need it? (I also just want to get off GitHub so can we move this convo there?) Either way Thank you so much for your feedback. I will get on it after school. :)

Skidamek commented 3 months ago

Yes that's me, sure we can talk on discord.