-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix ItemSlotSystem
popup Logic
#28856
base: master
Are you sure you want to change the base?
Fix ItemSlotSystem
popup Logic
#28856
Conversation
I updated from master and I thought I saw that someone already fixed it. Nope it was just the popup message for a successful insertion which has always been there. |
|
||
public enum CannotInsertReason | ||
{ | ||
ContainerSlotNull, | ||
SlotFullNoSwap, | ||
ItemNotWhitelisted, | ||
ItemBlacklisted, | ||
SlotLocked, | ||
EventCanceled, | ||
|
||
/** | ||
* calling _containers.CanInsert returned false, i dunno why you figure it out | ||
*/ | ||
CanInsertFalse | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having a big enum hardcoded like this defeats the purpose of the eventbus; I haven't had a chance to read the rest yet but this needs to go.
Signed-off-by: Brandon Li <sirbrandonthenerd@gmail.com>
I realised I am stupid and you don't need Events are also overkill. |
Oh also the other changes in the commit is because I formatted the file. |
if you don't need the output variable you can just do |
I think I got rid of the overloaded method. Found a less turbo stupid way to do it. |
Updated the technical details comment |
if (lastFailedSlot.WhitelistFailPopup != null) | ||
_popupSystem.PopupClient(Loc.GetString(lastFailedSlot.WhitelistFailPopup), uid, args.User); | ||
else if (allLocked && lastFailedSlot.LockedFailPopup != null) | ||
_popupSystem.PopupClient(Loc.GetString(lastFailedSlot.LockedFailPopup), uid, args.User); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this properly show the WhitelistFailPopup if not all slots are locked but the last slot is not whitelisted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, I just realized you'd want to show the locked popup if there's a slot that the item could go in, but the slot is locked.
OKAY, so the logic goes like this:
lastValidLockedSlot
stores, if applicable, the last slot that should technically allow the item but is locked- go through every single valid interactable slot
- if you can't insert the item,
- if the item is allowed by the whitelist / blacklist, but the slot is locked, set
lastValidLockedSlot
- if the item is allowed by the whitelist / blacklist, but the slot is locked, set
- if there are no slots that allow the item,
- if
lastValidLockedSlot
is set, display thelockedFailPopup
- otherwise, display the
whitelistFailPopup
- if
- otherwise, if there are valid slots
- insert the item into the valid slot as normal
If none of the slots would have let the item in, it will display the whitelistFailPopup
If there was at least slot that (maybe) would have let it in if it wasnt locked, and there are no other valid slots, display lockedFailPopup
.
I think it's correct now. I can't test it in game because, as far as I can tell, the only machine that takes advantage of the lockedFailPopup
is the AME.
I did test it again on the reagent dispenser, and it seems to work so.
ItemSlotSystem
popups being brokenItemSlotSystem
popup Logic
About the PR
As far as I know, nobody has reported this, which is kinda surprising. I only got around to fixing it because of #28410, which I think got partially fixed and then wasn't closed?
Anyway, basically anything using
ItemSlotSystem
to handle insertion / removal of items, notably, chem dispensers and chem masters (don't know about this hapenning to anything else), were bugged, so they would constantly spit out "you can't put that in here!" popups whenever you inserted anything. This bug only happens ifItemSlotsComponent
contains slots that have different whitelists.This is because the popups were triggered at
CanInsert
per-slot, so if it failed the whitelist for even a single slot it would display the popup, even if the item could be inserted.Technical details
popup
argument toCanInsert
so theCanInsert
method doesn't generate a popup (this was stupid to begin with)OnInteractUsing
so it triggers the popups instead, and also triggers them correctlyMedia
2024-06-11.09-54-23.mp4
Breaking changes
Changelog
🆑