yl_speak_up: on server join, I get every other player's detached inventory for NPCs #6235

New Issue

whosit · 2024-02-05T14:11:24Z

whosit commented

2024-02-05 14:11:24 +00:00

Detached inventories should be sent only to players they belong to, and ideally only on demand.

![image](/attachments/d0855f4f-b47f-471f-a39f-5346e732a753) Detached inventories should be sent only to players they belong to, and ideally only on demand.

image.png

137 KiB

whosit added the

1. kind/bug

label 2024-02-05 14:11:31 +00:00

Sokomine was assigned by whosit

2024-02-05 14:13:40 +00:00

flux commented

2024-02-05 19:50:46 +00:00

there's a way to restrict sending detached inventory actions to a single player:

https://github.com/minetest/minetest/blob/master/doc/lua_api.md?plain=1#L6243-L6247

however, there's no way to say that some small set of players should have access. is it correct to limit NPC inventories this way?

there's a way to restrict sending detached inventory actions to a single player: https://github.com/minetest/minetest/blob/master/doc/lua_api.md?plain=1#L6243-L6247 however, there's no way to say that some small set of players should have access. is it correct to limit NPC inventories this way?

whosit commented

2024-02-05 20:40:15 +00:00

Should be possible to just not send it until player is accessing NPC that contains it?
Or I'm missing something?

Should be possible to just not send it until player is accessing NPC that contains it? Or I'm missing something?

whosit commented

2024-02-05 20:58:12 +00:00

When I'm joining main, I get 368 detached inventories, obviously most of those players are not online and don't own NPCs

flux commented

2024-02-06 01:05:54 +00:00

i'm not really sure what the relationship between the detached inventory and the NPC is though - the NPC is owned by a particular player, but i think the inventory is NPC-specific, and anyone who has admin-access needs to have access to the NPC's inventory as well. and we can't use e.g. my FakeInventory from futil because you can't make that show up in a formspec. but maybe the inventory could normally be stored in a FakeInventory (or serialized inventory) and only reify when someone actually interacts with the NPC? but that's not a great solution either, because there's no reliable signal when a formspec is closed, and things get complicated if multiple people try to interact with the NPC at the same time.

flux commented

2024-02-06 01:06:53 +00:00

I get 368 detached inventories

how many node inventories do you get? there's tons of those around spawn. 368 is a lot, but not absurd for a one-time send.

> I get 368 detached inventories how many node inventories do you get? there's tons of those around spawn. 368 is a lot, but not absurd for a one-time send.

whosit commented

2024-02-06 04:56:05 +00:00

c7f9fc81c3/api/api_trade_inv.lua (L207)
It's this one, it's an inventory of a specific player, and number of inventories will just keep growing as players join, until next server restart.

Fix is: send it only to player that it it belongs to, and remove when player leaves?

https://gitea.your-land.de/your-land/yl_speak_up/src/commit/c7f9fc81c39d89830c37994b484a4afaa466d76a/api/api_trade_inv.lua#L207 It's this one, it's an inventory of a *specific player*, and number of inventories will just keep growing as players join, until next server restart. Fix is: send it only to player that it it belongs to, and remove when player leaves?

flux commented

2024-02-06 18:03:40 +00:00

c7f9fc81c3/api/api_trade_inv.lua (L207)
It's this one, it's an inventory of a specific player, and number of inventories will just keep growing as players join, until next server restart.

Fix is: send it only to player that it it belongs to, and remove when player leaves?

oh! yeah that's definitely what should be happening. it might even be better implemented as a player inventory.

> https://gitea.your-land.de/your-land/yl_speak_up/src/commit/c7f9fc81c39d89830c37994b484a4afaa466d76a/api/api_trade_inv.lua#L207 > It's this one, it's an inventory of a *specific player*, and number of inventories will just keep growing as players join, until next server restart. > > Fix is: send it only to player that it it belongs to, and remove when player leaves? oh! yeah that's definitely what should be happening. it might even be better implemented as a player inventory.

Sokomine commented

2024-02-06 19:06:18 +00:00

Really sounds like an error. I ought to limit at least the detached player inventory to the player himself. That inv is needed for giving items to the NPC, getting items, setting up trades etc.

It also ought to be removed when the player leaves.

The other detached inventory is that of the NPC. It gets created when the NPC gets talked to - and then cached. Removing that again is more difficult due to the reasons flux pointed out. I might be able to check if someone is still talking to the NPC and clean unused inventories up occasionally. Will have to check that out.

Thanks for discovering these errors!

Really sounds like an error. I ought to limit at least the detached player inventory to the player himself. That inv is needed for giving items to the NPC, getting items, setting up trades etc. It also ought to be removed when the player leaves. The other detached inventory is that of the NPC. It gets created when the NPC gets talked to - and then cached. Removing that again is more difficult due to the reasons flux pointed out. I might be able to check if someone is still talking to the NPC and clean unused inventories up occasionally. Will have to check that out. Thanks for discovering these errors!

👍 1

AliasAlreadyTaken commented

2024-02-07 16:20:18 +00:00

I'd like to point towards similar problems of "inventory exchange" for yl_trade and other mods that require detached inventories.

Was this not already once solved in smartshops?

I'd like to point towards similar problems of "inventory exchange" for yl_trade and other mods that require detached inventories. Was this not already once solved in smartshops?

whosit commented

2024-02-07 17:07:44 +00:00

There's a problem of "where" to store the data.

You can store it in players, inflating playermeta size and I think it also increases server lag if DB backend is not async because it's serialized and saved constantly (this was the "5-second-tick" problem I was trying to diagnose a while ago).
You can store it in blocks, and even hide it from clients - this works best for shops.
Or you can come up with some other storing option outside of the world and put your items in detached invs. This may be the best option, but needs careful management.

In my collectible_chest mod I have some client-side warnings because I create detached inv and then send the formspec with it on the same step, but either packers arrive out of order, or client processes them differently, but sometimes it prints a warning that inv does not exist, but then it appears inside the formspec just fine...

There's a problem of "where" to store the data. - You can store it in players, inflating playermeta size and I think it also increases server lag if DB backend is not async because it's serialized and saved constantly (this was the "5-second-tick" problem I was trying to diagnose a while ago). - You can store it in blocks, and even hide it from clients - this works best for shops. - Or you can come up with some other storing option outside of the world and put your items in detached invs. This may be the best option, but needs careful management. In my collectible_chest mod I have some client-side warnings because I create detached inv and then send the formspec with it on the same step, but either packers arrive out of order, or client processes them differently, but _sometimes_ it prints a warning that inv does not exist, but then it appears inside the formspec just fine...

flux commented

2024-02-07 23:17:55 +00:00

Was this not already once solved in smartshops?

the use of detached inventories in smartshop was temporary, but it happened every time the shops loaded. it was part of the logic that controls the shop's color and updates the infotext. the big problem was that every action on the detached inventory was sent to every client, even if the detached inventory was destroyed before anyone could actually interact with it. it got replaced with the fake inventory instead. that's not applicable here, because the NPCs actually need to have an inventory that players can interact with.

> Was this not already once solved in smartshops? the use of detached inventories in smartshop was temporary, but it happened every time the shops loaded. it was part of the logic that controls the shop's color and updates the infotext. the big problem was that every action on the detached inventory was sent to every client, even if the detached inventory was destroyed before anyone could actually interact with it. it got replaced with the fake inventory instead. that's not applicable here, because the NPCs actually need to have an inventory that players can interact with.

👍 1

flux commented

2024-02-07 23:24:21 +00:00

it also increases server lag if DB backend is not async because it's serialized and saved constantly

the player inventory is stored properly in a database table and not as a serialized blob, i'm don't think this is applicable.

You can store it in blocks, and even hide it from clients - this works best for shops.

you can hide metadata from clients, but not node inventories, so far as i know. i've tried.

> it also increases server lag if DB backend is not async because it's serialized and saved constantly the player inventory is stored properly in a database table and not as a serialized blob, i'm don't think this is applicable. > You can store it in blocks, and even hide it from clients - this works best for shops. you can hide metadata from clients, but not node inventories, so far as i know. i've tried.

whosit commented

2024-02-07 23:48:22 +00:00

it also increases server lag if DB backend is not async because it's serialized and saved constantly

the player inventory is stored properly in a database table and not as a serialized blob, i'm don't think this is applicable.

This was 100% the case with sqlite backend for me - because I turned on async option for sqlite and lag spike was gone.
There was also a patch that does same for postgresql, but it was not merged. Still want to see if there's a difference for postgres too.

You can store it in blocks, and even hide it from clients - this works best for shops.

you can hide metadata from clients, but not node inventories, so far as i know. i've tried.

I mean, you can just serialize whatever into a block and then generate detached inv to show it. What I was trying to say is that the data will be available when the block is loaded - so it makes sense to store it in a block.
Sure, you can't make private an inventory that you want to be processes client-side...

> > it also increases server lag if DB backend is not async because it's serialized and saved constantly > > the player inventory is stored properly in a database table and not as a serialized blob, i'm don't think this is applicable. This was 100% the case with sqlite backend for me - because I turned on async option for sqlite and lag spike was gone. There was also a patch that does same for postgresql, but it was not merged. Still want to see if there's a difference for postgres too. > > You can store it in blocks, and even hide it from clients - this works best for shops. > > you can hide metadata from clients, but not node inventories, so far as i know. i've tried. I mean, you can just serialize whatever into a block and then generate detached inv to show it. What I was trying to say is that the data will be available when the block is loaded - so it makes sense to store it in a block. Sure, you can't make private an inventory that you want to be processes client-side...

flux commented

2024-02-08 19:15:36 +00:00

There was also a patch that does same for postgresql, but it was not merged. Still want to see if there's a difference for postgres too.

i think alias tried that, but we didn't notice anything different

> There was also a patch that does same for postgresql, but it was not merged. Still want to see if there's a difference for postgres too. i think alias tried that, but we didn't notice anything different

whosit commented

2024-02-15 21:32:31 +00:00

should be fixed here:
691301019b...db2abf76fe

should be fixed here: https://gitea.your-land.de/Sokomine/yl_speak_up/compare/691301019bce8c6c5c928425f404fbcd87524e6a...db2abf76fe68bfed2b7b0067be83ae657c0e9bd9

whosit added the

4. step/ready to QA test

label 2024-02-15 21:32:47 +00:00

AliasAlreadyTaken added this to the 1.1.123 milestone 2024-02-16 05:07:55 +00:00

AliasAlreadyTaken commented

2024-02-16 08:50:05 +00:00

Unfortunately we can't use the latest version of NPCs due to the z fighting issue on the capes: your-land/bugtracker#6238

AliasAlreadyTaken added the

4. step/blocked

label 2024-02-16 08:50:11 +00:00

whosit commented

2024-02-16 10:01:54 +00:00

Unfortunately we can't use the latest version of NPCs due to the z fighting issue on the capes

Actual bug is this one, and I think it may be fixed now?
#6171

> Unfortunately we can't use the latest version of NPCs due to the z fighting issue on the capes Actual bug is this one, and I think it may be fixed now? https://gitea.your-land.de/your-land/bugtracker/issues/6171

👍 1

AliasAlreadyTaken removed the

4. step/blocked

label 2024-02-17 06:05:59 +00:00

AliasAlreadyTaken commented

2024-02-17 06:06:23 +00:00

QA

Back to testing ... whosti: could you wireshark the testserver and say whether it's solved?

QA Back to testing ... whosti: could you wireshark the testserver and say whether it's solved?

whosit commented

2024-02-17 06:52:24 +00:00

This is all detached inventories I get on join to test:

trash
airutils_whosit_977061
unified_trash
saddlebag_inventory
whosit_armor
yl_speak_up_player_whosit
whosittrash_review
whosit_bags
whositrefill

9 packets total

This is all detached inventories I get on join to test: - trash - airutils_whosit_977061 - unified_trash - saddlebag_inventory - whosit_armor - yl_speak_up_player_whosit - whosittrash_review - whosit_bags - whositrefill 9 packets total

whosit added the

ugh/QA OK

label 2024-02-17 06:53:47 +00:00

Bla commented

2024-02-17 13:07:34 +00:00

9 packets total

someone should disable that "whosit" mod 😜

> 9 packets total someone should disable that "whosit" mod 😜

flux commented

2024-02-18 05:59:10 +00:00

someone should disable that "whosit" mod

i don't know what the other mods are, but whosittrash_review should certainly be using a player inventory instead of a detached one.

> someone should disable that "whosit" mod i don't know what the other mods are, but `whosittrash_review` should certainly be using a player inventory instead of a detached one.

AliasAlreadyTaken commented

2024-02-18 09:01:34 +00:00

What's the difference between trash and unified_trash and trash_review?

I also don't see why bags need a detached inventory, its not that anyone else would interact with them

Same goes for armour?

What's the difference between trash and unified_trash and trash_review? I also don't see why bags need a detached inventory, its not that anyone else would interact with them Same goes for armour?

whosit commented

2024-02-18 09:27:30 +00:00

What's the difference between trash and unified_trash and trash_review?

I also don't see why bags need a detached inventory, its not that anyone else would interact with them

Same goes for armour?

I suggest we continue this here:
#6313

> What's the difference between trash and unified_trash and trash_review? > I also don't see why bags need a detached inventory, its not that anyone else would interact with them > > Same goes for armour? I suggest we continue this here: https://gitea.your-land.de/your-land/bugtracker/issues/6313

👍 1

flux added

5. result/fixed

and removed

4. step/ready to QA test

labels 2024-03-28 21:52:39 +00:00

Sokomine was unassigned by flux

2024-03-28 21:54:27 +00:00

flux commented

2024-03-28 21:54:56 +00:00

i don't see any more NPC inventories (or at least not tons of them)

flux closed this issue