Slimefun / Slimefun4

Slimefun 4 - A unique Spigot/Paper plugin that looks and feels like a modpack. We've been giving you backpacks, jetpacks, reactors and much more since 2013.
GNU General Public License v3.0
943 stars 536 forks source link

Error stack and some incorrect use of variables in the code #4211

Closed mcchampions closed 3 weeks ago

mcchampions commented 3 weeks ago

❗ Checklist

📍 Description

from SlimefunGuguProject/Slimefun4#912,This fork does not change the code related to this issue. The following is machine translation: Original content:For certain items, slimefun will directly make them stack, even if they have different slimefun IDs

Specific items: Same name, same lore, but different slimefun ID (i.e. NBT exists differently, you don't need to consider the full NBT to fix the bug, just consider the slimefun ID)

Problem tracing https://github.com/Slimefun/Slimefun4/blob/226af9be61d1bfc582cbe235909579b9f4f605cd/src/main/java/me/mrCookieSlime/Slimefun/api/inventory/DirtyChestMenu.java#L178-L183 https://github.com/Slimefun/Slimefun4/blob/226af9be61d1bfc582cbe235909579b9f4f605cd/src/main/java/io/github/thebusybiscuit/slimefun4/utils/SlimefunUtils.java#L306-L322 https://github.com/Slimefun/Slimefun4/blob/226af9be61d1bfc582cbe235909579b9f4f605cd/src/main/java/io/github/thebusybiscuit/slimefun4/utils/SlimefunUtils.java#L348-L387 Herein lies the problem——`equalsItemMeta`method https://github.com/Slimefun/Slimefun4/blob/226af9be61d1bfc582cbe235909579b9f4f605cd/src/main/java/io/github/thebusybiscuit/slimefun4/utils/SlimefunUtils.java#L459-L501 **Here's a thing to say about the `checkCustomModelData` variable in this method called `bypassCustomModelCheck`, which actually ignores checking, so it should be reversed when it is passed in, but it is not reversed when it is passed in** This code checks Lore & Name,The problem arises https://github.com/Slimefun/Slimefun4/blob/226af9be61d1bfc582cbe235909579b9f4f605cd/src/main/java/io/github/thebusybiscuit/slimefun4/utils/SlimefunUtils.java#L364-L386 Check here to see if you have overlooked a situation where it is a slimefun item and the two items are not the same slimefun item https://github.com/Slimefun/Slimefun4/blob/226af9be61d1bfc582cbe235909579b9f4f605cd/src/main/java/io/github/thebusybiscuit/slimefun4/utils/SlimefunUtils.java#L358-L378 The variable name is `sfItem`, which means that `sfItem` is a slimefun item by default. In the case of `id==null` (i.e. the item is not a Slimefun item), it is't necessary to continue to determine if it is necessary to return false if one is a Slimefun item and the other is not. Also, since `sfitem.hasItemMeta()` has been judged Why should the variable of this ItemMeta be named `possibleSfItemMeta`, and the same goes for `possibleItemId` In a word,maybe change like: ```java if (id != null && id.equals(possibleItemId)) { Debug.log(TestCase.CARGO_INPUT_TESTING, " Item IDs matched!"); /* * PR #3417 * * Some items can't rely on just IDs matching and will implement Distinctive Item * in which case we want to use the method provided to compare */ Optional optionalDistinctive = getDistinctiveItem(id); if (optionalDistinctive.isPresent()) { return optionalDistinctive.get().canStack(possibleSfItemMeta, itemMeta); } return true; } return false; ```

In a word,there is a wrong check and some incorrect use of variables(or wrong name?)

📑 Reproduction Steps

Put different slimefun items with the same lore and name into the charging bench And then they stacks.

💡 Expected Behavior

they shouldn't stack

📷 Screenshots / Videos

https://github.com/Slimefun/Slimefun4/assets/95963344/2545a8bb-da13-4155-ae34-713b4e28a149

📜 Server Log

No response

📂 /error-reports/ folder

No response

💻 Server Software

Paper

🎮 Minecraft Version

1.20.x

⭐ Slimefun version

latest

🧭 Other plugins

No response

Boomer-1 commented 3 weeks ago

charging bench is a duplicate issue. also please report issues on seperate report. also we aren't responsible for any forks of slimefun.

mcchampions commented 3 weeks ago

charging bench is a duplicate issue. also please report issues on seperate report. also we aren't responsible for any forks of slimefun.

No,Actually, I'm talking about the problem with the charging bench , but I found a lot of errors in this logic in the process.and the problem was found on this branch, but the branch doesn't change the code, so it's safe to assume that the problem works on the main branch as well(In fact, it does)

Boomer-1 commented 3 weeks ago

again, it's a duplicate issue. it's already been reported, and it's never safe to assume a fork doesn't affect code.

mcchampions commented 3 weeks ago

again, it's a duplicate issue. it's already been reported, and it's never safe to assume a fork doesn't affect code.

Yes,It's reported in other fork,but it isn't reported in here.And,in fact,is does't affect code,this question exists

mcchampions commented 3 weeks ago

again, it's a duplicate issue. it's already been reported, and it's never safe to assume a fork doesn't affect code.

Yes,It's reported in other fork,but it isn't reported in here.And,in fact,is does't affect code,this question exists

In fact,this issue is not quite the same as that one

Boomer-1 commented 3 weeks ago

it is reported here. #4183

mcchampions commented 3 weeks ago

it is reported here. #4183

Emm.no same

mcchampions commented 3 weeks ago

it is reported here. #4183

Emm.no same

Boomer-1 commented 3 weeks ago

you said you have items stacking in the charging bench on the official version. that was already reported in 4183. this is closed. move on

JustAHuman-xD commented 3 weeks ago

Boomer he is meaning different issues, it sounds like some other menus/methods combine slimefun items that shouldn't combine like how cargo does

balugaq commented 3 weeks ago

It's completely different, and the charging bench is just a convenient demonstration machine, not just the charging bench with this issue.

Boomer-1 commented 3 weeks ago

then as i said, open a seperate issue

Boomer-1 commented 3 weeks ago

this doesn't have anything to do with cargo

mcchampions commented 3 weeks ago

then as i said, open a seperate issue

If you read it carefully, you will see that this is not the same.

Boomer-1 commented 3 weeks ago

if the cause of the other issue is what you identified then it's the same. open a seperate issue not related to the charging bench

mcchampions commented 3 weeks ago

if the cause of the other issue is what you identified then it's the same. open a seperate issue not related to the charging bench

That's not the cause of the problem, it's talking about items that can't be stacked but stacked,but i will open a new issue

Boomer-1 commented 3 weeks ago

there's already an open pr to fix it

mcchampions commented 3 weeks ago

已经有一个开放的 PR 来修复它

Not really

mcchampions commented 3 weeks ago

已经有一个开放的 PR 来修复它

This issue should have just been discovered by now

balugaq commented 3 weeks ago

That PR maybe just fix the charging bench. But it is SlimefunUtils's wrong in fact.

Boomer-1 commented 3 weeks ago

as part of the review process the dev's can look into it to see if the root cause is addressed in the PR or if other changes need to be made to look at #4212

mcchampions commented 3 weeks ago

I am in China and it is 0 o'clock. I originally opened this issue for convenience, but it seems inconvenient. Also, this issue comes with a detailed solution and traceability process