Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tags support #107

Merged
merged 16 commits into from
Mar 3, 2024
Merged

Tags support #107

merged 16 commits into from
Mar 3, 2024

Conversation

slavaz
Copy link
Contributor

@slavaz slavaz commented Feb 4, 2024

Tested on Tunellers' Abyss server. Works as well.

Slava Zanko and others added 3 commits February 4, 2024 20:22
- add setting 'enable_tag_support';
- add setting 'pipeworks_item_tag_name_limit' (default 30)
- add functions for get and set tag into item stack.
- add tags cleanup while item dropped in a world or placed in some inventory.

Signed-off-by: Slava Zanko <[email protected]>
@S-S-X
Copy link
Member

S-S-X commented Feb 4, 2024

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

@S-S-X S-S-X added the enhancement New feature or request label Feb 4, 2024
@slavaz
Copy link
Contributor Author

slavaz commented Feb 4, 2024

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:

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

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
Copy link
Contributor

thanks for the summary.

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

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

@slavaz
Copy link
Contributor Author

slavaz commented Feb 4, 2024

changed. Sorry for the mistake.

@SwissalpS
Copy link
Contributor

SwissalpS commented Feb 4, 2024

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
Copy link
Contributor Author

slavaz commented Feb 4, 2024

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
Copy link
Contributor Author

slavaz commented Feb 4, 2024

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

@OgelGames
Copy link
Contributor

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

@BuckarooBanzay BuckarooBanzay mentioned this pull request Feb 5, 2024
6 tasks
Copy link
Member

@BuckarooBanzay BuckarooBanzay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm 👍 (tested it locally and it worked as advertised, even with edge-cases)

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

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

@SwissalpS
Copy link
Contributor

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

@slavaz
Copy link
Contributor Author

slavaz commented Feb 5, 2024

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
Copy link
Contributor Author

slavaz commented Feb 5, 2024

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

slavaz commented Feb 5, 2024

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
Copy link
Contributor Author

slavaz commented Feb 5, 2024

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 OgelGames self-requested a review February 6, 2024 14:08
@S-S-X
Copy link
Member

S-S-X commented Feb 20, 2024

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
Copy link
Contributor Author

slavaz commented Feb 20, 2024

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
Copy link
Member

S-S-X commented Feb 20, 2024

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
Copy link
Contributor Author

slavaz commented Feb 20, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

slavaz commented Feb 22, 2024

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
Copy link
Member

S-S-X commented Feb 23, 2024

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.

for further processing by the handler.

Can be useful for nodes with miltiple inventories, such as 'technic' macines (eg. Alloy Furnance).

Signed-off-by: Slava Zanko <[email protected]>
Tag now visible in event.item.tag
Tag can be assigned via:

return { side="red", tag="blabla"}

Signed-off-by: Slava Zanko <[email protected]>
@slavaz slavaz marked this pull request as ready for review February 27, 2024 19:20
@slavaz
Copy link
Contributor Author

slavaz commented Feb 27, 2024

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
Copy link
Contributor Author

slavaz commented Feb 27, 2024

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

@OgelGames OgelGames mentioned this pull request Feb 28, 2024
@OgelGames
Copy link
Contributor

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
Copy link
Contributor Author

slavaz commented Feb 28, 2024

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
Copy link
Member

S-S-X commented Feb 28, 2024

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
Copy link
Contributor Author

slavaz commented Feb 28, 2024

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

@FeXoR-o-Illuria
Copy link

FeXoR-o-Illuria commented Mar 2, 2024

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
Copy link
Contributor

Injectors tag all items the same

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

@FeXoR-o-Illuria
Copy link

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

@slavaz
Copy link
Contributor Author

slavaz commented Mar 2, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

slavaz commented Mar 2, 2024

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
Copy link
Contributor

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.

@OgelGames OgelGames merged commit 6c66a2f into mt-mods:master Mar 3, 2024
1 check passed
@slavaz slavaz deleted the tags_support branch March 3, 2024 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants