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

check for meta even if ids match #3383

Closed
wants to merge 1 commit into from
Closed

Conversation

char3210
Copy link
Contributor

@char3210 char3210 commented Dec 22, 2021

Description

this prevents backpacks from being marked as similar even if they have different ids

Proposed changes

check for item meta once the slimefun ids are matched

Related Issues (if applicable)

related: #3379
do note that this only fixes backpacks since the id is stored in the lore, but dankpacks still stacks as their id is stored in nbt

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.

@char3210 char3210 requested a review from a team as a code owner December 22, 2021 01:23
@WalshyDev
Copy link
Member

WalshyDev commented Dec 22, 2021

Is there ever a scenario where display name matters? getDisplayName is a very intensive call and only getLore seems to matter here. I feel like this is the wrong approach and just ruins the whole performance improvement for no reason

Alongside that, we should only run this when needed. Not just for all items (again, just ruins the entire performance improvement)


I think the best approach is:

  • If SlimefunItem has attribute like CompareLore (obviously a better name)
  • Compare ID + lore
  • Else
  • Compare ID

@WalshyDev WalshyDev closed this Dec 22, 2021
@Sefiraat
Copy link
Member

Sefiraat commented Dec 22, 2021

Also I assigned the issue to myself as I have started work on this already. Please refrain from picking up work where people are assigned. You just end up wasting either your time or mine which isn’t very nice in the long run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants