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

transfer_autosplitmerge fail on full inventory #201

Closed
TreaBeardGaming opened this issue Apr 27, 2024 · 17 comments
Closed

transfer_autosplitmerge fail on full inventory #201

TreaBeardGaming opened this issue Apr 27, 2024 · 17 comments
Labels
bug Something isn't working
Milestone

Comments

@TreaBeardGaming
Copy link

TreaBeardGaming commented Apr 27, 2024

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
Copy link
Author

TreaBeardGaming commented Apr 27, 2024

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
Copy link
Owner

peter-kish commented Apr 27, 2024

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

@peter-kish peter-kish reopened this Apr 27, 2024
@peter-kish peter-kish added the bug Something isn't working label Apr 27, 2024
@peter-kish peter-kish added this to the 2.4.8 milestone Apr 27, 2024
@TreaBeardGaming
Copy link
Author

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(!item_count.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
Copy link
Owner

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.

@peter-kish peter-kish modified the milestones: 2.4.8, 2.4.9 May 1, 2024
@TreaBeardGaming
Copy link
Author

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
Copy link
Author

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
Copy link
Author

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

@TreaBeardGaming
Copy link
Author

TreaBeardGaming commented May 2, 2024

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
Copy link
Owner

peter-kish commented May 3, 2024

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
Copy link
Author

I loaded up an example and using the inventory_grid_stacked_ex_transfer.tscn

  • created two CtrlInventoryGridEx
  • created two InventoryGridStacked
  • Added an item list, item count, BtnAdd

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
Copy link

sci-comp commented May 8, 2024

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Author

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

@TreaBeardGaming
Copy link
Author

TreaBeardGaming commented May 18, 2024

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

  • Item Count Shouldn't be Infinite: ✅ Appears to be resolved in testing
  • Overlapping Item slots of varied sizes (1x1/1x2/etc): ✅ Appears to be resolved in testing
  • Parameter 'get_viewport' is null: ⁉ I'm not sure how to replicate or test this

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
Copy link
Owner

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
Copy link
Owner

v2.4.9 has been merged to master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants