expressobits / inventory-system

Modular inventory system for godot 4 with nodes, compatible with multiplayer, separate logic from the UI, Using items as separate resources.
MIT License
501 stars 32 forks source link

Allow for dynamic item values in Slot #54

Closed Lodugh closed 11 months ago

Lodugh commented 1 year ago

I am mainly thinking of making something like item durability possible. Maybe a dictionary that is always passed along with the item.

scriptsengineer commented 1 year ago

I am mainly thinking of making something like item durability possible. Maybe a dictionary that is always passed along with the item.

Simple, create a code that extends the slot (SlotWithDurability or SlotWithDynamicInformation) that contains durability information. If you're going to have multiple items in a slot with durability (which I don't recommend as a game design) creating a list can solve it, and that could be part of the core at some point.

Update 26/11 - What's missing

Added SlotItem and integrated

Custom properties are now missing

Lodugh commented 1 year ago

Well it also needs some adjustments in the networking. I think this would be essential enough that it should be part of the core. Just a dictionary that can be used for any kind of dynamic data and also gets synchronized over the network. Maybe the dictionary could actually just contain all passed data: ID and amount plus custom dynamic variables and gets passed just like that.

scriptsengineer commented 1 year ago

Well it also needs some adjustments in the networking. I think this would be essential enough that it should be part of the core. Just a dictionary that can be used for any kind of dynamic data and also gets synchronized over the network. Maybe the dictionary could actually just contain all passed data: ID and amount plus custom dynamic variables and gets passed just like that.

Honestly in the case of the network, if there is an item only when there is quality I would optimize to send two ints because of the bandwidth, but this should certainly be an option for whoever is implementing it. Yes, we can have that in the core, I think it's very valid.

scriptsengineer commented 1 year ago

Idea 1 To solve the problem: In slot instead of having InventoryItem and amount now has a SlotItem and amount The SlotItem contains an InventoryItemand dynamic properties placed by a dictionary.

Cons:

Pros

Before:

@tool
extends Resource
class_name Slot

@export var item : InventoryItem
@export var amount := 0
@export var max_stack := -1

After

@tool
extends Resource
class_name Slot

@export var item : SlotItem
@export var amount := 0
@export var max_stack := -1

in SlotItem:

@tool
extends Resource
class_name SlotItem

@export var item : InventoryItem
@export var properties : Dictionary
scriptsengineer commented 1 year ago

Idea 2 to solve problem Simply add an additional dictionary to the slot

@tool
extends Resource
class_name Slot

@export var item : InventoryItem
@export var amount := 0
@export var max_stack := -1
@export var properties : Dictionary
Lodugh commented 1 year ago

Well I think understand-ability should always be a very high priority with these kind of plugins so I would go for the first one I think. However I dont really have the whole picture of it before me and how it integrates with the rest so I would mainly rely on your evaluation.

levidavidmurray commented 1 year ago

I may be off base here, but my gut's telling me that there needs to be a better separation of runtime and config data. In my mind, "dynamic item values" are runtime data, an "inventory slot" is runtime data, InventoryItem is config data, Recipe is config data.

Regarding idea 2 Currently when an inventory is initialized, it creates 32 blank slots (item is null)—meaning an item is inherently transient to a Slot. So, I don't think it makes sense to store item specific information (e.g. durability) within the slot. Those properties are logically tied to the item, not to the slot.

Regarding idea 1 I personally think you're on the right track with SlotItem (I'm not sure the naming is suitable for what it represents). Some kind of intermediary object is required to map to an item template to the game world, and I don't think it should be the slot. If an item is dropped, those custom properties should be dropped with it, which shows that the properties shouldn't exist on the slot.

Below is a rough proposal for how I might go about restructuring the current classes. It's worth noting that this would require stacked items of the same ItemResource to have matching properties (although workarounds could obviously implemented).

image

scriptsengineer commented 1 year ago

I may be off base here, but my gut's telling me that there needs to be a better separation of runtime and config data. In my mind, "dynamic item values" are runtime data, an "inventory slot" is runtime data, InventoryItem is config data, Recipe is config data.

Regarding idea 2 Currently when an inventory is initialized, it creates 32 blank slots (item is null)—meaning an item is inherently transient to a Slot. So, I don't think it makes sense to store item specific information (e.g. durability) within the slot. Those properties are logically tied to the item, not to the slot.

Regarding idea 1 I personally think you're on the right track with SlotItem (I'm not sure the naming is suitable for what it represents). Some kind of intermediary object is required to map to an item template to the game world, and I don't think it should be the slot. If an item is dropped, those custom properties should be dropped with it, which shows that the properties shouldn't exist on the slot.

Below is a rough proposal for how I might go about restructuring the current classes. It's worth noting that this would require stacked items of the same ItemResource to have matching properties (although workarounds could obviously implemented).

image

I'm following idea 2, but I'm still undecided due to the cons it shows

There is still an area of doubt that changes my mind How should slotItem behave when a slot is empty? Should there be a slotitem with no defined item that will be immobile?

If it must be something moved, when we share the item, should it be duplicated?

scriptsengineer commented 1 year ago

I decided to go with solution 2, don't rename anything yet, as I intend to do this in another interaction, (when changing from core to cpp #83)

What's missing: (Put it in the initial message too)

Added SlotItem and integrated

Custom properties are now missing