don-tnowe / godot-wyvernbox-inventory

Action RPG-focused inventory system for Godot 3 and 4
MIT License
97 stars 4 forks source link

adds GroundItemStackView2D/3D UUID / adds GroundItemManager load_from_array_differential #6

Closed Mazianni closed 1 year ago

Mazianni commented 1 year ago

Adds a differential form of load_from_array to do multiplayer sync without creating/destroying everything via load_from_array.

Probably not the best (or cleanest) implementation - feedback and criticism welcome.

don-tnowe commented 1 year ago

Thanks for the draft!

As UIDs are created as numbers, what's the reason to store them as strings (and not even StringNames)? I'd keep it as ints everywhere unless there's an explicit benefit.

Diff-based load_from_array should IMO just be the default way to load. My main concern, though, is not how items get deleted too much but how it's just too much data passed around by the server very often. Ideally, it's just an RPC that's like delete_item(item_uid : int). Or avoid the UID system entirely and just pass rounded-up position and all item data, which is still less stuff than copying the entire array!

randi() might be mostly safe but collisions still happen. Between players too. So I'd say the giver of UIDs should be centralized, checking for conflicts with already-in-use IDs and returning a valid one.

Mazianni commented 1 year ago

At one point, I was going to use another UUID generator but decided against that, but never undid my string assumption.

I'll see what I can do to add some support in for event-based calls. That is (generally) much better than this consistent hard-sync I've been relying on.

I'll add an array to keep track of UUIDs and make sure there's no collisions.

Mazianni commented 1 year ago

I think I will be widening this a fair amount to include giving item_stacks some form of UUID for inventory comparison and manipulation, not just ground items.

don-tnowe commented 1 year ago

Good idea, but thing to note: stacks exiting inventories are freed, so when added to another it's a different instance. Not doing this caused some issues, and rewriting it now might be a pain :/

Mazianni commented 1 year ago

Yeah, I noticed that. I'll have to figure something out in that regard (maybe just a signal to trigger a hard sync to clients, not sure yet).

Mazianni commented 1 year ago

Good idea, but thing to note: stacks exiting inventories are freed, so when added to another it's a different instance. Not doing this caused some issues, and rewriting it now might be a pain :/

What issues did this cause, exactly?

Also requesting some more clarification on 'exiting an inventory'. My tests didn't show the item being deleted if it's swapped between inventories or anything, which is my primary concern.

don-tnowe commented 1 year ago

That is, when an item is transferred to any inventory - it creates a new instance, rather than giving the destination inventory a reference to the old stack object. The old object gets freed as it's now unused. So moving an item into another inventory should make a new UID assigned since a new ItemStack is created

This had something to do with auto-adding items, or combining stacks, or that but only when shift-click-transfering, something like that.

Mazianni commented 1 year ago

That is, when an item is transferred to any inventory - it creates a new instance, rather than giving the destination inventory a reference to the old stack object. The old object gets freed as it's now unused. So moving an item into another inventory should make a new UID assigned since a new ItemStack is created

This had something to do with auto-adding items, or combining stacks, or that but only when shift-click-transfering, something like that.

That's interesting. That is not the behavior I have seen while moving a test item between two inventories. Insofar as I can see, the StackItem isn't deleted or replaced; the ref remains exactly the same, so I can only assume that the object is being preserved.

Mazianni commented 1 year ago

I've done some further thinking between bouts of bedrest lately. What are your feelings on the inclusion of this UUID generator?

I've realized that inventories will also need some form of UUID as well to properly keep track movement and transfers.

don-tnowe commented 1 year ago

Ideally, I'd like to avoid external dependencies, and for our purpose it seems to be the same as just using randi(). And we'll still need to check collisions.

We could also just see how another inventory addon, https://github.com/expressobits/inventory-system, handles netcode - seems to have a multiplayer demo. Or you could just, y'know, just use that instead, entirely and leave this buggy and untested mess of an addon to me :/

Mazianni commented 1 year ago

Well, speaking personally - I really do like how Wyvernbox handles things. I did look into that addon a little, but I've already heavily integrated Wyvernbox into my project and it'd feel like backtracking to rip that out and start over.

Please don't feel discouraged! Every project has it's bugs and faults.

If you genuinely want me to use another system, I will, if i've somehow come off as rude or somehow offended you.