Skip to content
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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

aspiringLich
Copy link
Contributor

@aspiringLich aspiringLich commented Jun 11, 2024

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 if ItemSlotsComponent 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

  • Removed the optional popup argument to CanInsert so the CanInsert method doesn't generate a popup (this was stupid to begin with)
  • Changed the logic of OnInteractUsing so it triggers the popups instead, and also triggers them correctly

Media

2024-06-11.09-54-23.mp4

Breaking changes

Changelog

🆑

  • fix: Fixed the logic triggering popups when inserting items into machines.

@aspiringLich
Copy link
Contributor Author

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.

@aspiringLich aspiringLich reopened this Jun 11, 2024
@Partmedia Partmedia added the PR: Bug Fix Fixes a clear bug, no refactoring or features label Jun 13, 2024
Comment on lines 279 to 294

public enum CannotInsertReason
{
ContainerSlotNull,
SlotFullNoSwap,
ItemNotWhitelisted,
ItemBlacklisted,
SlotLocked,
EventCanceled,

/**
* calling _containers.CanInsert returned false, i dunno why you figure it out
*/
CanInsertFalse
}

Copy link
Contributor

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.

@metalgearsloth metalgearsloth added the Status: Awaiting Changes This PR needs its reviews addressed or changes to be made in order to be merged. label Jun 27, 2024
Signed-off-by: Brandon Li <sirbrandonthenerd@gmail.com>
@aspiringLich
Copy link
Contributor Author

I realised I am stupid and you don't need reason at all. I overcomplicated it way too much.

Events are also overkill.

@aspiringLich
Copy link
Contributor Author

Oh also the other changes in the commit is because I formatted the file.

@github-actions github-actions bot added Status: Needs Review This PR requires new reviews before it can be merged. and removed Status: Awaiting Changes This PR needs its reviews addressed or changes to be made in order to be merged. labels Jun 29, 2024
@Plykiya Plykiya self-requested a review July 22, 2024 05:09
@Plykiya
Copy link
Contributor

Plykiya commented Jul 28, 2024

Added an overload method for CanInsert that doesnt have out var reason because optional out vars are not a thing in C# for some reason.

if you don't need the output variable you can just do out _

@aspiringLich
Copy link
Contributor Author

I think I got rid of the overloaded method. Found a less turbo stupid way to do it.

@aspiringLich
Copy link
Contributor Author

Updated the technical details comment

Comment on lines 243 to 246
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);
Copy link
Member

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?

Copy link
Contributor Author

@aspiringLich aspiringLich Aug 9, 2024

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 there are no slots that allow the item,
    • if lastValidLockedSlot is set, display the lockedFailPopup
    • otherwise, display the whitelistFailPopup
  • 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.

@aspiringLich aspiringLich changed the title Fix ItemSlotSystem popups being broken Fix ItemSlotSystem popup Logic Aug 9, 2024
@ShadowCommander ShadowCommander self-assigned this Oct 21, 2024
@ShadowCommander ShadowCommander added the PR: Approved Indicates that a PR has been approved by a maintainer and it may be merged soon by them. label Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Approved Indicates that a PR has been approved by a maintainer and it may be merged soon by them. PR: Bug Fix Fixes a clear bug, no refactoring or features Status: Needs Review This PR requires new reviews before it can be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants