replace isitemsimilar #4120

J3fftw1 · 2024-02-05T18:39:45Z

Description

This PR replaces isItemSImilar with a new method for now called compareItem()

Proposed changes

Replace isItemSImilar with a better check

Related Issues (if applicable)

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 have made sure that the proposed changes do not break compatibility across the supported Minecraft versions (1.16.* - 1.20.*).
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.

ToDo

add tests for all lines of the new code
Add comments
performance test

github-actions · 2024-02-05T18:40:01Z

Your Pull Request was automatically labelled as: "🧹 Chores"
Thank you for contributing to this project! ❤️

github-actions · 2024-02-05T19:15:20Z

Slimefun preview build

A Slimefun preview build is available for testing!
Commit: 9f01d73f

https://preview-builds.walshy.dev/download/Slimefun/4120/9f01d73f

Note: This is not a supported build and is only here for the purposes of testing.
Do not run this on a live server and do not report bugs anywhere but this PR!

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

WalshyDev · 2024-02-06T23:09:05Z

Testing required

This will need just general Slimefun testing. Some areas to specifically cover:

Can use a variety of items
Can craft items in enhanced crafting table
Can build multi block
Can craft with ancient alter
Backpacks still work
Cargo can move items as normal

Co-authored-by: Alessio Colombo <[email protected]>

WalshyDev · 2024-02-20T00:58:02Z

Boomer:

I even added teleporting, basic dust farming, tested a few nodes going thru cargo, and everything seems to work as expected. backpacks are fine. no issues on ancient altar. all i tested with though was core slimefun. no addons or outside plugins. Added multiple power generators, geo mining, teleporting with the gps pad, and everything seems to be running as expected.

the environment was 1.20.4, paper version 405. probably need to update that but, eh for now

ybw0014

the type check is the third check,

ybw0014 · 2024-02-20T05:06:28Z

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

+
+        // If neither have meta, compare the types and return
+        if (!itemOneHasMeta && !itemTwoHasMeta) {
+            return itemOne.getType() == itemTwo.getType();


why do a type check again here?

ybw0014 · 2024-02-20T05:07:01Z

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

+        ItemMeta itemMetaTwo = itemTwo.getItemMeta();
+        // If both metas are null, compare the types and return
+        if (itemMetaOne == null && itemMetaTwo == null) {
+            return itemOne.getType() == itemTwo.getType();


ybw0014 · 2024-02-20T05:07:29Z

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

+        // -- Vanilla items --
+        // This should only be vanilla items now
+        // Compare types and metas
+        return itemOne.getType() == itemTwo.getType()


TheBusyBiscuit

Good optimization but in its current form this will break a lot of machines because it is missing a check for the amount and special handling for some items.

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

TheBusyBiscuit · 2024-02-23T10:11:57Z

src/main/java/io/github/thebusybiscuit/slimefun4/core/networks/cargo/ItemFilter.java

@@ -200,7 +200,7 @@ public boolean test(@Nonnull ItemStack item) {
             * and thus only perform .getItemMeta() once
             */
            for (ItemStackWrapper stack : items) {
-                if (SlimefunUtils.isItemSimilar(subject, stack, checkLore, false)) {
+                if (SlimefunUtils.compareItem(subject, stack)) {


The "checkLore" option is ignored here

We already check lore in the method. We don’t specifically need to check for it with a boolean

The option exists in the gui yet it has no effect on the check.

TheBusyBiscuit · 2024-02-23T10:13:37Z

...t/slimefun4/implementation/items/electric/machines/accelerators/AnimalGrowthAccelerator.java

@@ -40,7 +40,7 @@ protected void tick(Block b) {

        for (Entity n : b.getWorld().getNearbyEntities(b.getLocation(), RADIUS, RADIUS, RADIUS, this::isReadyToGrow)) {
            for (int slot : getInputSlots()) {
-                if (SlimefunUtils.isItemSimilar(inv.getItemInSlot(slot), organicFood, false, false)) {
+                if (SlimefunUtils.compareItem(inv.getItemInSlot(slot), organicFood)) {


This compares with an unobtainable blank organic food item.
So this would break this machine.

TheBusyBiscuit · 2024-02-23T10:13:45Z

...uit/slimefun4/implementation/items/electric/machines/accelerators/CropGrowthAccelerator.java

@@ -58,7 +58,7 @@ private boolean grow(Block machine, BlockMenu inv, Block crop) {

        if (ageable.getAge() < ageable.getMaximumAge()) {
            for (int slot : getInputSlots()) {
-                if (SlimefunUtils.isItemSimilar(inv.getItemInSlot(slot), organicFertilizer, false, false)) {
+                if (SlimefunUtils.compareItem(inv.getItemInSlot(slot), organicFertilizer)) {


This compares with an unobtainable blank organic fertilizer item.
So this would break this machine.

TheBusyBiscuit · 2024-02-23T10:13:57Z

...uit/slimefun4/implementation/items/electric/machines/accelerators/TreeGrowthAccelerator.java

@@ -120,7 +120,7 @@ private boolean updateSaplingData(Block machine, Block block, BlockMenu inv, Sap
    }

    protected boolean isFertilizer(@Nullable ItemStack item) {
-        return SlimefunUtils.isItemSimilar(item, organicFertilizer, false, false);
+        return SlimefunUtils.compareItem(item, organicFertilizer);


This compares with an unobtainable blank organic fertilizer item.
So this would break this machine.

TheBusyBiscuit · 2024-02-23T10:14:01Z

...ub/thebusybiscuit/slimefun4/implementation/items/electric/machines/entities/AutoBreeder.java

@@ -111,7 +111,7 @@ protected void tick(Block b) {

        for (Entity n : b.getWorld().getNearbyEntities(b.getLocation(), 4.0, 2.0, 4.0, this::canBreed)) {
            for (int slot : getInputSlots()) {
-                if (SlimefunUtils.isItemSimilar(inv.getItemInSlot(slot), organicFood, false)) {
+                if (SlimefunUtils.compareItem(inv.getItemInSlot(slot), organicFood)) {


This compares with an unobtainable blank organic food item.
So this would break this machine.

...main/java/io/github/thebusybiscuit/slimefun4/implementation/items/multiblocks/OreWasher.java

src/main/java/io/github/thebusybiscuit/slimefun4/implementation/listeners/BlockListener.java

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

ybw0014 · 2024-02-24T22:19:37Z

Type check is the first, so items with changed item type (e.g. explosive pickaxe is diamond pickaxe, and using the improvement forge from FoxyMachines to upgrade it to netherite) will not match.
maybe we can have some sort of whitelist for tools and weapons. and other type mismatches will return immediately.

WalshyDev · 2024-02-25T15:06:27Z

An allowlist just leads to unmaintable again annoyingly

I'd say just do this when we know it isn't an Slimefun item but that will slow down a bunch...

github-actions bot added the 🧹 Chores Refactoring / Cleanup. label Feb 5, 2024

J3fftw1 marked this pull request as ready for review February 5, 2024 19:14

J3fftw1 requested a review from a team as a code owner February 5, 2024 19:14

J3fftw1 added the 🔧 API This Pull Request introduces API changes. label Feb 5, 2024

JustAHuman-xD reviewed Feb 5, 2024

View reviewed changes

src/main/java/io/github/thebusybiscuit/slimefun4/utils/SlimefunUtils.java Outdated Show resolved Hide resolved

WalshyDev reviewed Feb 5, 2024

View reviewed changes

src/main/java/io/github/thebusybiscuit/slimefun4/utils/SlimefunUtils.java Outdated Show resolved Hide resolved

WalshyDev added the 🎯 Needs testing This Issue needs to be tested by our team to see if it can be reproduced. label Feb 6, 2024

J3fftw1 requested a review from a team as a code owner February 17, 2024 18:26

J3fftw1 and others added 5 commits February 19, 2024 02:20

replace isitemsimilar

09fdb87

not sure what i did

f91ee0e

fix tests

2aa1f0c

Fixes exhaustion when loading large profiles (Slimefun#4127)

c013fad

Add new analytics service (Slimefun#4067)

5fbef63

Co-authored-by: Alessio Colombo <[email protected]>

WalshyDev force-pushed the chore/replace-isitemsimilar branch from f0da19c to f4e7ac1 Compare February 19, 2024 02:22

More work and tests

cbb40ef

WalshyDev force-pushed the chore/replace-isitemsimilar branch from f4e7ac1 to cbb40ef Compare February 19, 2024 02:24

J3fftw1 mentioned this pull request Feb 19, 2024

Optimize SlimefunUtils#isItemSimilar / Optimize SlimefunUtils#equalsItemMeta #4135

Closed

7 tasks

quick exist for item typs

cbb1f25

J3fftw1 added the 💡 Performance Optimization This pull request improves performance. label Feb 19, 2024

WalshyDev added Build tested Used to indicate the PR preview build has been tested by one of the team and removed 🎯 Needs testing This Issue needs to be tested by our team to see if it can be reproduced. labels Feb 20, 2024

ybw0014 suggested changes Feb 20, 2024

View reviewed changes

TheBusyBiscuit reviewed Feb 23, 2024

View reviewed changes

J3fftw1 added 2 commits February 24, 2024 20:34

check for ammount

8542e26

requested changes

22e0773

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

replace isitemsimilar #4120

replace isitemsimilar #4120

J3fftw1 commented Feb 5, 2024 •

edited by WalshyDev

Loading

github-actions bot commented Feb 5, 2024

github-actions bot commented Feb 5, 2024 •

edited

Loading

WalshyDev commented Feb 6, 2024

WalshyDev commented Feb 20, 2024

ybw0014 left a comment

ybw0014 Feb 20, 2024

ybw0014 Feb 20, 2024

ybw0014 Feb 20, 2024

TheBusyBiscuit left a comment

TheBusyBiscuit Feb 23, 2024

J3fftw1 Feb 23, 2024

TheBusyBiscuit Feb 23, 2024

TheBusyBiscuit Feb 23, 2024

TheBusyBiscuit Feb 23, 2024

TheBusyBiscuit Feb 23, 2024

TheBusyBiscuit Feb 23, 2024

ybw0014 commented Feb 24, 2024

WalshyDev commented Feb 25, 2024 •

edited

Loading

replace isitemsimilar #4120

Are you sure you want to change the base?

replace isitemsimilar #4120

Conversation

J3fftw1 commented Feb 5, 2024 • edited by WalshyDev Loading

Description

Proposed changes

Related Issues (if applicable)

Checklist

ToDo

github-actions bot commented Feb 5, 2024

github-actions bot commented Feb 5, 2024 • edited Loading

Slimefun preview build

WalshyDev commented Feb 6, 2024

WalshyDev commented Feb 20, 2024

ybw0014 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBusyBiscuit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ybw0014 commented Feb 24, 2024

WalshyDev commented Feb 25, 2024 • edited Loading

J3fftw1 commented Feb 5, 2024 •

edited by WalshyDev

Loading

github-actions bot commented Feb 5, 2024 •

edited

Loading

WalshyDev commented Feb 25, 2024 •

edited

Loading