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.

Sign up for GitHub

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

Jump to bottom

fix AContainer can't find recipes with similar items #4172

Closed
wants to merge 1 commit into from
Closed

fix AContainer can't find recipes with similar items #4172

wants to merge 1 commit into from

Conversation

lijinhong11
Copy link

@lijinhong11 lijinhong11 commented Apr 18, 2024 •
edited
Loading

Description

#4166

Proposed changes

Skip scanned slots

Related Issues (if applicable)

#4166

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.

@lijinhong11 lijinhong11 requested a review from a team as a code owner April 18, 2024 11:16
@github-actions GitHub Actions
Copy link
Contributor

Pro Tip!
You can help us label your Pull Requests by using the following branch naming convention next time you create a pull request. ❤️

Branch naming convention Label
feature/** 🎈 Feature
fix/** ✨ Fix
chore/** 🧹 Chores
api/** 🔧 API
performance/** 💡 Performance Optimization
compatibility/** 🤝 Compatibility
If your changes do not fall into any of these categories, don't worry. You can just ignore this message in that case! 👀

@JustAHuman-xD
Copy link
Contributor

Have you tested this?

@JustAHuman-xD JustAHuman-xD added the ✨ Fix This Pull Request fixes an issue. label Apr 18, 2024
@JustAHuman-xD JustAHuman-xD self-assigned this Apr 18, 2024
@lijinhong11
Copy link
Author

not :( but I will test later

@lijinhong11
Copy link
Author

lijinhong11 commented Apr 18, 2024 •
edited
Loading

Tested and it fixed with [64,64] ([64,32] may too)

@JustAHuman-xD
Copy link
Contributor

If we could get another person to test and verify that'd be great!

@github-actions GitHub Actions
Copy link
Contributor

Slimefun preview build

A Slimefun preview build is available for testing!
Commit: e021d694

https://preview-builds.walshy.dev/download/Slimefun/4172/e021d694

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!

JustAHuman-xD
JustAHuman-xD reviewed Apr 22, 2024
View reviewed changes
Copy link
Contributor

@JustAHuman-xD JustAHuman-xD left a comment

Choose a reason for hiding this comment

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

Just a code style (and kinda performance but just micro optimization) thing

src/main/java/me/mrCookieSlime/Slimefun/Objects/SlimefunItem/abstractItems/AContainer.java
@@ -422,6 +422,8 @@ protected MachineRecipe findNextRecipe(BlockMenu inv) {
for (ItemStack input : recipe.getInput()) {
for (int slot : getInputSlots()) {
if (SlimefunUtils.isItemSimilar(inventory.get(slot), input, true)) {
if (found.containsKey(slot)) continue; //skip scanned slots
Copy link
Contributor

Choose a reason for hiding this comment

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

You should invert this check and merge it with the surrounding of statement, have it be the first boolean expression

@JustAHuman-xD
Copy link
Contributor

JustAHuman-xD commented Apr 22, 2024 •
edited
Loading

If we could get another person to test and verify that'd be great!

I want to make sure this doesn't somehow break any existing thing as I'm worried this might break a recipe that is setup as [32,32] and the slot setup is something like [1, 64]

@lijinhong11
Copy link
Author

So there's the only way is to stack items' amount

@JWJUN233233
Copy link
Contributor

im afraid that it wont work.
if there is a recipe a[Diamondx64] and a recipe b[Diamondx64, Iron_ingot] and there are 64 diamonds in the AContainer, it should match recipe a instead of b. i may give another solution later.

@lijinhong11
Copy link
Author

lijinhong11 commented Apr 27, 2024 via email

@JWJUN233233
Copy link
Contributor

and i wonder why not use hashcode. if the NBT of item is different from that in recipe, it shouldnt match.

@JustAHuman-xD
Copy link
Contributor

and i wonder why not use hashcode. if the NBT of item is different from that in recipe, it shouldnt match.

Because there are tons of cases where nbt wouldn't be identical but the ingredient is still valid

@JWJUN233233
Copy link
Contributor

and i wonder why not use hashcode. if the NBT of item is different from that in recipe, it shouldnt match.

Because there are tons of cases where nbt wouldn't be identical but the ingredient is still valid

Could you please give an example?

@JustAHuman-xD
Copy link
Contributor

JustAHuman-xD commented Apr 27, 2024 •
edited
Loading

Could you please give an example?

Sure! We already have problems with it in other areas so I can just give a couple of those:

  1. the recipe calls for a diamond hoe, you have a diamond hoe that was used and then repaired, nbt won't be identical.

  2. You have a recipe that calls for fertilizer, the fertilizer has a custom model nbt because the server previously had a resource pack but now does not or the model number changed, nbt won't be identical

Etc etc

@JWJUN233233
Copy link
Contributor

thats so bad

@JustAHuman-xD
Copy link
Contributor

thats so bad

?

@JWJUN233233
Copy link
Contributor

JWJUN233233 commented Apr 27, 2024 •
edited
Loading

i created my pr #4177

@lijinhong11 lijinhong11 closed this May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JustAHuman-xD JustAHuman-xD JustAHuman-xD left review comments

Assignees

@JustAHuman-xD JustAHuman-xD

Labels
✨ Fix This Pull Request fixes an issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lijinhong11 @JustAHuman-xD @JWJUN233233