GTNewHorizons / GT-New-Horizons-Modpack

New Modpack with Gregtech, Thaumcraft and Witchery
https://www.gtnewhorizons.com/
Other
1.01k stars 304 forks source link

Item counts in NEI pinned recipe chains are buggy #17148

Open Meegooo opened 2 months ago

Meegooo commented 2 months ago

Your GTNH Discord Username

Meegoo

Your Pack Version

2.6.1 (Reproduced with only NEI in dev environment)

Your Server

SP

Java Version

Java 17

Type of Server

Single Player

Your Expectation

Something consistent. Here is what I think is the intended behavior. Screenshots with messy item counts for reference image image

  1. With Ctrl held down:
    • For pure inputs, scrolling up or down should increase/decrease item count by one recipe amount (i.e. 5 iron for vanilla hopper)
    • For intermediate items. Same as pure inputs, plus increasing/decreasing amount of that item in recipe, that gives this item as output.
    • For outputs, item count should go up/down by one recipe amount, without requesting extra inputs
  2. With Shift+Ctrl held down:
    • For any item, scrolling should increase/decrease amount of items by one recipe for all of inputs, intermediates and outputs. Without changing amount of requested items (shown in bottom right with blue background), except for the recipe I scrolled on.
  3. With extra Alt, multiplier from NEI bottom right field is added to all increases/decreases.

The Reality

Scrolling can randomly lead to item counts breaking. I don't have exact steps to reproduce, but it seems to be some sort of race condition. Did not debug too deep, but BookmarkCraftingChain::calculatedItems seems to become complete nonsense after some fiddling with Ctrl+scroll.

image

In this state, scrolling down on input planks for a chest, causes plank amount to go up by 8. The reason for that is the mapping 232x -> 248x in BookmarkCraftingChain::calculatedItems. BookmarkGrid::realItems, as seen from this method, contains 232x, but getSlotMouseOver(mousex, mousey) (which eventually looks into BookmarkCraftingChain::calculatedItems returns x248. 248 gets decreased by 8 to 240, and then I guess calculatedItems gets updated, and 232x -> 248x becomes 240x -> 256x.

There is also some other nonsense, like 0x oreIron -> 1x oreIron, which is visible in game as ore count going down as I scroll down on iron ingots, ore count dropping to zero, and then, on next scroll down, setting itself from 0 to 1, without touching iron ingots.

Toggling recipe chain seems to temporarily fix it. I suppose because BookmarkCraftingChain::calculatedItems gets... recalculated

Your Proposal

Not having that bug would be nice, but I have other proposals to improve UX of this feature.

On first screenshots there is 4 chests in input for hopper crafts, and 5 as output from planks. 5 is calculated as 4 requested by hopper plus one extra craft that is "requested" by me (shown in blue with 1 in bottom right). When i scroll chests in hopper inputs up, amount of chests that is requested by me decreases by 1. Same in opposite direction, but not always. If I previously messed with counts of chests, it goes down up to the amount I messed with, and then starts adding items as if they were requested by me. That doesn't make much sense

Also, I would argue that input count in recipe should max out at whatever recipe requests, so in my example, for hopper recipe, chests should be maxed at 2 and iron at 10. But the amounts should be adjustable individually, not in steps of "one recipe", and this adjustment should be shown explicitly, as an offset from requested amount. This would be useful when I have 3 iron, so I can set iron requirement to 7, and with shift pressed it would show -3 somewhere. For intermediates it's slightly more complex, because you can have multiple recipes requesting iron. So maybe forcing amount of intermediates in inputs to always be equal to requested amount, but allowing adjustment on recipe that produces said intermediate. With similar -3 shown.

And last but not least, this

  • For outputs, item count should go or down by one recipe amount, without requesting extra inputs

doesn't make much sense either. At the very least extra hoppers without associated inputs should not be marked as requested image image

Final Checklist

Meegooo commented 2 months ago

This is an issue purely with NEI, so it might make more sense in NotEnoughItems' repo. Whatever you guys decide

Meegooo commented 2 months ago

Looked a bit through the code, found a bunch of problems and inconsistensies (like using ItemStack as key in hashable containers). So, if everyone is alright with it, I'm gonna try to rewrite that part of code. And maybe make a recipe graph, as described here https://github.com/GTNewHorizons/NotEnoughItems/issues/487

slprime commented 1 month ago
  1. This is create specifically so that you can scroll intermediate recipes, as well as ingredients separately. Also it is possible to add items to the chain without recipes to indicate that you already have them and do not need to calculate them.
  2. What version did you check on because I corrected some errors in the calculations after the release of 2.6.1?
  3. How will you solve the problem with looped recipes?
  4. How will you solve the problem if there is more than one final recipe in the chain?

P.S. I think you haven't grasped all the possibilities and flexibility of this functionality

Meegooo commented 1 month ago

This is create specifically so that you can scroll intermediate recipes, as well as ingredients separately.

You mean Shift+Scroll? Yeah, I got that. But I'm not sure if that's really necessary. Because (assuming vanilla recipes), I added a chest recipe, a hopper recipe and a chest minecart recipe. Then I shift+scrolled on planks in chest recipe. What is supposed to happen now? Does hopper take priority? Or minecart?

Also it is possible to add items to the chain without recipes to indicate that you already have them and do not need to calculate them.

Aha. I was planning to do this as well, but with completely locked ingredient amounts. So if you need to get a sum of items, or reduce/increase it, you pin that item.

What version did you check on because I corrected some errors in the calculations after the release of 2.6.1?

Whatever was in main branch at the time of creating this ticket.

How will you solve the problem if there is more than one final recipe in the chain?

In general speak. I want to add a separate counter for requested items into metadata, decoupled from current ItemStack size, stored in BookmarkGrid::realItems. Then, when crafting mode is enabled, I traverse the graph, starting from each item that is requested more than once, accumulating number of crafts, inputs and outputs for each recipe.

How will you solve the problem with looped recipes?

I looked into how Applied Energestics appears to do it (by behaviour, I did not look into code). Which is, go until you encounter a loop, and instead of continuing on, just don't add any more recipes to the queue from this branch. Which means, that adding Block->Ingot recipe and Ingot->Block recipe to the chain and requesting 10 blocks, you will see two recipes: 10 blocks <- 90 ingots, and 90 ingots <- 10 blocks.

P.S. I think you haven't grasped all the possibilities and flexibility of this functionality

In my book this is just a relatively simple graph traversal with a couple extra steps. The biggest hurdle is just minecraft code in general.

slprime commented 1 month ago

You mean Shift+Scroll? Yeah, I got that. But I'm not sure if that's really necessary. Because (assuming vanilla recipes), I added a chest recipe, a hopper recipe and a chest minecart recipe. Then I shift+scrolled on planks in chest recipe. What is supposed to happen now? Does hopper take priority? Or minecart?

  1. In the case of your example, how do you propose to change the quantity?
  2. The scroll will not work on intermediate recipes, only on latest ones? Do you have an idea how to visualize this so as not to break user experience?

P.S. It would make sense to remove ctrl+scroll then. And for change count always use shift+scroll

Meegooo commented 1 month ago

In the case of your example, how do you propose to change the quantity?

I was honestly thinking ctrl+scroll to adjust single output item quantities, to match non crafting chain mode. Meaning you can "request" 5 planks, and get told that you need 2 logs, you'll get your 5 planks plus 3 remaining (will be shown in the tooltip).

As for shift, I don't know. To match current behavior as closely as possible, I am thinking about increasing requested amount by "one craft" (i.e. 4 for planks). And probably allowing to scroll on inputs as well (it will increase requested output amount). If recipe has multiple outputs, all outputs will increase by one craft.

The scroll will not work on intermediate recipes, only on latest ones?

I'm not planning to distinguish between intermediate and final recipes, because any intermediate recipe can become both intermediate and final recipe at any moment, if you request some of that item. The exception is background colors while holding shift:

The thing I haven't figured out yet, is what numbers to show. Currently the plan is as follows:

slprime commented 1 month ago

I am confused by the many numbers on the item. Could it look dirty and unreadable? The negative amounts also confuses me. I need to see what it looks like.

How will an item added to the chain without a recipe behave? (Now it signals the algorithm how much of this item the player has. now it works like yours "negative amounts")

P.S. ItemStack may be more the millions P.S.2. I don't know if you've seen the new tooltip: image

slprime commented 1 month ago

...because any intermediate recipe can become both intermediate and final recipe at any moment, if you request some of that item.

What do you mean by the word "request"?

P.S. English is not my main language :)

Meegooo commented 1 month ago

As for Millions, Billions, etc, I reused the existing code, so big numbers are displayed the way they currently are.

I'm still working on the highlighting, but here are some screenshots image +8.0M on hoppers means I want 8 million hoppers. -4M on iron blocks means I have 4M iron blocks. 444K in the bottom right means I still need to get/craft 444K iron blocks.

Here is with shift held. image. This says that I need to do 8M crafts of hoppers, 4.4M crafts of iron ingots (4.4M 9 per craft will get me 40M ingots), 8M chest crafts, 16M wood plank crafts (16M 4 per craft = 64M).

PS. Speaking of highlights. How about showing highlights without having to hover over items? For all groups at once, when mouse is over bookmark panel in general. Thinking this because hovering over item shows tooltip, which is in the way.

P.S.2. I don't know if you've seen the new tooltip:

Yep

P.S. English is not my main language :)

Saw in the discord that your main language is Russian. I do speak Russian, so feel free to ping/DM me in discord for any details.