Error stack and some incorrect use of variables in the code

[mcchampions]

❗ Checklist

• I am using the official english version of Slimefun and did not modify the jar.
• I am using an up to date "DEV" (not "RC") version of Slimefun.
• I am aware that issues related to Slimefun addons need to be reported on their bug trackers and not here.
• I searched for similar open issues and could not find an existing bug report on this.

📍 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

Slimefun4/src/main/java/me/mrCookieSlime/Slimefun/api/inventory/DirtyChestMenu.java

Lines 178 to 183 in 226af9b

	if (SlimefunItem.getByItem(item) != null) {
	// Patch: use sf item check
	if (!SlimefunUtils.isItemSimilar(stack, wrapper, true, false)) {
	continue;
	}
	} else {

Slimefun4/src/main/java/io/github/thebusybiscuit/slimefun4/utils/SlimefunUtils.java

Lines 306 to 322 in 226af9b

	} else if (checkDistinctiveItem
	&& sfitem instanceof SlimefunItemStack stackOne
	&& item instanceof SlimefunItemStack stackTwo) {
	if (stackOne.getItemId().equals(stackTwo.getItemId())) {
	/*
	* 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
	*/
	if (stackOne instanceof DistinctiveItem && stackTwo instanceof DistinctiveItem distinctiveItem) {
	return distinctiveItem.canStack(stackOne.getItemMeta(), stackTwo.getItemMeta());
	}
	return true;
	}
	return false;
	} else if (item.hasItemMeta()) {

Slimefun4/src/main/java/io/github/thebusybiscuit/slimefun4/utils/SlimefunUtils.java

Lines 348 to 387 in 226af9b

	} else if (sfitem instanceof ItemStackWrapper && sfitem.hasItemMeta()) {
	Debug.log(TestCase.CARGO_INPUT_TESTING, " is wrapper");
	/*
	* Cargo optimization (PR #3258)
	*
	* Slimefun items may be ItemStackWrapper's in the context of cargo
	* so let's try to do an ID comparison before meta comparison
	*/
	Debug.log(TestCase.CARGO_INPUT_TESTING, " sfitem is ItemStackWrapper - possible SF Item: {}", sfitem);
	ItemMeta possibleSfItemMeta = sfitem.getItemMeta();
	String id = Slimefun.getItemDataService().getItemData(itemMeta).orElse(null);
	String possibleItemId = Slimefun.getItemDataService()
	.getItemData(possibleSfItemMeta)
	.orElse(null);
	// Prioritize SlimefunItem id comparison over ItemMeta comparison
	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<DistinctiveItem> optionalDistinctive = getDistinctiveItem(id);
	if (optionalDistinctive.isPresent()) {
	return optionalDistinctive.get().canStack(possibleSfItemMeta, itemMeta);
	}
	return true;
	} else {
	Debug.log(
	TestCase.CARGO_INPUT_TESTING,
	" Item IDs don't match, checking meta {} == {} (lore: {})",
	itemMeta,
	possibleSfItemMeta,
	checkLore);
	return equalsItemMeta(itemMeta, possibleSfItemMeta, checkLore, checkCustomModelData);
	}
	} else if (sfitem.hasItemMeta()) {

Herein lies the problem——equalsItemMetamethod

Slimefun4/src/main/java/io/github/thebusybiscuit/slimefun4/utils/SlimefunUtils.java

Lines 459 to 501 in 226af9b

	private static boolean equalsItemMeta(
	@Nonnull ItemMeta itemMeta,
	@Nonnull ItemMeta sfitemMeta,
	boolean checkLore,
	boolean bypassCustomModelCheck) {
	if (itemMeta.hasDisplayName() != sfitemMeta.hasDisplayName()) {
	return false;
	} else if (itemMeta.hasDisplayName()
	&& sfitemMeta.hasDisplayName()
	&& !itemMeta.getDisplayName().equals(sfitemMeta.getDisplayName())) {
	return false;
	} else if (checkLore) {
	boolean hasItemMetaLore = itemMeta.hasLore();
	boolean hasSfItemMetaLore = sfitemMeta.hasLore();
	if (hasItemMetaLore && hasSfItemMetaLore) {
	if (!equalsLore(itemMeta.getLore(), sfitemMeta.getLore())) {
	return false;
	}
	} else if (hasItemMetaLore != hasSfItemMetaLore) {
	return false;
	}
	}
	if (!bypassCustomModelCheck) {
	// Fixes #3133: name and lore are not enough
	boolean hasItemMetaCustomModelData = itemMeta.hasCustomModelData();
	boolean hasSfItemMetaCustomModelData = sfitemMeta.hasCustomModelData();
	if (hasItemMetaCustomModelData
	&& hasSfItemMetaCustomModelData
	&& itemMeta.getCustomModelData() != sfitemMeta.getCustomModelData()) {
	return false;
	} else if (hasItemMetaCustomModelData != hasSfItemMetaCustomModelData) {
	return false;
	}
	}
	if (itemMeta instanceof PotionMeta && sfitemMeta instanceof PotionMeta) {
	return ((PotionMeta) itemMeta).getBasePotionData().equals(((PotionMeta) sfitemMeta).getBasePotionData());
	}
	return true;
	}

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

Slimefun4/src/main/java/io/github/thebusybiscuit/slimefun4/utils/SlimefunUtils.java

Lines 364 to 386 in 226af9b

	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<DistinctiveItem> optionalDistinctive = getDistinctiveItem(id);
	if (optionalDistinctive.isPresent()) {
	return optionalDistinctive.get().canStack(possibleSfItemMeta, itemMeta);
	}
	return true;
	} else {
	Debug.log(
	TestCase.CARGO_INPUT_TESTING,
	" Item IDs don't match, checking meta {} == {} (lore: {})",
	itemMeta,
	possibleSfItemMeta,
	checkLore);
	return equalsItemMeta(itemMeta, possibleSfItemMeta, checkLore, checkCustomModelData);
	}

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

Slimefun4/src/main/java/io/github/thebusybiscuit/slimefun4/utils/SlimefunUtils.java

Lines 358 to 378 in 226af9b

	ItemMeta possibleSfItemMeta = sfitem.getItemMeta();
	String id = Slimefun.getItemDataService().getItemData(itemMeta).orElse(null);
	String possibleItemId = Slimefun.getItemDataService()
	.getItemData(possibleSfItemMeta)
	.orElse(null);
	// Prioritize SlimefunItem id comparison over ItemMeta comparison
	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<DistinctiveItem> optionalDistinctive = getDistinctiveItem(id);
	if (optionalDistinctive.isPresent()) {
	return optionalDistinctive.get().canStack(possibleSfItemMeta, itemMeta);
	}
	return true;
	} else {

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:

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<DistinctiveItem> 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

343782159-dc0ad5a8-745b-4829-8c6e-148c2552e470.mp4

📜 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]

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

[mcchampions]

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]

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]

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]

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]

it is reported here. #4183

[mcchampions]

it is reported here. #4183

Emm.no same

[mcchampions]

it is reported here. #4183

Emm.no same

[Boomer-1]

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]

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]

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

[Boomer-1]

then as i said, open a seperate issue

[Boomer-1]

this doesn't have anything to do with cargo

[mcchampions]

then as i said, open a seperate issue

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

[Boomer-1]

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]

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]

there's already an open pr to fix it

[mcchampions]

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

Not really

[mcchampions]

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

This issue should have just been discovered by now

[balugaq]

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

[Boomer-1]

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]

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