Villager Leads

[HalbFettKaese]

Project Page: https://www.planetminecraft.com/data-pack/villager-leads/

[EstEarth202]

• Certified Compatibility ❌
• Stamp of Quality ❌

Common Trait Convention ☑️

Custom Model ID ☑️

Global ignoring tag ❌

data\tddh_villager_leads\functions\player\find_villager\loop.mcfunction -> as @e[type=villager,tag=!global.ignore,dx=0] global.ignore.pos missing.

Global Durability ☑️

Namespace Convention ✔️

Shulker Box Loot Table ✔️

Datapack Advancement ✔️

Datapack Uninstallation ❌

Reloading Message ✔️

Note

✔️ = Followed ❌ = Doesn't follow ☑️ = Not required/Skip

[HalbFettKaese]

1. Why do I need global.ignore.pos? Is it because my method of killing them is teleporting them under the map? Because in this case, I wouldn't consider the position change relevant, as the entity is killed anyways and the entity that it is replaced with retains the former position of the old entity, and I'm already testing for the global.ignore.kill tag. Of course, I can change the unless entity @s[tag=global.ignore.kill] to an if entity @s[tag=!global.ignore.kill, tag=!global.ignore.pos], but I still wouldn't agree that this change improves compatibility as to the outside, the entity's position does not appear to change.
2. I don't get why a data pack uninstallation function would be necessary, as my data pack doesn't add any content to be removed from the world and therefore doesn't leave anything broken behind when uninstalled normally. What it adds is the action of connecting the lead, anything else is still vanilla. Everything pack-exclusive left behind would be the tddh.leashed tag on the villagers, which should not be able to affect how anything behaves after the pack is uninstalled from the world, and even if the pack was re-installed after that, it would be able to handle these tags having been untreated for a while without any noticeable impact. I could implement a function to unleash all villagers that are leashed or even exclusively filter for those who were leashed through my pack, but I'd rather not include that feature than have something inaccurate that leaves out unloaded villagers, or something laggy that keeps track of all leashed villagers' positions just so that they can be found when uninstalling the pack when nothing is broken in any way when the pack is directly uninstalled without an uninstall function.

[EstEarth202]

First, why does it have to be uninstalled?

What I see is you have added and used scoreboard and storage, which if the user disables pack It will remain stuck in the world map, which 2 things are easy to delete.

On the other hand, I understand that a tag, setblock doesn't need to be deleted. It depends on each person how much they can do and the forceload is still overlooked because there may be other packs using it.

Advice: You just need to add files. uninstall.mcfunction In which there will be

scoreboard objectives remove tddh.lead.temp

data remove storage tddh.villager_leads: temp Items
data remove storage tddh.villager_leads: temp Villager
data remove storage tddh.villager_leads: temp PlayerUUID

schedule clear tddh_villager_leads: general / tick * See at Technical Advice

If you use the `tick.json` file when you remove everything, it will still work, so datapack must be disabled after uninstallation. But it results in automatic reloading. This may result in other packs being restarted from scratch. 
The above method suggested to avoid `datapack disabled`, but just `schedule clear`.

And tell to the user on the planetminecraft web page How to uninstall.

Second, why do you need global.ignore.pos?

Because the file resummon.mcfunction has tp, if that villager doesn't have global.ignore.kill but has global.ignore.pos it will tp anyway.

After I carefully checked the functionality of your pack again, I found the pack incorrectly working slightly. Which the pack will work properly, you just need to move the command line down.

Before:

data merge entity @s {Offers: {Recipes: []}}

data modify storage tddh.villager_leads: temp Villager set from entity @s {}
data modify storage tddh.villager_leads: temp Villager.Leash.UUID set from storage tddh.villager_leads: temp PlayerUUID

After:

data modify storage tddh.villager_leads:temp Villager set from entity @s {}
data modify storage tddh.villager_leads:temp Villager.Leash.UUID set from storage tddh.villager_leads:temp PlayerUUID

data merge entity @s {Offers:{Recipes:[]}}

Note: As I checked, if the data merge is above it will cause the {Offers: {Recipes: [...]}} data to be deleted from the villager before it is stored in storage.

And I found that there are some commands that are not required . And after I reconsidered it, I tried changing the command. Which works are the same Which points to change are as follows

data \ tddh_villager_leads \ functions \ player \ find_villager \ loop.mcfunction

   ↳ as @e [type = villager, tag =! global.ignore, tag =! global.ignore.kill, tag =! global.ignore.pos, dx = 0] 

   Where global.ignore must be entered Here because It should be filtered out at first before proceeding to the next step.

next -> give_lead\start.mcfunction -> do.mcfunction

From the previous step Thus eliminating the need for these 2 lines of commands

execute store success score $resummon tddh.lead.temp unless entity @s[tag=global.ignore.kill]
execute if score $resummon tddh.lead.temp matches 1 unless data entity @s Offers.Recipes[0] run scoreboard players set $resummon tddh.lead.temp 0

Which will allow these two lines to use a command like this

execute if data entity @s Offers.Recipes[0] run function tddh_villager_leads:on_villager/give_lead/resummon
execute unless data entity @s Offers.Recipes[0] run function tddh_villager_leads:on_villager/give_lead/no_resummon

And I also found that villager who traded with the player then GUI trade will disappear delay than villager who just chose the profession. Which can be solved by kill @s after tp @s ~ -300 ~ There will be make sense to do as @e [type = villager, tag =! global.ignore, tag =! global.ignore.kill, tag =! global.ignore.pos, dx = 0)

Technical Advice (Optimization)

After another check I see that you use the scoreboard operation to slow down commands to 1s. Which has a shorter and easier way

Which can delete tick.json file

scoreboard objectives add tddh.lead.global dummy
scoreboard players set $ 20 tddh.lead.global 20

and

scoreboard players add $ timer tddh.lead.global 1
scoreboard players operation $ timer tddh.lead.global% = $ 20 tddh.lead.global

execute if score $ timer tddh.lead.global matches 0 as @e [type = villager, tag = tddh.leashed] unless data entity @s Leash run tag @s remove tddh.leashed

load.json file will become

{
    "values": [
        "tddh_villager_leads: general / load",
        "tddh_villager_leads: general / tick"
    ]
}

And tick.mcfunction file will become

execute as @e [type = villager, tag = tddh.leashed] unless data entity @s Leash run tag @s remove tddh.leashed

schedule function tddh_villager_leads: general / tick 1s

The tick.mcfunction file will be called only once by the load.json file and it will loop with the 1 second schedule function command all the time.

[HalbFettKaese]

Thanks for the advice with adding a /kill @s. I thought I tested that before and it made the GUI not close at all, but it seems the test wasn't isolated enough. Also thank you for mentioning the bug with the data being removed even before it's saved, though that was already fixed last week.

As for the commands that are "not required": The no_resummon function can be seen as a fallback implementation that does not include killing the villager or changing its position, so that should still run, even if the global.ignore.pos or global.ignore.kill tags are found. Apart from that, storing the result of an NBT test in a score first and then testing for its result is still more efficient than applying the NBT test twice. As for the uninstall function, it would not have occured to me that also removing the unseen parts would be relevant too, but it makes sense. On that note, it would be good to add more detail in the official specifications to help clear up misunderstandings like this.

About the discussion about whether to test for the global.ignore.pos tag or not: I have always been fully aware that the villager was teleported under some circumstances, but the point I'm making is that it should be teleported in those cases, because to any other pack that this should be compatible with, it will only look like the villager died, and it will be impossible for any pack to detect that before that, the villager's position also changed. Either way, I added the check despite personally considering it to go against the intention of the tag, and in case you agree with my reasoning, I'm still able to change it back.

[EstEarth202]

It's okay & Thanks for the reply on the subject. The ambiguity and detail that could be missing in the Official Conventions.

After I know your reasons, you can do this.

data \ tddh_villager_leads \ functions \ player \ find_villager \ loop.mcfunction

as @e [type = villager, tag =! global.ignore, dx = 0] here will not change anything.

Since the condition here is or, it cannot be use this @s [tag = global.ignore.pos, tag = global.ignore.kill] because it is a condition and.

execute store success score $ resummon tddh.lead.temp unless entity @s [predicate = tddh_villager_leads: global_ignore]

{
    "condition": "alternative",
    "terms": [
        {
            "condition": "entity_properties",
            "entity": "this",
            "predicate": {
                "nbt": "{Tags: ['global.ignore.kill']}"
            )
        ),
        {
            "condition": "entity_properties",
            "entity": "this",
            "predicate": {
                "nbt": "{Tags: ['global.ignore.pos']}"
            )
        )
    )
)

If doing this, villager with either only global pos or kill. Or have both Will run at the no_resummon file as you have told

[HalbFettKaese]

Thanks for the suggestion, but I already had if entity @s[tag=!global.ignore.kill, tag=!global.ignore.pos] in the version I uploaded yesterday, which is equivalent to your suggestion in accordance to DeMorgan's law, except that it doesn't have the large overhead of calling a predicate and running up to two NBT tests.

[EstEarth202]

Certified Compatibility ✅

Stamp of Quality 💕

And Good pack, LOL Datapack reviewer @EstEarth202. :thumbsup:

ps. What?? advancement_convention.json Why don't you name this file villager_leads.json 😂