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.

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

Implement new crafting algorithm #132

Open
wants to merge 20 commits into
base: master
from

Conversation

@andrenete
Copy link

commented Aug 22, 2019

I wrote a better version of crafting algorithm that, I believe, optimally matches inventory items to crafting grid.

The algorithm can be briefly described in the following steps:

  • Get info about crafting items and groups, their positions in the crafting recipe (craft_index)
  • Get info about inventory items that can be used in the craft, their total count in inventory (item_index)
  • Match craft items and groups with inventory items according to the recipe and also find the amount of items for every recipe cell:
    • Item matching is easy because its always 1:1 match
    • Optimal group matching requires the specific order of iteration through groups (their items counts). For that, every group items counts are first sorted in descending order and then compared "lexicographically".
      After that, we iterate through sorted groups (their items counts) and for every group we take item with max possible total_count / (times_matched + 1) value.
      Global amount of items per cell is the min of this values.
  • Check if crafting grid already contains only needed items (or empty cells) and move stacks from main inventory.

As to why the order of group iteration is important, consider an example:

Inv Item 1 (count = 28) Inv Item 2 (count = 15) Inv Item 3 (count = 20)
Group Cell 1
Group Cell 2

If we start with group 2, we first match item 1 for group 2 (28 / (0 + 1) = 28) then item 2 for group 1 (because [28 / (1 + 1)] < [15 / (0 + 1)]), and the best amount we can get is 15. But actually 20 is the best (group 1 - item 1, group 2 - item 3).

The really hard case is when crafting grid is not empty. In that case the algorithm still find match for every recipe cell and moves matched items only if the craft grid already have the same items in the same positions (or empty). Actually, I don't like this behavior on non-empty grid, but there are too many cases to consider to make the algorithm work correctly (so it can, for example, ignore some cells if the crafting grid already have large amount of items in them), especially when there are groups of items in the craft recipe. Also it would require an extra re-matching step.

I believe that the algorithm works correctly with any crafting recipe whether it contains groups or not, but if you've found some bug or mistake please tell about it. I hope that this PR helps!

@andrenete

This comment has been minimized.

Copy link
Author

commented Aug 22, 2019

Oh, I just found that I implemented recipe iteration wrong. Sorry, I will fix that!

register.lua Outdated Show resolved Hide resolved
@andrenete

This comment has been minimized.

Copy link
Author

commented Aug 23, 2019

Found one more problem: move code works incorrectly with unstackable items and also should check if take_count == 0 or else some items can be moved incorrectly or disappear. I will fix that. Sorry, I should've read the previous code more carefully.

@SmallJoker

This comment has been minimized.

Copy link
Member

commented Aug 23, 2019

I'm interested in a saner method to fill the crafting grid. My recent changes on this code were to make it fast (finally 10x faster), while keeping it reasonably small. But as a downside it cannot spread the items well and only provides a partial copy-to-grid function.
Please also do come execution time benchmarking to ensure it's not worse than the old code (before #123).

@andrenete

This comment has been minimized.

Copy link
Author

commented Aug 24, 2019

In #123 you wrote that you test "various" recipes for crafting 10 items. Do you remember what recipes and items in inventory you tried, so we can compare the results? Maybe you could test it better if you know some worst cases?

The worst time I got so far is when crafting locked chest when having main inventory filled with different types of wood planks and steel: 3735 μs. The average from some craft recipe - inventory items combinations is around 2000 μs.

For testing, I measured the time before and after unified_inventory.craftguide_match_craft call in register.lua.

@andrenete

This comment has been minimized.

Copy link
Author

commented Aug 24, 2019

This non empty grid case made me think: maybe it's better to forbid players to keep things in the craft grid? Because that's not what it's for anyway.
Is it possible to return items to the main inventory when the player closes the inventory page? Or when there is no space in the main inventory just drop items on the ground? This is what minecraft does and I think it's much better than to manage matching leftovers and stuff by hand.

@SmallJoker

This comment has been minimized.

Copy link
Member

commented Aug 24, 2019

Do you remember what recipes and items in inventory you tried, so we can compare the results?

That was the bookshelf recipe (10x), also with different kinds of planks and sometimes (partially) filled crafting slots.

Or when there is no space in the main inventory just drop items on the ground?

No, this behaviour may not change. Player expect the copy function to be safe since it's always been that way for years. You could move items stackwise back to the inventory, and skip those where's no space available.

3.7ms is quite good compared to the version before #123 (you might test that for comparison as well). All fine regarding the execution time.

@andrenete

This comment has been minimized.

Copy link
Author

commented Aug 24, 2019

I tested the bookshelf recipe. The worst case that I've been able to get is when I tried to craft "all" but with already filled crafting grid for 10 bookshelves (See video).

It turns out that the biggest time was spent on actual moving and not the matching process:

Craft index time: 44 μs
Item index time: 376 μs
Match time:      682 μs
Move check time:  13 μs
Move time:     10575 μs -- for match_pos, item_name in pairs(matched_items) do ... end
Inv set time:    599 μs

Total time: 12289 μs

I tried that recipe again, but this time I measured the time between inv:remove_item(src_list_name, matched_stack) in the moving loop:

348  μs
1902 μs
591  μs
602  μs
818  μs
900  μs
909  μs
708  μs
637  μs

Total time: 7415 μs

So it all comes down to the fact that remove_item works pretty slow.

@andrenete

This comment has been minimized.

Copy link
Author

commented Aug 24, 2019

I just compiled a new minetest version from source and item removing seems to be working faster. But now there's a bug that main inventory doesn't immediately updates after stack moving (Video). It affects both my version and master version of crafting code, so I guess it's some core problem.

@SmallJoker

This comment has been minimized.

Copy link
Member

commented Aug 25, 2019

I guess it's some core problem.

It indeed is (or rather was). Fixed by now, don't worry about this issue.
EDIT: It's possible to speed up remove_item and add_item by collecting all inventory changes to send at once, but that's a different story. For now I think it's best to move stacks as big as possible.

@andrenete

This comment has been minimized.

Copy link
Author

commented Aug 26, 2019

I finally made it work correctly with non empty crafting grid. Code is extremely ugly now, but takes many more cases into account, can swap unneeded items, swap items inside crafting grid if main inventory is full. Need lots of refactoring and also corner cases checking.
EDIT: add and remove from crafting table should exclude already matched positions, I will fix that

@SmallJoker

This comment has been minimized.

Copy link
Member

commented Aug 27, 2019

PS: The change to optimize remove_item (and alike) is already pending. minetest/minetest#8856
After it's merged, these operations will be remarkably faster (did not speed test yet).

@andrenete

This comment has been minimized.

Copy link
Author

commented Sep 1, 2019

I'm still thinking about the correct implementation of the mathing algorithm and I realized that my current approach is wrong when groups in the crafting recipe overlap (inventory contains items that belong to multiple groups).

To clear things up, the task I'm trying to solve is follow:

  • Suppose we have a set of resources: {r1, r2, r3, ..., rn}.
    This is our items in inventory. Each resource ri is determined by its total count: ri.count.

  • We also have a set of cells that we can put resources into: {c1, c2, ..., ck}.
    This cells represent the crafting grid and in our case k = 9.

  • For a specific crafting recipe we know, what resources we can put in every cell. In other words we have a map:
    c1 -> {r11, r21, ..., rn1}
    c2 -> {r21, r22, ..., rn2}
    ...
    ck -> {r1k, r2k, ..., rnk}

The task is to select exactly one resource for every cell (if it's possible, of course), such that the minimum resource count per cell is maximal. Note that one resource can be splitted between multiple cells, but one cell can contain only one resource (because different items are unstackable).

Example:

{r1, r2, r3}
r1.count = 30
r2.count = 28
r3.count = 20

{c1, c2, c3}
c1 -> {r2, r3}
c2 -> {r1, r2}
c3 -> {r1}

Best:
c1 -> r3
c2 -> r2
c3 -> r1
Minimum count: 20

Not best:
c1 -> r2
c2 -> r1
c3 -> r1
Minimum count: 15 (divide r1 between two cells)

For this example, my algorithm will produce the second result, which is worse than the first one. So 'cell lookup order' approach is incorrect here.

We can also imagine our map as a mathematical graph with resources and cells as vertices and 'resource can be put in cell' connections as edges. Since edges only connect resources with cells, this graph is a bipartite graph.

Maybe there are existing algorithms that already solve this?

@SmallJoker

This comment has been minimized.

Copy link
Member

commented Sep 1, 2019

mind = blown

I never analysed algorithms this percise, neither am I familiar with any words mentioned below:

Since edges only connect resources with cells, this graph is a bipartite graph.

From what I can tell: There's yet no function to count the total of similar nodes within the inventory, but that should be doable. My approach for this craft recipe copying would be the following:

  1. Iterate through any crafting slot
  2. Select the biggest (in total) ItemStack type that matches the required group(s)
  3. If the crafting stack is non-empty: look out for that specific ItemStack
  4. If there are not enough same ItemStacks: put the crafting stack back to the main list and do step 2

Requires to:

  1. Count all present ItemStacks, including those in the crafting list
  2. Check whether there's enough space in the inventory to perform swapping from 4) above.
@andrenete

This comment has been minimized.

Copy link
Author

commented Sep 1, 2019

I believe the hardest part is step 2. If we take biggest stack every time, we may end up with unfilled crafting slots, or with unoptimal placement of items in the grid. This is what I'm trying to solve, and this is why there's so much code :) How this would work, for example, if I have just one stack 'copper 20' in main and want to craft 'All' copper blocks?

Other steps is what I actually do now.

  • I cache inventory information from main and craft lists one time and store info in item_index table. (add_list_items function adds all list items in index).
  • If swapping in main is impossible it tries to move to another craft slot, but after current slot was already matched (We also need to check if this another craft slot is not previously matched slot, take_item_skip and add_item_skip functions). If this also impossible then it tries to revert moving steps.
@SmallJoker

This comment has been minimized.

Copy link
Member

commented Sep 1, 2019

I meant the sum of all same stacks. Then the sum of all stacks within each group. Latter needs re-calculating after each movement, but this way you could spread the required items per craft slot proportionally.
Great to see that the situation described in step 2 is already handled - very nice.

@andrenete

This comment has been minimized.

Copy link
Author

commented Sep 1, 2019

Proportionally yes, but not optimally.

Imagine you want to craft 'All' wooden swords. In main you have 99 sticks, 4 pine wood planks and 12 apple wood planks. Wood group total = 16 and we have two crafting slots that require wood group. So we divide 16 by 2 and get 8. Then we start moving:

  • max ItemStack for first slot is apple wood planks. We take 8 and decrease count: total = 8, apple wood planks = 4.
  • max ItemStack for second slot is also apple wood planks (or pine because we now have equal amount of them). We take 4 and stop.

So for this two slots we match 8 and 4. But best is 6 and 6 (12 apple wood planks / 2). Sticks count doesn't really matter here, 99 just because it's more than wood group total.

I think this is why I rejected this idea at the start. Or maybe I miss something?

for match_pos = 1, dst_count do
local item_name = matched_items[match_pos]

if item_name ~= nil then

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Oct 10, 2019

Member

You might want to put this into a pseudo loop to reduce the indents below:

repeat
	-- ...
	if src_leftover:is_empty() then
		break
	end
	-- ...
until true
for i = 1, list_count do
local stack = inv_list[i]

if not stack:is_empty() then

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Oct 10, 2019

Member

Maybe repeat-until here to reduce indent levels as well?

@SmallJoker

This comment has been minimized.

Copy link
Member

commented Oct 10, 2019

Sorry for the delay. I just tested the PR, and it works really well - at least after the optimizations in 5.1.0-dev.

Test conditions: 4x acacia wooden planks stacks + 4x books stacks, craft "All" bookshelves
5.1.0-dev (10 June):

Exec time: 3608 μs
Exec time: 7116 μs -- non-empty craft grid
Exec time: 4173 μs
Exec time: 12708 μs -- non-empty craft grid
Exec time: 4456 μs

5.1.0-dev (9 Oct):

Exec time: 1005 μs
Exec time: 1205 μs
Exec time: 1126 μs -- non-empty craft grid
Exec time: 1099 μs
Exec time: 1101 μs -- non-empty craft grid

Would you please be so nice to add an introduction to "match_crafts.lua", to understand the logic behind this code? Also some function descriptions would be incredible helpful if any bugs show up in the future.

@andrenete

This comment has been minimized.

Copy link
Author

commented Oct 15, 2019

Looks like the problem with the optimal placement is more complicated than I thought... Instead, I gave up this "lexicographical sort" idea and rewrote the entire code from scratch to make it more understandable and simple. And besides, I don't even know if there are recipes with overlapping groups of items....
I also added a description for each function.

@andrenete

This comment has been minimized.

Copy link
Author

commented Oct 16, 2019

As for indents, the maximum depth has been reduced to 5. I don't think a pseudo loop is needed now.

Copy link
Member

left a comment

Amazing work. Thank you so much for your efforts!

Will merge in a few days. Please ping me if I forgot to do so.

@andrenete

This comment has been minimized.

Copy link
Author

commented Oct 17, 2019

Sorry, made one small change again. Depth is 6 now, but I think its ok.

break
repeat
if not occupied:is_empty() then
local leftover = unified_inventory.add_item(inv, src_lists, occupied)

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Oct 17, 2019

Member

Could there be an edge-case where it cannot be added do any stack?
Also, what's the difference to an integrated loop (below)? It should do the same.

while not occupied:is_empty() do
...
end

This comment has been minimized.

Copy link
@andrenete

andrenete Oct 17, 2019

Author

There is a case where both main and craft lists are full and occupied stack item differs from all other items both in main and craft. Also note that before this, in line 350, this position in craft list (destination) is pre-filled with matched item so this position is not empty and match item can differ from occupied. If this is the case then revert this pre-fill (put leftover to current position) and dont take items from removed and go to the next position. Whats interesting that in this case items still can be placed in the remaining positions correctry if remaining occupied items can be stacked and can be placed to positions other than used in the match.

Dont know about while loop, can be possibly infinite, so better avoid that. I also dont modify occupied, or you mean if we do occupied = leftover? I only wanted to change break to continue in line 358. If we break from for loop itself as it was previously, we can skip positions where we still can move matched items to.

I think I will add screenshot for this case later.

P.S. add_item in line 368 is always successful because removed count after the for loop is less than or equal to the count before it.

This comment has been minimized.

Copy link
@andrenete

andrenete Oct 17, 2019

Author

https://imgur.com/a/5r6h1V1

Actually, the last corner case is when at the moment of moving to some position there is no free space for occupied item, but there IS after the whole process of moving. For this example, this is because we only take floor(99 / 6) * 6 = 96 steel and the rest 3 is taking space that can be used for gold block.

@andrenete

This comment has been minimized.

Copy link
Author

commented Oct 17, 2019

Just discovered that inv:remove_item correctly handles oversized stacks, but inv:add_item does not? I mean shouldn't it add stacks with at most stack_max items (99 + 99 instead of 198, for example)?

Last corner case can be solved by removing at most stack_max * count_pos instead of bounded_amount * count_pos to ensure that if all count_pos positions are already occupied with the matched item they all will be freed at the current step. The problem is that if we do add_item after that we will (in most cases) end up with oversized stack in the inventory.

@andrenete

This comment has been minimized.

Copy link
Author

commented Oct 17, 2019

I'm so sorry, current code doesn't work for recipes with unstackable items with wear or metadata. Removing all bounded_amount * count_pos items at once can lead to metadata loss (inv:remove_item is pretty dangerous in that sence, though it allows for oversized stacks.)
But removing them sequentially can lead to other problems... will think about it.

@SmallJoker

This comment has been minimized.

Copy link
Member

commented Oct 18, 2019

I mean shouldn't it add stacks with at most stack_max items (99 + 99 instead of 198, for example)?

There were so many "bugfixes" in that area that I don't remember any more what exactly the side-effects of that behaviour were. The inventory mechanism is quite generic, hence rules for players also apply to mods.
I think there were also some functions that did not return the correct leftover stack sizes. Please check whether 5.1.0 works the same way (or preferably, better).

@andrenete

This comment has been minimized.

Copy link
Author

commented Oct 19, 2019

I fixed the issue with metadata and placement now works for this last corner case (screenshot that I added earlier). I think this is the final version.

There are cases when items from oversized stacks can dissapear though. But for previous version of the code it could happen too.

EDIT: items from oversized stacks are now being dropped to the ground.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.