peter-kish / gloot

A universal inventory system for the Godot game engine.
MIT License
561 stars 20 forks source link

transfer_autosplitmerge fail on full inventory #201

Closed TreaBeardGaming closed 3 months ago

TreaBeardGaming commented 4 months ago

transfer_autosplitmerge fails to transfer any items to a full inventory with full stacks or even partial stacks. assert(!item_count.eq(ItemCount.inf()), "Item count shouldn'td bew infinite!") adding a print before this reveals the value is -1, which is the default value set for inf in the item_count.gd script. print(str("Item Count: ",item_count," Count: ",item_count.count))

Item Count: <RefCounted#-9223366887021179699> Count: -1

In my script the transfer_automerge failed returning false so it falls back onto transfer_autosplitmerge which throws an assert cause of the item_count == -1

My thought process was if the automerge return false I would instead use autosplitmerge to fill up the remaining stacks.

TreaBeardGaming commented 4 months ago

Update, for some reason it is letting me set the stack size in code higher than the max stack size of the item. I corrected this, now adding items to the inventory with stack size between 1-16 depending on randi/etc

Removed the lines of code for transfer_automerge and just used transfer_autosplitmerge. I filled the overflow inventory before doing transfer_autosplitmerge, same thing occurs if I transfer after each item is added, was just harder to tell what was happening so I filled the overflow first before the transfer.

[Item Count]: <RefCounted#-9223371211902259560> [Count]: 36
   At: res://addons/gloot/core/constraints/stacks_constraint.gd:262:transfer_autosplit()
[ASM]: @Node@171:<Node#556735164419> - Oak Wood [Stack Size]: 28
[ASM]: @Node@172:<Node#556785496181> - Oak Wood [Stack Size]: 33
[ASM]: @Node@174:<Node#556886159192> - Oak Wood [Stack Size]: 8
[Item Count]: <RefCounted#-9223371169925673282> [Count]: -1
   At: res://addons/gloot/core/constraints/stacks_constraint.gd:262:transfer_autosplit()

so the playerinventory previously had 28 white logs, and in the overflow there was 64 white logs. It succesfully transfered 36 white logs making the stack full at 64, and leaving 28 in the overflow.

And this was fine till another transfer was initiated, which could not be completed and the assertion was hit. image

peter-kish commented 4 months ago

I put together two tests to verify this, but couldn't get the assertion to happen :(

The first test tries to transfer an item stack into a full inventory:

func issue_201_1() -> void:
    # Create two 2x2 inventories
    var inv1 = InventoryGridStacked.new()
    inv1.item_protoset = preload("res://tests/data/item_definitions_grid.tres")
    inv1.size = Vector2i(2, 2)

    var inv2 = InventoryGridStacked.new()
    inv2.item_protoset = preload("res://tests/data/item_definitions_grid.tres")
    inv2.size = Vector2i(2, 2)

    # Fill the first inventory with 1x1 item stacks with max stack size
    for i in range(4):
        var new_item = inv1.create_and_add_item("item_1x1")
        InventoryGridStacked.set_item_max_stack_size(new_item, 3)
        assert(InventoryGridStacked.set_item_stack_size(new_item, 3))

    # Add a 1x1 item stack with max stack size to the second inventory
    var item = inv2.create_and_add_item("item_1x1")
    InventoryGridStacked.set_item_max_stack_size(item, 3)
    assert(InventoryGridStacked.set_item_stack_size(item, 3))

    # Try to transfer the 1x1 item from the second inventory to the first one
    # using transfer_autosplitmerge
    assert(!inv2.transfer_autosplitmerge(item, inv1))

    inv1.free()
    inv2.free()

while the second one transfers into into an inventory with a partial stack:

func issue_201_2() -> void:
    # Create two 2x2 inventories
    var inv1 = InventoryGridStacked.new()
    inv1.item_protoset = preload("res://tests/data/item_definitions_grid.tres")
    inv1.size = Vector2i(2, 2)

    var inv2 = InventoryGridStacked.new()
    inv2.item_protoset = preload("res://tests/data/item_definitions_grid.tres")
    inv2.size = Vector2i(2, 2)

    # Fill the first inventory with 1x1 item stacks with max stack size, except
    # for one
    for i in range(4):
        var new_item = inv1.create_and_add_item("item_1x1")
        InventoryGridStacked.set_item_max_stack_size(new_item, 3)
        if i == 0:
            assert(InventoryGridStacked.set_item_stack_size(new_item, 1))
        else:
            assert(InventoryGridStacked.set_item_stack_size(new_item, 3))

    # Add a 1x1 item stack with max stack size to the second inventory
    var item = inv2.create_and_add_item("item_1x1")
    InventoryGridStacked.set_item_max_stack_size(item, 3)
    assert(InventoryGridStacked.set_item_stack_size(item, 3))

    # Try to transfer the 1x1 item from the second inventory to the first one
    # using transfer_autosplitmerge
    assert(inv2.transfer_autosplitmerge(item, inv1))

    inv1.free()
    inv2.free()

In the first case transfer_autosplitmerge returns false and in the second it returns true, but there are no assertions.

Did I understand the use case correctly or maybe I missed some details?

Edit: Reopening the issue, closed it by accident

TreaBeardGaming commented 4 months ago

The stacks_constraint.gd file when calling transfer_autosplitmerge() does a var new_item using func transfer_autosplit, it's in that func that the assert exists:

func transfer_autosplit(item: InventoryItem, destination: Inventory) -> InventoryItem:
    assert(inventory._constraint_manager.get_configuration() == destination._constraint_manager.get_configuration())
    if inventory.transfer(item, destination):
        return item

    var stack_size := get_item_stack_size(item)
    if stack_size <= 1:
        return null

    var item_count := _get_space_for_single_item(destination, item)
    print_debug(str("[Item Count]: ",item_count," [Count]: ",item_count.count))
    assert(!item_count.eq(ItemCount.inf()), "Item count is infinite!")
    var count = item_count.count

    if count <= 0:
        return null

    var new_item: InventoryItem = split_stack(item, count)
    assert(new_item != null)
    assert(destination.add_item(new_item))
    return new_item

If I comment out the: _assert(!itemcount.eq(ItemCount.inf()), "Item count is infinite!") then I don't get the assertion errors, and the split merge works just fine. I'm not sure entirely the reason, and what the purpose of the assertion line in that function is supposed to do.

I have 3 inventories of the GridStacked/Grid type, currently I am manually making the list of items I want to pull from, but eventually I will just get them from the protoset. I'll figure that part out later. And I'll clean up my mess eventually

func harvest_done():
    if health == 0 && !Engine.is_editor_hint():
        rootParent = find_parent("WorldEnvironmental")
        guiChild = rootParent.find_child("GUIContainer",false, true)

        tempInventoryGrid = guiChild.find_child("TempCtrlInventoryGrid",true,true)
        overflowInventoryGrid = guiChild.find_child("OverFlowCtrlInventoryGrid",true,true)
        ctrlInventoryGrid = guiChild.find_child("CtrlInventoryGrid",true,true)

        tempInventory = tempInventoryGrid.inventory as Inventory
        overflowInventory = overflowInventoryGrid.inventory as Inventory
        inventoryGrid = ctrlInventoryGrid.inventory as Inventory

        var items_to_add = {0:"oak_wood_common",1:"oak_wood_magic",2:"oak_wood_rare",3:"oak_wood_epic",4:"oak_wood_legendary",5:"oak_wood_unique"}
        var item_to_rates = {0:100,1:750,2:3500,3:9500,4:25000,5:750000}

        for key in items_to_add.keys():
            tempInventory.create_and_add_item(items_to_add[key])
            var theItem = tempInventory.get_item_by_id(items_to_add[key])
            var iDropAmount = harvest_droprate((item_to_rates[key])*150,resource_amount,key)
            if iDropAmount >= 1:
                tempInventory.set_item_stack_size(theItem, iDropAmount)
                tempInventory.transfer_automerge(theItem, overflowInventory)
            else:
                tempInventory.clear()

        for am in overflowInventory.get_items():
            if overflowInventory.transfer_automerge(am, inventoryGrid):
                print(str("[AM]: ",am," - ", am.get_property("Item_Name")," [Stack Size]: ", am.get_property("stack_size")))
            else:
                print(str("[AM] Failed to Transfer: ",am," - ", am.get_property("Item_Name")," [Stack Size]: ", am.get_property("stack_size")))

        for asm in overflowInventory.get_items():
            if overflowInventory.transfer_autosplitmerge(asm, inventoryGrid):
                print(str("[ASM]: ",asm," - ", asm.get_property("Item_Name")," [Stack Size]: ", asm.get_property("stack_size")))
            else:
                print(str("[ASM] Failed to Transfer: ",asm," - ", asm.get_property("Item_Name")," [Stack Size]: ", asm.get_property("stack_size")))
        _timer_reload()

image

peter-kish commented 4 months ago

It's difficult to tell what's going on without executing the code, but i can notice that your screenshot shows three InventoryGridStacked nodes and your code references InventoryGrid nodes (at least according to the node names):

overflowInventoryGrid = guiChild.find_child("OverFlowCtrlInventoryGrid",true,true)

You also seem to cast these inventories into base Inventory nodes for reasons unclear to me:

overflowInventory = overflowInventoryGrid.inventory as Inventory

but then use them as an InventoryGridStacked:

if overflowInventory.transfer_autosplitmerge(asm, inventoryGrid):

The assertion you mentioned shouldn't really ever happen (that's why it's an assertion an not a null return). That said, none of the things I mentioned above should cause any issues as long as you're calling transfer_autosplitmerge on something that is an InventoryGridStacked or a InventoryStacked node under the hood. But maybe clearing things up a bit could narrow down where the problem is.

TreaBeardGaming commented 4 months ago

It's difficult to tell what's going on without executing the code, but i can notice that your screenshot shows three InventoryGridStacked nodes and your code references InventoryGrid nodes (at least according to the node names):

I changed the harvest_done() function as you mentioned I was doing wierd things. again still learning hehe.

func harvest_done():
    if health == 0 && !Engine.is_editor_hint():
        rootParent = find_parent("WorldEnvironmental")
        guiChild = rootParent.find_child("GUIContainer",false, true)

        tempInvGridStacked = guiChild.find_child("TempCtrlInventoryGrid",true,true).inventory
        overflowInvGridStacked = guiChild.find_child("OverFlowCtrlInventoryGrid",true,true).inventory
        playerInvGridStacked = guiChild.find_child("CtrlInventoryGrid",true,true).inventory

        #tempInv = tempInvGridStacked.inventory as Inventory
        #overflowInv = overflowInvGridStacked.inventory as Inventory
        #playerInv = playerInvGridStacked.inventory as Inventory

        var items_to_add = {0:"oak_wood_common",1:"oak_wood_magic",2:"oak_wood_rare",3:"oak_wood_epic",4:"oak_wood_legendary",5:"oak_wood_unique"}
        var item_to_rates = {0:100,1:750,2:3500,3:9500,4:25000,5:750000}

        for key in items_to_add.keys():
            tempInvGridStacked.create_and_add_item(items_to_add[key])
            var theItem = tempInvGridStacked.get_item_by_id(items_to_add[key])
            var iDropAmount = harvest_droprate((item_to_rates[key])*150,resource_amount,key)
            if iDropAmount >= 1:
                tempInvGridStacked.set_item_stack_size(theItem, iDropAmount)
                tempInvGridStacked.transfer_automerge(theItem, overflowInvGridStacked)
            else:
                tempInvGridStacked.clear()

        for am in overflowInvGridStacked.get_items():
            if overflowInvGridStacked.transfer_automerge(am, playerInvGridStacked):
                pass
        for asm in overflowInvGridStacked.get_items():
            if overflowInvGridStacked.transfer_autosplitmerge(asm, playerInvGridStacked):
                pass
        _timer_reload()

Could it be possible that I am moving the items too fast and it being made null, I recall seeing the item when I had print() lines where it would show up twice then null then show again. I'll add the print() back in and see what I get.

TreaBeardGaming commented 4 months ago

So I change code some more to first put the list of items into a var, then check if that is not empty before trying to move anything.

However the same Assertion fails: Item count is infinite!

Hmm... it seems to me like it is setting the stack_size but the item has no other properties that were specified in my item Protoset...

I'm still able to get the prototype values I set for the items in code... but why are the item nodes missing properties from the protoset?

image image

TreaBeardGaming commented 4 months ago

Do I have to specify the missing values everytime in code?

TreaBeardGaming commented 4 months ago

Think I narrowed it down to the _get_space_for_single_item() function, which returns a large negative number.

func transfer_autosplit(item: InventoryItem, destination: Inventory) -> InventoryItem:
    assert(inventory._constraint_manager.get_configuration() == destination._constraint_manager.get_configuration())
    if inventory.transfer(item, destination):
        return item

    var stack_size := get_item_stack_size(item)
    print_debug(str("This item: ",item," Has stacks: ",stack_size," / ",get_item_max_stack_size(item)))
    if stack_size <= 1:
        return null

    var item_count := _get_space_for_single_item(destination, item)
    assert(!item_count.eq(ItemCount.inf()), "Item count is infinite!")

    var count = item_count.count

    if count <= 0:
        return null

    var new_item: InventoryItem = split_stack(item, count)
    assert(new_item != null)
    assert(destination.add_item(new_item))
    return new_item

func _get_space_for_single_item(inventory: Inventory, item: InventoryItem) -> ItemCount:
    var single_item := item.duplicate()
    print_debug(single_item)
    assert(set_item_stack_size(single_item, 1))
    var count := inventory._constraint_manager.get_space_for(single_item)
    print_debug(count)
    single_item.free()
    return count

This item: oak_wood_common_0:<Node#1909800858898> Has stacks: 32 / 255 At: res://addons/gloot/core/constraints/stacks_constraint.gd:258:transfer_autosplit() oak_wood_common_0:<Node#1911042364728> At: res://addons/gloot/core/constraints/stacks_constraint.gd:281:_get_space_for_single_item() <RefCounted#-9223370125795626560> At: res://addons/gloot/core/constraints/stacks_constraint.gd:287:_get_space_for_single_item()

Looks like the node changed... is that from item.duplicate() ? or did I do something? I did check the Remote tree and Node#1909800858898 is there, but I do not see Node#1911042364728 anywhere in Remote tree

peter-kish commented 4 months ago

Unfortunately, this is not going to work 😬:

print_debug(count)

count is an instance of ItemCount, which is not a "printable" class because it is not convertable to String. It's just a helper class I'm using for some calculations and I didn't bother implementing _to_string for it. If you want to print it, you can do it like this:

print_debug(count.count)

where a value of -1 will represent infinity. Or you can insert the following ItemCount._to_string() implementation into item_count.gd:

func _to_string() -> String:
    if self.is_inf():
        return "INF"
    return str(count)

The large negative number that you're seeing is just a numeric representation of a reference to an ItemCount object (which often result in such weird numbers).

Looks like the node changed... is that from item.duplicate() ? or did I do something?

That looks fine. The item duplicate is only a temporary object that is not added to the scene and is therefore not visible in the remote scene tree.

Do I have to specify the missing values everytime in code?

Nah, that also looks fine. The properties listed when inspecting an item in the debugger are only the overridden properties. The properties that have the same value as the prototype are not listed (i.e. no need to store them if they have the same value as the prototype).

Wish I could help you more, but all I can say here is that for some reason _get_space_for_single_item thinks that the given inventory can receive an infinite number of given items. That shouldn't really be the case if you're using an InventoryGridStacked because the grid must be of a finite size and the each field can receive a finite number of items (max_stack_size is not infinite) 🤔

TreaBeardGaming commented 4 months ago

I loaded up an example and using the inventory_grid_stacked_ex_transfer.tscn

The same issue occurs, but now something new I didn't see before since I was only using 1x1 items. The 2x2 items overlap 1x1 items...


extends Control

const info_offset: Vector2 = Vector2(20, 0)

@onready var ctrl_inventory_left := $"%CtrlInventoryGridLeft"
@onready var inventory_left  :InventoryGridStacked

@onready var ctrl_inventory_right := $"%CtrlInventoryGridRight"

@onready var ctrl_inventory_temp := $"%TempCtrlInventoryGrid"
@onready var inventory_temp  :InventoryGridStacked

@onready var ctrl_inventory_overflow := $"%OverFlowCtrlInventoryGrid"
@onready var inventory_overflow :InventoryGridStacked

@onready var btn_sort_left: Button = $"%BtnSortLeft"
@onready var btn_sort_right: Button = $"%BtnSortRight"
@onready var btn_split_left: Button = $"%BtnSplitLeft"
@onready var btn_split_right: Button = $"%BtnSplitRight"

@onready var lbl_info: Label = $"%LblInfo"
@onready var itemList:ItemList=%ItemList
@onready var itemCount:SpinBox=%ItemCount
@onready var btn_add:Button=$"%BtnAdd"

func _ready() -> void:
    ctrl_inventory_left.item_mouse_entered.connect(_on_item_mouse_entered)
    ctrl_inventory_left.item_mouse_exited.connect(_on_item_mouse_exited)
    ctrl_inventory_right.item_mouse_entered.connect(_on_item_mouse_entered)
    ctrl_inventory_right.item_mouse_exited.connect(_on_item_mouse_exited)
    btn_sort_left.pressed.connect(_on_btn_sort.bind(ctrl_inventory_left))
    btn_sort_right.pressed.connect(_on_btn_sort.bind(ctrl_inventory_right))
    btn_split_left.pressed.connect(_on_btn_split.bind(ctrl_inventory_left))
    btn_split_right.pressed.connect(_on_btn_split.bind(ctrl_inventory_right))

    btn_add.pressed.connect(_on_btn_add.bind(ctrl_inventory_temp))

func _on_item_mouse_entered(item: InventoryItem) -> void:
    lbl_info.show()
    lbl_info.text = item.prototype_id

func _on_item_mouse_exited(_item: InventoryItem) -> void:
    lbl_info.hide()

func _input(event: InputEvent) -> void:
    if !(event is InputEventMouseMotion):
        return

    lbl_info.set_global_position(get_global_mouse_position() + info_offset)

func _on_btn_sort(ctrl_inventory) -> void:
    if !ctrl_inventory.inventory.sort():
        print("Warning: InventoryGrid.sort() returned false!")

func _on_btn_add(ctrl_inventory) -> void:
    inventory_temp = ctrl_inventory_temp.inventory
    inventory_overflow = ctrl_inventory_overflow.inventory
    inventory_left = ctrl_inventory_left.inventory

    var whatGive = itemList.get_selected_items()
    var whatAmount = itemCount.value
    var dropAmount = randi_range(0,1)
    assert(!whatGive.is_empty(), "Can not add nothing! Select an item!")

    dropAmount = randi_range(1,16)
    for key in whatGive:
        var createTempItem = inventory_temp.create_and_add_item(itemList.get_item_text(key))
        var theTempItem = inventory_temp.get_item_by_id(itemList.get_item_text(key))
        inventory_temp.set_item_stack_size(theTempItem, dropAmount)
        inventory_temp.transfer_automerge(theTempItem, inventory_overflow)

    if inventory_overflow.get_item_count() >= 1:
        for item in inventory_overflow.get_items():
            inventory_overflow.transfer_automerge(item, inventory_left)

    if inventory_overflow.get_item_count() >= 1:
        for item in inventory_overflow.get_items():
            inventory_overflow.transfer_autosplitmerge(item, inventory_left)

func _on_btn_split(ctrl_inventory) -> void:
    var inventory_stacked := (ctrl_inventory.inventory as InventoryGridStacked)
    if inventory_stacked == null:
        print("Warning: inventory is not InventoryGridStacked!")
        return

    var selected_items = ctrl_inventory.get_selected_inventory_items()
    if selected_items.is_empty():
        return

    for selected_item in selected_items:
        var stack_size := InventoryGridStacked.get_item_stack_size(selected_item)
        if stack_size < 2:
            return

        # All this floor/float jazz just to do integer division without warnings
        var new_stack_size: int = floor(float(stack_size) / 2)
        inventory_stacked.split(selected_item, new_stack_size)

image image image

sci-comp commented 4 months ago

Hi, I'm not sure if this is the same issue as is described by the original poster, however, I've encountered a similar error as is described in the title.

In the demo scene,

After setting up the right inventory as is shown, I click on "Split," then receive the error shown.

Screenshot 2024-05-08 025456

peter-kish commented 4 months ago

Ok, I think I managed to reproduce all three issues:

  1. "Item count shouldn'td be infinite"
  2. "Parameter 'get_viewport()' is null"
  3. Overlappint items

I'm already familiar with number 3 and I'm working on a fix. The other two I still have to investigate, but seems like there are some corner cases when the inventory has no space available, but new items are added. Hope I'll figure out a solution soon.

peter-kish commented 3 months ago

I pushed some changes to the dev_v2.4.9 branch that should fix most of the issues mentioned above. It would be great if someone could give it a try. The issues were mostly caused by the decision to make InventoryItem inherit the Node class. I had to use some ugly hacks to fix them, though after a quick check they don't seem to have any side-effects. Hopefully it will stay that way until v3.0 is out, where InventoryItems won't be nodes in the scene graph anymore. I'll do some more testing and if no issues are found, I'll include the changes in the next release.

TreaBeardGaming commented 3 months ago

I should be able to do some testing this weekend on that

TreaBeardGaming commented 3 months ago

Looks good, not seeing any errors with automerge or autosplitmerge like before.

I did see a large performance impact once inventory is full... which was because I used .sort() on the inventory after every item merge... I should probably defer that or just let the player choose when to sort...

I'll make a seperate bug for that issue

peter-kish commented 3 months ago

Update: I noticed some additional errors caused by the previous fix, so I came up with a new one and pushed it to dev_v2.4.9. It's still a hack but somewhat less ugly. Will keep testing for side effects...

peter-kish commented 3 months ago

v2.4.9 has been merged to master.