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

Also use the player's inventory to handle NEI fills #575

Merged
merged 1 commit into from Dec 21, 2014

Conversation

Projects
None yet
5 participants
@riking
Contributor

riking commented Dec 10, 2014

If the items for a recipe are not available in the ME network when you shift-left-click the NEI question mark, also try to pull from the player's inventory.

The 'ic' local variable was renamed to 'testInv', as it's used to test the IRecipe on various crafting propositions to see if the item satisfies the recipe. I can remove this if you want, but I think it increases the readability.

Adds #564, "Shift clicking "?" on NEI recipe ignores items in the player's inventory".

@AlgorithmX2

This comment has been minimized.

Contributor

AlgorithmX2 commented Dec 10, 2014

I believe your missing some logic related to realForFake, which defines if the item should actually be removed, or if it should just be "simulated"

in one case for Pattern Terminal, and in the other for the Crafting Terminals

@riking

This comment has been minimized.

Contributor

riking commented Dec 10, 2014

Wait, if it's simulated, shouldn't Platform.extractItemsByRecipe() always succeed in getting the items?

Because the // If that doesn't get it, grab exact items from network section (line 170-189) doesn't check for simulated mode either.

So it's a "problem" in the existing code too, which would get surfaced only from a refactor.

@AlgorithmX2

This comment has been minimized.

Contributor

AlgorithmX2 commented Dec 10, 2014

extractItemsByRecipe only returns items available to it, simulation is an existence check.

yeah I don't know what the deal with the middle section is, a random guess would be that that never happens when the item exists and its pointless. But that is just a guess.

And it can't extract an item that doesn't exist anyway, I have a hard time thinking that extractItemsByRecipe would miss the item some how.

@AlgorithmX2

This comment has been minimized.

Contributor

AlgorithmX2 commented Dec 10, 2014

Err, That is not what I said, extractItemsByRecipe can of course return null, if the item is not in the network.

Anyhow I'll leave up weather this is an acceptable solution to someone more involved in the project.

@riking

This comment has been minimized.

Contributor

riking commented Dec 10, 2014

Oh, I see. Whoops. Testing, amirite?

Will replace/amend the commit.

@riking

This comment has been minimized.

Contributor

riking commented Dec 10, 2014

Actually, this brings up a 'regression': You can't encode a crafting pattern if you don't have the items, which was possible in AE1. (I didn't fix that)

Added a commit that fixes the simulated case, for the new code at least.

@psxlover

This comment has been minimized.

psxlover commented Dec 10, 2014

According to AlgorithmX2 it's intended behavior and it wasn't available in AE1 either (it was added by NEI addons).

(Issue #335 and http://ae-mod.info/Tracker/0759/)

@riking

This comment has been minimized.

Contributor

riking commented Dec 10, 2014

I see. Well we can ignore that part, then. I think I got the SIMULATED behavior right?

@AlgorithmX2

This comment has been minimized.

Contributor

AlgorithmX2 commented Dec 10, 2014

@psxlover is dead on, Patterns via NEI is a new feature to AE2, and does require the items ( intended ).

It seems like it would work fine at a glance, only testing can confirm.

@yueh

This comment has been minimized.

Member

yueh commented Dec 11, 2014

testInv as variable sounds like a quick'n'dirty hack for a proof of concept.
But the whole class should be refactored (not necessarily in this PR). Adding inline comments to give the following lines a context is a clear indication to move it into a method.

But my opinion is still, that it is a controversial change as it changes the behaviour to the contrary one. Just because a few (vocal) see it as QoL improvement, does not make it one for every users.
This feedback will very likely only provided after it is released

For me personally it would be quite the opposite, because I use the inventory to prevent the items from being used. It is far easier to notice a missing item in the crafting grid instead of constantly remembering your whole inventory including the size of each stack and check after each craft if something is missing.

Based on the code changes, I do not see anything preventing it being optional.
PacketNEIRecipe is even providing the basic support for it already.

@AlgorithmX2

This comment has been minimized.

Contributor

AlgorithmX2 commented Dec 11, 2014

@yueh

because I use the inventory to prevent the items from being used

You pull items you don't want used as a pattern out of the network and stick them in your inventory?

I can see how this would matter for crafting, like if you wanted to make sure you didn't use your fancy wood or something. But I don't really see the significance of this for the NEI Integration.

I'm only being vocal about how I think more options for this are on unessiary because try as I might to think of a good reason for them, I can't think of any reason why it would cause anything other then a quick adjustment ( which since NEI is QoL already, I don't see as an issue. )

@yueh

This comment has been minimized.

Member

yueh commented Dec 11, 2014

Mainly crafting, it is ok for patterns as long as it does not use the player inventory to craft as well.
Noone really mentioned if it should apply to either patterns or crafting. Or both even.
So I assumed for everything.

@AlgorithmX2

This comment has been minimized.

Contributor

AlgorithmX2 commented Dec 11, 2014

I've been assuming this was just an NEI Integration change, not for the crafting terminals crafting extraction feature ( which I think should remain unchanged, since the crafting terminal should craft from the network, not from your inventory )

That said, I guess this would make the initial crafting selection from NEI work for the crafting terminal ( this should probably not happen )

Honestly seems a bit mixed up now, could be why I didn't bother with it eariler.

@thatsIch

This comment has been minimized.

Member

thatsIch commented Dec 20, 2014

@riking can you squash the commits please?

Also use the player's inventory to handle NEI fills
If the items for a recipe are not available in the ME network when you
shift-left-click the NEI question mark, also try to pull from the
player's inventory.

The 'ic' local variable was renamed to 'testInv', as it's used to test
the IRecipe on various crafting propositions to see if the item
satisfies the recipe.

Closes #564, "Shift clicking "?" on NEI recipe ignores items in the
player's inventory".
@riking

This comment has been minimized.

Contributor

riking commented Dec 21, 2014

@thatsIch sploosh

thatsIch pushed a commit that referenced this pull request Dec 21, 2014

thatsIch
Merge pull request #575 from riking/recipe-pull-player
Use player's inventory to handle NEI fills

@thatsIch thatsIch merged commit b069fa0 into AppliedEnergistics:rv2 Dec 21, 2014

1 check passed

default Finished TeamCity Build Applied Energistics :: Pull Requests : Tests passed: 3
Details
@yueh

This comment has been minimized.

Member

yueh commented Dec 21, 2014

How is the behaviour now? Does it only use the items for the pattern terminal and not the crafting one?

@riking riking deleted the riking:recipe-pull-player branch Dec 24, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment