Add an option for always showing items received while offline #221
Conversation
| @@ -153,7 +153,9 @@ private static void StartOrResumeGame(bool newGame, MenuLabel errorLabel) | |||
| } | |||
| else | |||
| { | |||
| Archipelago.Instance.MenuSettings = Archipelago.Instance.ApSettings with { }; | |||
| Archipelago.Instance.MenuSettings = Archipelago.Instance.ApSettings with { | |||
| AlwaysShowItems = Archipelago.Instance.MenuSettings.AlwaysShowItems | |||
There was a problem hiding this comment.
I don't think this should be needed
There was a problem hiding this comment.
Unfortunately it is. Without that change the save-specific ApSettings will override the MenuSettings whenever a save file is loaded, which causes the AlwaysShowItems setting to be overridden. If you have another solution I'm happy to hear it, but this was the simplest fix I was able to find.
There was a problem hiding this comment.
Oh yuck didn't think of that. Settings really need to be refactored but there is just not a way to do it that doesn't break anyone with an existing install
There was a problem hiding this comment.
I'm sure it's possible to do while still being backwards-compatible, but it was a bigger change than I wanted to make in this PR, so I opted for the simpler solution. I'm happy to take a crack at that refactor if you'd prefer it done that way though.
|
This approach seems fine, was any testing done with this change? |
thislooksfun
force-pushed
the
option-always-show
branch
from
38c2934 to
6da00c0
Compare
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. |
|
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 |
thislooksfun
force-pushed
the
option-always-show
branch
from
6da00c0 to
70245c8
Compare
|
^ rebased onto main, and marked as draft until the collect issue is resolved. |
|
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 |
Correct.
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)
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. |
|
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. |
| // FROM this world, which can SERIOUSLY lock up the item display if someone collects a | ||
| // lot of their items at once (usually via the !collect command). It also prevents these | ||
| // items from displaying in RecentItemsDisplay (if installed) for the same reason. | ||
| MarkLocationAsChecked(location, true, true); |
There was a problem hiding this comment.
Won't this also hide your own received items from recent items? That seems wrong.
|
This will be affected by 760eb05 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 |
|
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 |
|
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. |
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.