mt-mods / pipeworks

Pipeworks is a mod for Minetest allowing the crafting and usage of pipes and tubes
Other
14 stars 19 forks source link

Tags support #107

Closed slavaz closed 7 months ago

slavaz commented 8 months ago

Tested on Tunellers' Abyss server. Works as well.

S-S-X commented 8 months ago

Could you add explanation to description about what it is, why it is needed / what problem it solves and how it works?

slavaz commented 8 months ago

The readme-tag from fork:

Welcome to Tags Pipeworks world!

I hope, this my addon will help to to construct more amazing setups...

Main idea is: to assign some tag to a stack of items into pipelines and to make decision about routing based on the tag. In this case, Injectors should support tag assignment (as they are kind of'entrances' into pipelines) and filters should support tags (at least, LuaTube). In additional, also was added 'Tags Sorting Pneumatic Tube Segment' which works like a usual 'Sorting Pneumatic Tube Segment', but operates with items tags instead of items. So in 'Tags Pipelines' world doesn't matter what is the item, much important, which tag the item has.

Common rules for tags in Injectors and LuaTubes

Commas in tags aren't allowed and will be replaced by undescore (_). Leading and trailing spaces in tags will be trimmed.

Eg. If you specify a tag like ' My ,,,,, Tag ' then it will be processed as 'My _____ Tag'

Maximum lenght of a tag is 30 (may be redefined via pipeworks_item_tag_name_limit)

Rules for 'Tags Sorting Pneumatic Tube Segment'

If an item in the tube doesn't have a tag, but you want to sort it, then you should specify special tag '<<notag>>' in one (or in few) of direction fields. If you don't specify the tag, the tube will works like usual sorting tube with empty directions.

You may specify few tags for a direction into 'Tags Sorting Pneumatic Tube Segment', comma separated. Eg:

'ToFurnance, <<notag>> , ToStorage, My ,,, Tag'

it will be processed like the list:

empty tag definitions are skipped.

Maximum lenght of one string with comma-separated tags is 30*6 = 180 (may be redefined via pipeworks_item_tag_name_limit, will be multiplied by 6)

Digiline Filter Injector

Your LuaController now may send a command to an Injector with tag definition which should be assigned to injected stack.

Example:

digiline_send("MyInjector",
    {
       item="default:dirt",
       count = 99,
       tag = "SomeTag"
    }
)

In this case, even if your Digiline Injector already has a tag in a settings, the tag will be redefined by a tag from event message. If you didn't specify a tag into event message, then predefined tag will be used. If you didn't set predefined tag, then no any tags will be assigned to items.

LuaTube

LuaTubes may have an access to item tags via functions:

myVar = get_item_tag()

set_item_tag("MyTag")
SwissalpS commented 8 months ago

thanks for the summary.

there's a recurring typo: "lenght" --> "length"

(maybe run the entire text through a spell checker to be safe)

slavaz commented 8 months ago

changed. Sorry for the mistake.

SwissalpS commented 8 months ago

If you didn't specify a tag into event message, then predefined tag will be used. If you didn't set predefined tag, then no any tags will be assigned to items.

"... no any tags will be ..." --> "... no tags are assigned ..."

Possible other wording: "If the event message doesn't specify a tag, then the predefined tag from the injector's formspec is used. If no tag is set in the formspec nor does the event message provide one, then no tag is assigned to the items."

Edit: if this is merged, I can help with some other language cleanups ;)

slavaz commented 8 months ago

Em... the readme-tag.md wasn't published in the merge request. It's from another fork: https://gitlab.com/tunnelers-abyss/pipeworks/-/blob/master/README-tags.md?ref_type=heads

So thanks for your proposal, I'll change a text, but the file is out of scope of the merge request...

slavaz commented 8 months ago

And sorry for my English, it isn't my native language.

OgelGames commented 8 months ago

This is a cool feature, I see it being very useful for an automated crafting machine, being able to tag items with their destination.

SwissalpS commented 8 months ago

Is there a way to clear tags? (other than putting items in drawers etc)

slavaz commented 8 months ago

i took the liberty and added a commit to fix some indent issue.

Yes, of course, thank you. My 'VS Code' IDE formatted it in the way...

if you have the text for the docs already could you add it in another PR to the (not-yet-existing) docs: #104 thanks :)

Aghr... As you may see, I'm better at writing code than writing documentation (as I'm not a native English speaker). So probably I need a help from some native-speakers at the way...

P.S. some playwords here: If you want to punish a programmer, make him write documentation ;)

slavaz commented 8 months ago

Is there a way to clear tags? (other than putting items in drawers etc)

tags may be cleared in 3 ways:

1) when items moved outside the pipes into inventories 2) when items dropped in a world (broken pipe or some other issue) 3) in LuaTube via set_item_tag(nil) call

SwissalpS commented 8 months ago

I'm better at writing code than writing documentation

that's ok. It is also easier for you to write the first draft and easier for others to then edit it than to come up with one from reading the code :D

SwissalpS commented 8 months ago

tags may be cleared in 3 ways:

  1. when items moved outside the pipes into inventories
  2. when items dropped in a world (broken pipe or some other issue)
  3. in LuaTube via set_item_tag(nil) call

3 is cool.

1 & 2 make me think that the tag isn't stored in item's meta at all, which means using buffer chests would clear the tags. (That's fine by me btw, just wanted to be sure)

slavaz commented 8 months ago

btw, there is ingame tutotial, but it's placed on "Tunellers' Abyss" server (37.46.208.34:30000). Don't know how to explain a path to the tutorial, but I'll try :) 1) connect to the server 2) go to Spawn (enter '/spawn' in a chat) 3) Go to forward and a little bit left until you see 'Travelnet Hub' (in front of spawnpoint is placed a shop, the travelnet hub entrance at left side of he shop) 4) in the 'travelnet hub' go to 3'rd floor and enter to travelnet # 279 5) Go to 'Pipelines tags Museum'

In the museum you may see usage cases, how to use tags...

slavaz commented 8 months ago

tags may be cleared in 3 ways:

  1. when items moved outside the pipes into inventories
  2. when items dropped in a world (broken pipe or some other issue)
  3. in LuaTube via set_item_tag(nil) call

3 is cool.

1 & 2 make me think that the tag isn't stored in item's meta at all, which means using buffer chests would clear the tags. (That's fine by me btw, just wanted to be sure)

Tags are stored in item meta only inside tubes. Outside it will be cleaned up in all cases (like, no any tags were assigned).

OgelGames commented 7 months ago

I noticed that items can only have one tag, I think it would be better if items could have multiple tags, just like the tag tubes can sort multiple tags.

slavaz commented 7 months ago

I noticed that items can only have one tag, I think it would be better if items could have multiple tags, just like the tag tubes can sort multiple tags.

Well.. I thought about it, but I was afraid that it would be rejected with the reason: "too complicated" :) Also, I had few ideas to route such multiple tags and I don't know which one I should implement...

But if you are proposed it, I'm not afraid now :) And I have questions:

1) 'borderline processing': looks like we should introduce new one config parameter, like 'maximum amount of tags per item', like in Tags Tube. BTW, an amount of items in the Tag Tube now hardcoded to 6 (like, it's the same amount of items in sorting tube). probably, need to move it into config params too, what do you think?

2) Routing rules in Tag Tube: I see few usecases: 2.1) An item has tags 'Tag1,Tag2', Tag Tube has: 'green'='Tag1'. Simple case, Item should be routed to 'green' direction 2.2) An item has tags 'Tag1,Tag2', Tag Tube has: 'green'='Tag1', 'red'='Tag2'. Item should be routed randomly between 'red' and 'green' directions, right? 2.3) An item has tags 'Tag1,Tag2,Tag3', Tag Tube has: 'green'='Tag1', 'red'='Tag2,Tag3'. Well.... there are three route strategies:

a) route to 'red' direction as it has two tags from item tagset (like 2.1 case). b) Simple random between 'red' and 'green' (like 2.2 case) c) a random based on 'weight' of a direction. Eg, 'green' direction has one matched tag, 'red' - two matched tags. So 'green' = '0.33', 'red'='0.66' probability of routing...

P.S. There is a variant 'd': to implement all three strategies and to select a strategy from some config param...

What do you think?

S-S-X commented 7 months ago

Warning: I didn't really put much time into this so there's possibility this option is not viable / is completely broken, sharing anyway for some brainstorming.

Possible option: this probably could be first implemented as a single tag solution and afterwards updated to handle multiple tags after we've heard comments about real use cases players might come up with. This could also provide useful feedback about possible/wanted algorithms for multi tag routing.

Doing that should not create any direct conflicts for update because it is stack that could have multiple tags but does not strictly need multiple tags for complete multi tag like functionality because routing can handle that but instead multi tags make it easier for players to implement routing (if multi tag decisions done right in mod code supporting most important use cases players would have).

API functions are now getter and setter, if going for staged implementation/release then getter will cause some issues: with only single tag it probably should return string but with multiple tags it probably should return table.

For second stage updates

Setter is simple: it will clear tags and set new tag (singular) for stack. For multi tag support it would probably anyway useful to have setter that clears other tags. So for multi tag update it would make sense to introduce:

So, for getter which gets more complicated for staged release, one of these could possibly be implemented:

As you probably noted I've also written these as OOP functions to bring easy to use API for players while avoiding any lookup performance penalties or event table overhead introduced server side. Data of course could still be directly accessible if needed. But of course this does not have to be OOP model, can as well be just directly accessible data or other API functions but OOP model allows coupling functions next to data and can also be done in a way that defers underlying data access until it is actually needed (no significant performance impact for constructing events for Lua tubes that do not use tags).

slavaz commented 7 months ago

Well, if you want to hear what "users" have to say about the usage, this feature was implemented on the Tunellers Abyss server over a month ago. As a user, I don't feel like I need multi-tag support. However, I feel like I need "tag levels" support. Let's imagine that you have a tag named "Factory-1.Line-3.Machine-5.Slot-2". As a user, I want to have a chain of tag pipes, the first pipe in the chain should be able to route all tags starting with "Factory-1" in a specific direction. The second tube should work with part of the tag "Factory-1.Line-3" and so on.

And implementation of 'tag levels' feature in Tags Tubes will be very simple, something like this:

- if item_tag == tag  then 
+ if item_tag == tag or start_with(item_tag, tag..'.') then
...

For LuaTube, we may add some new function like:

bool is_tag_in_group(tag, group)

or

bool tag_start_with(tag, prefix)

or
....

or please propose your function name, my English knowledge is over here...

Regards to implementation tag functions into event object: It's definetelly good idea because currently an environment for LuaTube is created before ANY event: 'program', 'interrupt', 'item'... But with the implementation tag functions inside event we can assign it for 'item' events only. This will definitively reduce a payload...

So I think I will reimplement it, just need to reach agreements on what the API will look like.

Will it be:

event {
  type = 'item'
  get_tag() = function()
  set_tag(tag)   = function()
  tag_start_with = function() -- or is_in_group() or your funcname
}

or:

event {
  type = 'item'
  tag {
    get() = function()
    set(tag)   = function()
    start_with = function() -- or is_in_group() or your funcname
 }
}

?

slavaz commented 7 months ago

Well, guys, I'm awaiting for your decision which way we should select: 'multi tags' or 'tags levels'. tags functions inside event or not...

Implementation will not spend a lot of time, but I want to get a confirmation from your side to avoid unnecessary changes and to save your and mine time...

OgelGames commented 7 months ago

Both, multiple tags will provide the same functionality. BTW, when I say multiple tags I don't mean separate values, but comma-separated tags in a single string.

As for the Lua tube, I realized there's a much simpler way to set the tag: use a second return value. The Lua tube code could be something like this:

if event.type == "item" then
    -- Tag read from event
    if event.tag == "trash" then
        return "blue"
    elseif item.name == "default:wood" then
        -- Tag set by second return value
        return "red", "storage,wood"
    end
end

I can make a PR with the changes if that's easier for you.

slavaz commented 7 months ago

You seem to be pushing for a "multiple tags" (comma separated, multiple tags) solution instead of "tag levels" (dot separated, one tag) ...

Ok then.

well, despite the fact that several tags will be in one line, later they will have to be parsed into a table. And I like your solution with simple event.tag field, but returning new value... hm... It looks like for getting a tag, you should look at one place, but for setting a tag you should do it in another place...

if develop your idea, it would be nice to set a tag via:

event.tag = "blabla"

one place for getting and setting tags...

What do you think?

OgelGames commented 7 months ago

I think the event table should remain an input only. Also, the input/output direction works the same way (reading the input from event.pin and returning the output), so it's something that users are accustomed to already.

slavaz commented 7 months ago

Well if it's ok that the second return parameter will be reserved for the tag then yes it's reasonable...

But you know, I have a few ideas and 'Tag support' - it's just one of ideas :)

A second one my idea : have ability to split in LuaTube item stacks by directions... eg:

return true, { -- first param is bolean instead of string... or even a table, no second param
  "green" = {"count" = 49},
  "white" = {"count" = 30}
  "black" = {"count" = <any number even zero as last table item is for all other remaining number of items>}
}

I don't know if I will implement this or not (maybe I'm too lazy or maybe it will be rejected by the mod developers/maintainers).

But looks your idea will not have conflicts with my future idea... even with tags.

Ok, agree.

Regarding to 'comma separated tags' (miltitags): I suggest not implementing it in the merge request and following MVP (Minimal Value Product) concept. Your proposal with "event.tag" and "return 'blue', 'tag name'" is fully compatible with future solutions: as well as "multi-tags, comma-separated" and "single-tag, levels, dot-separated".

S-S-X commented 7 months ago

A second one my idea : have ability to split in LuaTube item stacks by directions... eg:

For priority and to keep it simple you'd need indexed tables:

return { { "black", 10 }, { "white", 20 }, { "red" } }

instead of

return { "black" = 10, "white" = 20, "red" = some_rest_of_it_marker }

It is true that hash tables do have order for lua implementation but likely should not rely on that or encourage relying on something that is undefined in lua spec. Entries on indexed tables on the other hand do have clearly defined order and there's spec for it.

Both can support any kind of output configuration. Both can serve split output while using first return value only. Both do make sense for user. Only first makes sense for priority when it comes to Lua table key order specification (hash tables do not have order).

slavaz commented 7 months ago

Um...my idea for stack splitting with LuaTube is completely outside the scope of tag support (offtopic, yes). I just shared my vision, sorry if it confused you...

S-S-X commented 7 months ago

Um...my idea for stack splitting with LuaTube is completely outside the scope of tag support (offtopic, yes). I just shared my vision, sorry if it confused you...

My point is that you do not need second return value for your vision, single return value would be enough and would also make sense as first return value is already used for routing while tags are not directly used for routing by Lua tube itself even if tags could be used for routing if user decides so.

edit for clarification: I should have pointed this out in previous comment but didn't think of it and wrote misleading reply by focusing on table itself. I hope this comment however makes it clear from routing standpoint. For extra clarification I believe that routing data should also include possible splitting if it would be added.

slavaz commented 7 months ago

Ok, if we leave this stack split out of the discussion, how we may change a tag from LuaTube?

OgelGames proposed todo it via returning two values:

return "red", "storage,wood"

and you are proposed via returning one value, am I right? Like:

return {"red", "storage,wood"}

?

S-S-X commented 7 months ago

and you are proposed via returning one value, am I right?

Actually I didn't propose anything about tag return values. However if stack splitting would be added in future then it would likely be better to use single return value so that it could support setting tags for individual output stacks without having to manage indexes for multiple tables in user program.

Defaults for second index could be tag and made conditional based on scalar type later but if even more features would be added in future then doing that would very likely make it overly complicated through tech debt and through having to introduce multiple ways to select and handle possible new features.

If we'd like to avoid that possible complication then it would likely be better to not go further than basic routing with scalars and use configuration tables either for everything or at least everything besides basic routing.

Basically this would allow adding features without complicating handling and requirements if more features would be added in future:

return { "red", { tag = "storage,wood" } }

instead of this:

return "red", "storage,wood"

Consider that if we'd add splitting in future and want to add different tags for each output stack:

return { {"red", {count = 10, tag = "storage"}}, {"black", {count = 20, tag = "wood"}} }

Instead of this:

return { {"red", {count = 10}}, {"black", {count = 20}} }, { "storage", "wood" }

Main difference for user program is that first one has all the data in single object so you do not have to additionally manage indexes for second return value. Basically while first form is longer it is also more descriptive and simpler to construct than second form with multiple return values.

I mean decisions here can significantly affect what has to be done in future if more features would be added. And of course considering if we'd want to decide now that tags are actually worth for introducing second return value, I mean what if someone comes up with a thing that would need third return value?

Explanation why I am proposing indexed entries also for root level of returned table (and root level of indexed entries in case of possible splitting):

It would allow reducing output configuration with reducing structure of data, say you'd want to include shorthand for equal splitting (as equal as it can get with integer division):

return { "red", "black" }

Instead of

return { {direction = "red"}, {direction = "black"} }

So pulling stack splitting again into this discussion is because if changing return values we are also making decisions that will have significant impact on future development on same area. It seems like a simple and straightforward thing but it really isn't.

Good to consider that all this might actually make either API functions better way for managing tags but personally I'd be fine with either of solutions, just saying that adding return values might not be as good idea as it first seemed to be.

slavaz commented 7 months ago

Good point! Brilliant!

It allows us to recognize a kind of 'a structure version of returning values'. Eg,

if type(retval) == "string" then
   handle_it_like_before_tags()
elseif type(retval) == "table" and type(retval[1]) == "string" then
  handle_it_with_tags()
end

and in future just one more check will be added like:

...
elseif type(retval) == "table" and type(retval[1]) == "table"
   handle_it_with_output_split_feature()
end

In this case, each handler may re-use previous by calling with parts of returning table as a parameter ... good!

So for now, better to implement it in the way:

return { "red", { tag = "storage,wood" } }

Any objections from other reviewers?

OgelGames commented 7 months ago

I would prefer to keep the simple string return values, but it does make sense to introduce the table return values now rather than later.

So, I suggest four types of return values, each of which can be easily distinguished with type checks:

-- Current way of returning output side
return "red"

-- Table for a single output
return {side = "red", tag = "storage"}

-- Nested tables for multiple outputs
return { {side = "red", count = 1}, {side = "blue", tag = "trash"} }

-- Shorthand for equal splitting
return {"red", "blue"}

For now it would just be the first two, the others can be added later when stack splitting is added.

slavaz commented 7 months ago

I may create two merge requests with different solutions in returning format... and whoever of you, guys, presses the 'Merge' button first wins the battle... :)

S-S-X commented 7 months ago

I may create two merge requests with different solutions in returning format... and whoever of you, guys, presses the 'Merge' button first wins the battle... :)

Well, now that we all agree on second return value being evil these different suggestions aren't exactly incompatible. Mostly it is just about how verbose it will be but so far I think all of these can be extended equally and all of these can also support both explicit and implicit keys for route selection.

slavaz commented 7 months ago

I think, it's ready to review again. I've implemented solution proposed by @OgelGames

In additional, I passed one more parameter into minetest.registered_nodes[node.name].tube.insert_object() to bring a chance for processing tags by endpoints. eg, can be helpful for technic machines with multiple input slots (Alloy Furnance)

Please review.

slavaz commented 7 months ago

Sorry, approve again, please. I think, I implemented all suggestions from the discussion

OgelGames commented 7 months ago

So, I was testing and reviewing this today, and I found a few bugs and issues. I was going to add comments and suggestions here, but I realized it was easier to just make a separate PR based on this one. See #111

slavaz commented 7 months ago

Thank you! It's good idea as I'm a third-party contributor and I don't know how to contribute in your project in right way... I just shared working solution :) But you know better how it should looks because you are supporting existing codebase... So thank you again, that you didn't force me to do all of this :)

So can I close this merge request as you opened new one?

S-S-X commented 7 months ago

For visibility and individual review states it would be better to move changes from #111 here, close #111 and continue working or reviews here.

slavaz commented 7 months ago

done. Thanks for the tip. All @OgelGames's changes are now in the branch.

FeXoR-o-Illuria commented 7 months ago

In it's current state I have a hard time finding this useful.

Injectors tag all items the same meaning the items are already sorted by tag in the output of the injector. That means that for any further sorting that may be required the tags are not useful.

For the plans about splitting stacks in LUA tubes the same can be achieved by splitting without tagging. (And instead of splitting round robin can on average also achieve the same with e.g. sorting tubes allowing 3 directions and 2 having the same TP tube target routing 2/3rd to one place and 1/3rd to another with less server load and code to maintain)

The uses I can see for tagging are tagging per injector slot. That would require a more sophisticated formspec though. E.g. inject 3 default:leaves tagged craft_mulch and 99 default:leave untagged to clean the container. (Of cause 2 injectors with only one leading to the autocrafter could also achieve that and doesn't need a sorting tube so not really more nodes required just more signaling instead of node checking in tubes. Not sure what's e.g. heavier on the server) LUA tubes can already utilize tags in the current version of this branch - but again only really useful when more destinations are available than the output directions of the LUA tube. (Digiline filter injectors ~could~ are making use of that with digiline signals per injection tagging differently - ~but that's another mod~ EDIT: Thank you @SwissalpS :D)

Tubes already have priority (not communicated to the player in-game), stack size and item type sorting. Still, sending half of the stacks of say leaves one way and everything else another (including the other half of the leave stacks) is not possible in one sorting tube. IMO rethinking the already present concept of sorting in tubes would be more valuable for pipeworks so dear to me than adding another questionable way to sort.

But this is just my point of view ofc. and I'm sincerely grateful for your enthusiasm ;)

SwissalpS commented 7 months ago

Injectors tag all items the same

unless you use the digiline injector, which is part of this mod.

FeXoR-o-Illuria commented 7 months ago

Ok, so, yea, digiline filter injectors with tagging seem useful to me :)

slavaz commented 7 months ago

Also please don't forget that not all players know how to write programs, so they build/use big (and sometimes crazy) setups with pipewors and meseecons (without digilines). For this sort of people itemwise/stackwise injectors are much important rather than 'LuaTube' or 'Digilines Injector'... these items are usefulness for them and I pretty sure, they ask questions to themself like : 'what is it? why it was implemented at all'?

I just want to say, that opinion related to tags usage into Itemwise and Stackwise it's good opinion and good usecase(btw, thanks for the opinion) but... it's just one opinion. Pipeworks mod has much wider audience of players with different opinions and with different use cases in each case...

SwissalpS commented 7 months ago

Some players will use tags as a messaging system. (Datatransfer) So I'm wondering if the tag length/amount of tags is limited in some way?

slavaz commented 7 months ago

one tag - no more than 32 symbols (can be changed via settings) Tag sorting tube - max 16 tags per direction, one tag max 32 symbols. https://github.com/slavaz/pipeworks/blob/tags_support/item_transport.lua#L7C1-L8C83

And if users will use tags as a signals transferring system... well, In my opinion, it's just makes gameplay more rich and interesting :)

SwissalpS commented 7 months ago

And if users will use tags as a signals transferring system... well, In my opinion, it's just makes gameplay more rich and interesting :)

absolutely, we just need to make sure they don't overdo it ;)

Thanks for the link and response. Sounds good to me.