dennisjenkins75 / digiline_craftdb

Minetest mod for a digiline queryable database holding all regular and technic crafting recipes.
GNU General Public License v3.0
2 stars 2 forks source link

Is there actual reason to return request in response? #6

Closed S-S-X closed 3 years ago

S-S-X commented 3 years ago

I mean this and similar stuff around it: https://github.com/dennisjenkins75/digiline_craftdb/blob/1e9e8815c1d13fa2d39127e5b4cf8d91e22835ef/init.lua#L33-L37

Would it be better to just directly return what CraftDB:find_all_matching_items / CraftDB:get_all_recipes returns instead of adding 2 more tables?

If this is just for pagination I think pagination is not really needed as you're working with relatively small datasets anyway but still would have to test to say for sure...

Just feeling as there probably will not be that many results and actually searching with pagination probably adds way more overhead than processing results within luac because single iteration requires environment initialization, mem serialization, 2 way message delivery, new environment initialization with mem serialization again and so on. Each page adds single iteration.

~If however keeping request in response without copying it I think it would be good to make sure that luac / digilines it is not delivering reference to original table but copies it for delivery.~ Luac seems to handle this already.

S-S-X commented 3 years ago

To be clear, not direct request to change that but more open up discussion about usefulness of pagination and having request included in response. And also to make sure that one cannot keep references to previous environment after queue has been processed.

dennisjenkins75 commented 3 years ago

The reason why I included the original request in the response for each message was:

"get_all_recipes" - Imagine that you ask your universal autocrafter to make "technic:hv_solar_array". Your LUAC the fetches the recipe for this (and likely caches the result in mem.cache[item]). Ok, it then iterates through all of the items in that recipe, and checks to see if it has them in the lastest inventory from your mithril chests (or whatever inventory source you have). You then discover that you're missing everything. So you need to fetch recipes for carbon plates, compressed plates, hv_transformer, hv_cable, mv_solar_array. Since digiline messages are purely async, and there are no "promises" or "futures", you need a way to know that a given recipe response was for your original request. I assumed that one would just fire off a recipe request for every missing item all at once (5 in this case, but maxes at 9).

However, returning the original request might not be needed since the "output" should list the item that your requesting. Its not guaranteed by the API that 'outputs[]' would include the item, but we could assume it to be true. Worse case, your own LUAC code forever requests the same recipe and gets a penalty.

One area for improvement might be to allow the fetching of multiple recipes at once. In the request, "item" would become "items", as an indexed table of items to fetch recipes for, and the response would include all of them, up to some limit.

For both messages, I wanted to cover the case where one or more luacs are concurrently requesting both recipes and item searches. When handling a response, a luac would need to know if it was meant for them, and if it was a search request, what the search string was (a single luac might fire off multiple searches in one cycle also).

As for "find_all_matching_items": imagine an interactive digistuff:touchscreen, presenting the user with a free-form text search field (like current inventory screen),a "search" button, 5x4 grid of responses (item names and images), and a free-form text input field for typing in a quantity and buttons for "make it" and "cancel". Now imagine that the user types in "technic" and clicks "search". There are way more than 20 responses.

I was unaware that "keeping a reference to the original request" was a problem, but if it is, we could implement a deep-copy. It was always my intention that the caller could add extra data to the request table, like a "_my_id", and since that would get echoed back, they could just check that value to see if the response really was for them. Or, I suppose, such advanced machines should just use multiple craftdb's w/ different channel names.

I suppose that the problems with "find_all_matching_items" could be addressed by creating an additional node that acts like the touch0screen ordering UI. Instead of the user needing to code up the LUAC to create their own UI and drive it (since 45 formspec widgets can't be done in one luac cycle anyway), they would just use this node, give it a channel name, and it would handle all of the UI of acting like an inventory screen for "ordering" some quantity of some item. It would just send a message like "{ item = 'technic:foo', qty = 5 }" to whatever channel name it was assigned when the user makes a final selection and clicks the "submit" button.

I still need to reconcile these messages w/ the need to look up group items. Ex: "default:pick_stone" needs "group:wood". Should the user ask for "group:wood" via get_all_recipes (its not really a recipe) or find_all_matching_items (better fit imho).

S-S-X commented 3 years ago

Since digiline messages are purely async

No, they are not. It just looks like that but some are actually using this fact to build a lot more efficient machines. edit. Clarification: process for firing events and delivering messages is asynchronous FIFO

you need a way to know that a given recipe response was for your original request

This is relevant only if you do connect multiple lua controllers to single craftdb node, and even then it is possible to flag pending request on luac that sends recipe request and only handle response if flag is up.

One area for improvement might be to allow the fetching of multiple recipes at once. In the request, "item" would become "items", as an indexed table of items to fetch recipes for, and the response would include all of them, up to some limit.

This sounds good, Limit could probably be leaved out and let luac timeout handle it if user did something he should not do, if collecting/transforming data is very complex then most probably some indexes/caching could be added.

When handling a response, a luac would need to know if it was meant for them, and if it was a search request, what the search string was (a single luac might fire off multiple searches in one cycle also).

All messages are ordered and responses will always come in same order as messages are sent. It would also be good to discourage connecting multiple luacs to same backbone unless player who does so also does clearly understand how system works, if he does understand then request in response is also not needed to identify response.

imagine an interactive digistuff:touchscreen, presenting the user with a free-form text search field (like current inventory screen),a "search" button, 5x4 grid of responses (item names and images), and a free-form text input field for typing in a quantity and buttons for "make it" and "cancel". Now imagine that the user types in "technic" and clicks "search". There are way more than 20 responses.

This is not a problem, especially not a problem of craftdb. Some hundred responses should not be problem at all. More about this below (advanced touchscreen).

I was unaware that "keeping a reference to the original request" was a problem

I think this point is actually invalid as lua controller at least ensures that message sent out is safe to modify as it is copy instead of original, I think it probably does that also for messages it receives because if it wont this could possibly be security issue depending on how messages are handled by other mods. Did not test yet but should be easy to verify. Anyway if this is problem then it should be actually fixed in lua controller instead.

such advanced machines should just use multiple craftdb's w/ different channel names.

Yes, it is always better to fire less events for lua controllers because even if you will not do anything in your own lua code event will still cause significant amount of work launching new sandbox environment. If this can be encouraged by not allowing easy identification without actually adding it to luac program it would be only good.

This of course wont prevent connecting all luacs and craftdbs to same line but at least makes it less likely and more clearly shows that it is better choice than having everything connected to single backbone.

I suppose that the problems with "find_all_matching_items" could be addressed by creating an additional node that acts like the touch0screen ordering UI. Instead of the user needing to code up the LUAC to create their own UI and drive it (since 45 formspec widgets can't be done in one luac cycle anyway),

I would have very strong objections for this proposal because it would be attempt to work around symptoms of problem with unnecessary complexity without bringing anything new instead of targeting actual problem.

With current touchscreen for recipe list one should use dropdown or textlist instead of individual buttons. I do think this is not something that craftdb or any other mod should handle but instead should be fixed in touchscreen mod or advanced touchscreen used instead of old touchscreen. Also advanced touchscreen should be able to do this and more without any problems. Currently at pandorabox recipe is disabled because it can possibly cause some problems and needs testing.

I still need to reconcile these messages w/ the need to look up group items. Ex: "default:pick_stone" needs "group:wood". Should the user ask for "group:wood" via get_all_recipes (its not really a recipe) or find_all_matching_items (better fit imho).

As group is not item and it also is not recipe I think find_all_matching_items is best by how is sounds... Without looking at code I'd imagine that function will take in some kind of more or less free-form query and return list of items that match query parameters. Specifically I would not expect it to take same type as input that it returns. Btw for autocrafter there's also PR open adding support for item groups.

dennisjenkins75 commented 3 years ago

Since digiline messages are purely async

No, they are not. It just looks like that but some are actually using this fact to build a lot more efficient machines. edit. Clarification: process for firing events and delivering messages is asynchronous FIFO

Is this published in documentation (https://mesecons.net/luacontroller/) somewhere? This is a hidden detail; its not guaranteed in the API contract. The API presents digiline messages as if they were asynchronous. Therefore, it could change.

Consider a LUAC with the following code:

  digiline_send('craftdb', { command = 'list', item = 'moreores:pick_mithril' })
  digiline_send('craftdb', { command = 'list', item = 'moreores:axe_mithril' })
  digiline_send('craftdb', { command = 'list', item = 'moreores:sword_mithril' })
  digiline_send('craftdb', { command = 'list', item = 'moreores:shovel_mithril' })

Will all 4 messages be sent before the craftdb (or any other digiline node on the network) processes them, or will the sending LUAC be suspended immediately after the first message is sent, all receivers run, then execution of the LUAC resumes, sending the second message, etc... This is what I meant by "asynchronous". If you've ever had the misfortune of writing low-level win32 API code, this is the difference between 'SendMessage()' and 'PostMessage()'.

One area for improvement might be to allow the fetching of multiple recipes at once. In the request, "item" would become "items", as an indexed table of items to fetch recipes for, and the response would include all of them, up to some limit.

This sounds good, Limit could probably be leaved out and let luac timeout handle it if user did something he should not do, if collecting/transforming data is very complex then most probably some indexes/caching could be added.

A LUAC will "overheat" if it attempts to store > 100KiB in "mem" (when serialized) (this limit is published). There is also a limit of 50KiB max digiline message size. I've not done an exhaustive search for craftable items w/ the most recipes, but I once saw one (not on pandora box) w/ 120+ ways to make it.

I will modify the API so that one can request crafting recipes for multiple items at the same time. This should help optimize for sending fewer messages, especially when a LUAC is recursively looking up recipes for a complex build.

All messages are ordered and responses will always come in same order as messages are sent.

Is this guaranteed in any published API documentation (and not just a hidden detail buried in raw lua code)?

It would also be good to discourage connecting multiple luacs to same backbone unless player who does so also does clearly understand how system works, if he does understand then request in response is also not needed to identify response.

Ok, that's fair.

imagine an interactive digistuff:touchscreen, presenting the user with a free-form text search field (like current inventory screen),a "search" button, 5x4 grid of responses (item names and images), and a free-form text input field for typing in a quantity and buttons for "make it" and "cancel". Now imagine that the user types in "technic" and clicks "search". There are way more than 20 responses.

This is not a problem, especially not a problem of craftdb. Some hundred responses should not be problem at all. More about this below (advanced touchscreen).

The regular inventory system paginates on the screen b/c its not practical to show 3,000+ craftable items all at once. Imagine if the user searches for the empty string or for "%s". What should craftdb return? With no limit, it would return all the list of all 3k+ items, with image filenames, which is pretty big. Even if this does not exceed the 100k limit, someday pandora box might add more mods, with more nods, and going over this limit.

BTW, as of right now, pandorabox has 3,608 craftable items listed in inventory. 45 full pages of 80 each, and 1 page of 8.

I planned to add a "filter" option to the API so that it would omit recipes and item matches that are the result of using a tablesaw to slice up a node (ex: "moreblocks:slope_stone_block_inner_cut"). There is a recipe for making a stone block w/ that as input; but for my own use-case anyway, I don't care about those and would rather they get filtered out in the craftdb, but I can see that someone else might want them in the result set.

Yes, it is always better to fire less events for lua controllers because even if you will not do anything in your own lua code event will still cause significant amount of work launching new sandbox environment. Ok. Maybe this point should be made clear in the documentation then?

If this can be encouraged by not allowing easy identification without actually adding it to luac program it would be only good. This seems rather heavy-handed.

This of course wont prevent connecting all luacs and craftdbs to same line but at least makes it less likely and more clearly shows that it is better choice than having everything connected to single backbone.

I suppose that the problems with "find_all_matching_items" could be addressed by creating an additional node that acts like the touchscreen ordering UI. Instead of the user needing to code up the LUAC to create their own UI and drive it (since 45 formspec widgets can't be done in one luac cycle anyway),

I would have very strong objections for this proposal because it would be attempt to work around symptoms of problem with unnecessary complexity without bringing anything new instead of targeting actual problem.

I've tried to create touchscreen UIs with 40+ widgets. Constructing the table of "addbutton", "addlabel", ... commands requires some LUAC processing cycles. About 45 widgets seems to be the limit. After that, the LUAC times out. IIRC, its allowed 50 microseconds of server tick. So how would someone create a dropdown or textlist w/ 3k+ items in one server tick? Can a LUAC even iterate over 1 table w/ that many items in one "LUAC quantum"? They would need to spread that over many ticks, which adds complexity (iterator state maintenance). The idea for a specialized touch-screen like node (the only player-configurable property would be the channel name, like a digiline filter injector), would be to remove that complexity.

I should not have mentioned this other possible node in this issue though; its a distraction.

With current touchscreen for recipe list one should use dropdown or textlist instead of individual buttons. I do think this is not something that craftdb or any other mod should handle but instead should be fixed in touchscreen mod or advanced touchscreen used instead of old touchscreen. Also advanced touchscreen should be able to do this and more without any problems. Currently at pandorabox recipe is disabled because it can possibly cause some problems and needs testing.

I still need to reconcile these messages w/ the need to look up group items. Ex: "default:pick_stone" needs "group:wood". Should the user ask for "group:wood" via get_all_recipes (its not really a recipe) or find_all_matching_items (better fit imho).

As group is not item and it also is not recipe I think find_all_matching_items is best by how is sounds... Without looking at code I'd imagine that function will take in some kind of more or less free-form query and return list of items that match query parameters. Specifically I would not expect it to take same type as input that it returns. Btw for autocrafter there's also PR open adding support for item groups.

The craftdb API was designed around what I could find on the published/formal digiline docs, LUAC limits and my practical experience using them. However, I can see that it would simplify the API be removing the top-level table and directly returning just the "response". The digistuff RAM and EEPROM work this way (simple responses), and craftdb should not be needlessly unique.

If its guaranteed that messages are always processed in a FIFO, then I will have no problem changing the craftdb API.