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

DistinctiveItem/isSimilar + Backpack canStack #3384

Closed
wants to merge 5 commits into from

Conversation

Sefiraat
Copy link
Member

@Sefiraat Sefiraat commented Dec 22, 2021

Description

SlimefunItemStacks that share an ID are currently automatically stacked when running through cargo. Items that vary by lore or another meta aspect are being 'merged' with items that happen to match on ID but are, otherwise, distinct. See issue #3379. This likely only effects backpacks within core Slimefun but also effects any similar SlimefunItems using similar distinctions - DankPacks and Tinkers parts (I'm so self centred!) being an example.

Proposed changes

New ItemAttribute called DistinctiveItem to act as an identifier for items that match this criterion.
Distinctive item has a method that allows an implementation to say how two same-ID items can be distinguished.
SlimefunBackpack has been amended to implement this interface.
SlimefunUtils#isItemSimilar now checks if the stack's SlimefunItem is a DistinctiveItem and, if so, return the result of the canStack method.

Discussion with Walshy shows this is making an extra call to get the ID from the Meta that was already made in the previous getItemData that may mean this PR needs more work.

Related Issues (if applicable)

Resolves #3379

Checklist

  • I have fully tested the proposed changes and promise that they will not break everything into chaos.
  • I have also tested the proposed changes in combination with various popular addons and can confirm my changes do not break them.
  • I followed the existing code standards and didn't mess up the formatting.
  • I did my best to add documentation to any public classes or methods I added.
  • I have added Nonnull and Nullable annotations to my methods to indicate their behaviour for null values
  • I added sufficient Unit Tests to cover my code.

@Sefiraat Sefiraat added the ✨ Fix This Pull Request fixes an issue. label Dec 22, 2021
@Sefiraat Sefiraat requested a review from a team as a code owner December 22, 2021 20:10
Copy link
Contributor

@char3210 char3210 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make sure to check for this in line 280 as well, and it should store the results of getItemData (should not call hasEqualItemData)


/**
* Implement this interface for any {@link SlimefunItem} to prevent
* cargo using only it's ID when comparing. #isSame is used when
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be its

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, quite right - thanks 👍

@Sefiraat Sefiraat marked this pull request as draft December 22, 2021 22:43
@Sefiraat
Copy link
Member Author

make sure to check for this in line 280 as well, and it should store the results of getItemData (should not call hasEqualItemData)

Line 280 is under discussion, and as the PR states, getItemData is in question also. On that note I should have made this draft

@Sefiraat
Copy link
Member Author

So, near as I can tell, the 5th branch (line 280) is never called by items moving through cargo though it will be reached when directly calling isItemSimilar. Given that meta is required to check distinction this will could add a lot of overhead to any other machine using this. Given the above, do we still want to check distinction outside of cargo - given that we do not currently and it, in itself, hasn't shown issues. Input welcome for how to add this in.

Also - changes in commit 92b45ad tested under the same conditions with expected and wanted results.

@char3210
Copy link
Contributor

if that's the case then i would say it's fine as is

@Sefiraat
Copy link
Member Author

See #3417

@Sefiraat Sefiraat closed this Jan 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ Fix This Pull Request fixes an issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cargo ignores lore of slimefun items, causing backpacks to stack.
2 participants