drawers icon indicating copy to clipboard operation
drawers copied to clipboard

Add controller "get" command

Open Bituvo opened this issue 3 months ago • 10 comments

Implements #96. Send a table to a drawer controller and receive information on all drawers in its network:

  1. command (string) - "get"
  2. offset (integer) - Used to paginate the results if the amount of drawers in the network exceeds drawers.CONTROLLER_MAX_COUNT. Defaults to 1.
  3. max_count (integer) - Must be between 1 and CONTROLLER_MAX_COUNT. Defaults to CONTROLLER_MAX_COUNT. Maximum amount of drawers to return.

Feel free to respond with suggestions for improvement.

Example Response

image
{
	{
		position = {x = 0, y = 9, z = 5},
		slots = {
			{count = 150, max = 1200, name = "default:bronzeblock"},
			{count = 1, max = 1200, name = "default:cobble"},
			{count = 1, max = 1200, name = "default:cobble"},
			{count = 150, max = 1200, name = "default:acacia_wood"}
		},
	},
	{
		position = {x = 1, y = 9, z = 5},
		slots = {
			{count = 150, max = 2400, name = "default:brick"},
			{count = 150, max = 2400, name = "default:acacia_leaves"}
		},
	},
	{
		position = {x = 2, y = 9, z = 5},
		slots = {
			{count = 150, max = 4800, name = "default:coral_skeleton"}
		},
	}
}

Bituvo avatar Aug 12 '25 18:08 Bituvo

Looks good. Would you please be so nice to put your API description into a file instead of only mentioning it in the PR description?

SmallJoker avatar Aug 16 '25 06:08 SmallJoker

I added documentation for the drawer controller's digilines functionality.

Bituvo avatar Aug 24 '25 00:08 Bituvo

This approach sounds fine, it limits digiline dataload. I haven't been able to read and test the actual code yet. Here are some concerns I have:

  • drawers.CONTROLLER_MAX_MATCHES should be a server setting
  • limiting dataload is fine but if a get request causes [drawers] to re-index network contents, this can still be problematic. As is, if drawers are full and there are a lot of them on the network, a server can easily be brought to a crawl by sending items to a drawercontroller -> re-index is provoked and takes a huge amount of time.

SwissalpS avatar Aug 29 '25 20:08 SwissalpS

@SwissalpS The code in the PR does not re-index a drawer network for each "get" request, it simply gets a list of connected drawers (in range) and reads their meta fields. I'll move drawers.CONTROLLER_MAX_MATCHES to settings.

Bituvo avatar Aug 29 '25 20:08 Bituvo

The code of this PR isn't using the cached index local drawer_net_index = core.deserialize(meta:get_string("drawers_table_index")) Instead it is re-scanning the entire network and sorting it on every 'get' call. This uses a lot of resources especially when a user is requesting several 'pages'.

I have concerns that this can easily be used to bring a server to a crawl.

SwissalpS avatar Aug 30 '25 07:08 SwissalpS

The keys of the cached table index are item names, and this PR aims to provide information about each individual drawer. If two drawers contain the same item, the cached table index will only store one of those drawers, whereas the "get" command would return both of them.

If the table index approach in #100 was implemented then this PR could use it instead, but for now, the cached table index doesn't provide enough information for the "get" command.

Bituvo avatar Aug 30 '25 14:08 Bituvo

Not merging until the cache topic is resolved.

@SwissalpS As of now, the "drawers_table_index" metadata is only helpful when inserting items into the controller. It improves the lookup speed if there's already a stack of the same name somewhere in the network. The index is not updated when removing nodes, or when interacting with specific drawers individually. What would you suggest to change? To re-evaluate the network upon node place/remove? That would surely introduce more code complexity.

One improvement that I would consider is to replace core.find_nodes_in_area with a large VManip to traverse large networks more efficiently.

SmallJoker avatar Aug 31 '25 11:08 SmallJoker

When re-writing this mod to not use entities to store data I found that a lot of time is spent when serializing the network table. I was looking at options to also fix the updating issue. I'm not sure from memory if I ever finished testing which method would be fastest. My recollection hints that I was going to go for an in memory cache that is rebuilt after every reboot as areas get loaded. Similar to how [technic.plus] indexes it's networks. Too bad I haven't had time to continue that project though.

Short: I suggest caching in memory and updating on tube take/put events as well as manual take/put actions. An initial indexing to build the cache using VManip sounds reasonable. The way drawer controllers currently work, the max volume is known. This approach would probably be faster but I don't know if the RAM-burden is acceptable.

SwissalpS avatar Aug 31 '25 14:08 SwissalpS

I implemented a DFS with a VoxelManip instead of recursive find_nodes_in_area. This should make it faster, but I don't think the list of connected drawers should be cached. Drawers can be placed or removed by a player, leading to a cache invalidation that is difficult to solve without complicated on_construct and on_destruct calls. A similar situation arises when storing items in drawers without running them by the drawer controller; I have to agree with @SmallJoker here.

The drawer controller should know if any of these events have occurred when a "get" command is sent to it. This DFS implementation should significantly speed up the process of finding all connected drawers within range, not only for the "get" command but for drawer indexing as well. I believe that further changes to how the drawer index is interpreted are out of the scope of this PR.

Bituvo avatar Aug 31 '25 20:08 Bituvo

It would be great to benchmark at least the time used for version before and after the VManip and other changes to make sure these actually do improve the performance time-wise.

SwissalpS avatar Sep 03 '25 05:09 SwissalpS