search

ArchipelagoMW-HollowKnight / Archipelago.HollowKnight

Hollow Knight mod enabling Archipelago multiworld interoperability.
MIT License
5 stars 10 forks source link

Add an option for always showing items received while offline #221

Open thislooksfun opened 4 months ago

thislooksfun commented 4 months ago

I decided to take a stab at doing this myself. I'm happy to change it up if you'd rather it be done a different way.

Fixes #220.

BadMagic100 commented 4 months ago

This approach seems fine, was any testing done with this change?

thislooksfun commented 4 months ago

This approach seems fine, was any testing done with this change?

Yes it was, I have been running this version for a while now. I have discovered one issue that I want to resolve though. It does what I want with getting items, but it doesn't behave quite as I'd like with other players collecting their items. That blocks for a very long time with irrelevant information. I want to take a stab at fixing that.

thislooksfun commented 4 months ago

Ah, I've realized the issue. Releasing items while the game is running works as expected, but collecting items (via the AP server) does not. By that I mean that if a player !collects while this mod is running and connected, nothing will update until the save file is closed and re-opened. This can either be fixed so that !collecting while the mod is connected works instantly (and I'll add a second toggle for whether or not you want to list those items as well), or I can just never show the !collected items in the corner (but they still display in RecentItems, if that is installed, which is weird). I am personally leaning towards the former, but I'd like to hear your thoughts if you have any.

thislooksfun commented 4 months ago

^ rebased onto main, and marked as draft until the collect issue is resolved.

BadMagic100 commented 4 months ago

So the collect issue is:

That seems very odd, and strikes me as something which would be a multiclient issue. But if this is the case then I would tend to agree that the game should react immediately, and should probably not show the items either in the corner or recent items. Don't feel like it's worth it to be a toggle

thislooksfun commented 4 months ago

So the collect issue is:

  • Player A is playing HK
  • Player B collects
  • Player A's client never sees that the items were collected until reloading the file?

Correct.

That seems very odd, and strikes me as something which would be a multiclient issue.

Possibly. It might also be that the multiclient handles it correctly and this mod doesn't subscribe/react properly. This mod definitely doesn't handle the locations being checked mid-game by anyone other than the active HK player, but I haven't dug enough to know if there is something that the AP client is exposing that just isn't being plugged into. It's starting to sound like its own ticket though, so I shall file that in a moment. (edit: filed as #222)

But if this is the case then I would tend to agree that the game should react immediately, and should probably not show the items either in the corner or recent items. Don't feel like it's worth it to be a toggle

I both do and don't agree. I don't feel that we need to list every single item Player B collected, but it might be nice to show something to signal the player that things have changed. Maybe we could display a single message saying something like "Player B collected their remaining items"? That would keep it nice and short (only one entry in both the corner and RecentItems) while still letting the HK player know they should re-check their plan because the map state has changed.

thislooksfun commented 4 months ago

Alright, I made it hide the collected items during load for now. That behavior can be revisited in #222 if desired. I made the change via a fixup commit for easier re-reviewing, and I will squash it down when its ready. Or I can do it now if you'd prefer.

BadMagic100 commented 2 months ago

This will be affected by https://github.com/ArchipelagoMW-HollowKnight/Archipelago.HollowKnight/commit/760eb051113330eeeeec2098fe3de09c1971a32b which moves to use separate local and global settings classes with connection details as a member. So this setting would move out of connection details and onto global settings

BadMagic100 commented 1 month ago

Another thought on this, rather than a true/false on/off, we could have a "progression only" mode which only queues items classified as progression to the corner display

thislooksfun commented 1 month ago

Ooh, that's not a bad idea. Should help cut down on some of the geo rock spam.

Sorry for totally dropping the ball on this PR btw, my motivation for working on this just vanished and I haven't managed to find it again yet.