Adds destroyable/placeable key syntaxes

[cheeezburga]

Description

This pull request implements a condition, expression, and effect, to allow for the destroyable/placeable keys of an item to be modified.

Here are some examples of the syntaxes:

# adding
add (stone, dirt, diamond ore) to destroyable restrictions of player's tool
allow {_item} to destroy (emerald block, oak wood planks) in adventure mode

# removing
remove sand from destroyable blocks of {_item} in adventure mode
prevent {_item} from being placed on (diamond ore, diamond block) in adventure
clear break restrictions of {_item}

# checking
send true if player's tool has any build restrictions else false

command /has specific destroyable keys:
    trigger:
        send true if {_item} is able to break (stone and dirt) in adventure mode else false

---

Target Minecraft Versions: any
Requirements: none
Related Issues: none

[cheeezburga]

Was wondering while making these if they could each be merged into just one condition, expression, and effect? Like, all the classes which are for either destroyable or placeable keys are pretty much identical, obviously with just changes to the methods used.

[sovdeeth]

I haven't reviewed the whole thing, because there are some critical issue that need to be addressed first.

Main things:

• Each pair of cond/eff/exprs should be combined into a single one, as there's a ton of duplicate code right now.
• NamespacedKeys aren't a Skript type. This PR either needs to add them (personally think this isn't a great idea), use just itemtypes, or use strings and items.
• There's many times where hasItemMeta isn't checked, which could cause exceptions if players use air in the syntax.

There are more minor nitpicks too, but it's not worth pointing them out right now because the above issues will probably change a lot of the code. Mostly, they're centered around the Skript coding conventions and there are some intricacies with ItemType we can discuss later.

Overall, it needs some work, but it's got good bones. nice work and thanks for making the PR!

[Fusezion]

Quick overview haven't kept up with new reviews some might be copies

[cheeezburga]

I think, for the most part, a lot of those issues that were there before should be better, if not fixed, now. When I finish work I'll try combining the expressions and effects to just have one syntax each.

[sovdeeth]

tests failing

[cheeezburga]

tests failing

I think the reason tests are failing is because Paper just fully removed all the destroyable/placeable keys methods. And apparently got rid of the Namespaced class. What should I change so that the syntax still works on versions lower than 1.20.5 and the tests pass?

[Fusezion]

only java 21 would be failing if that was the case, the issue appears to be a class issue

[sovdeeth]

NamespacedKey still exists, and has since 1.13, I believe.
the methods being removed will be slightly more annoying.

[cheeezburga]

This is my first time doing any reflection kind of stuff, so I'm not entirely sure if the way that I've done this is correct and/or good. Did some fairly limited testing with this new version and it seems to all work as intended.

[sovdeeth]

As someone who has done very little reflection, this seems alright. However, all the Since versions need to say Paper <1.20.6

[Moderocky]

So the thing that concerns me with this is that I don't think ItemType is a good analogy for the restrictions.

As well as saying a player's tool can break a block (e.g. stone) you can also say it can break a category of blocks, or a tag (e.g. endermen_can_pickup, all_signs, acacia_logs, bamboo_plantable_on)
and in fact these are typically more useful for this feature, since you're usually looking for something like "this axe can break only wooden things" or "this flower can only be placed on proper flower spots"
so not only will skript not be able to properly interact with the restrictions from other sources (they'll just return null or throw an error) it'll also not be able to apply them in a nice way.

Now I'm not sure about the best way of fixing this, personally I wonder if it might merit some wider changes to aliases to integrate tags in some way, but you could also do things with resourcekeys directly.
If you do take the ResourceKey route (I think bukkit has a semi-analogous name space key class or something) then you'll need to be very careful with converters because it's one of those things that could be anything.

Here is a nice list of all the (current) tags: https://mcreator.net/wiki/minecraft-block-tags-list

[sovdeeth]

So the thing that concerns me with this is that I don't think ItemType is a good analogy for the restrictions.

...

Here is a nice list of all the (current) tags: https://mcreator.net/wiki/minecraft-block-tags-list

oh interesting, I didn't realize it allowed those tags as well. Honestly, we may want to but this on the back burner for now, given that Paper has completely removed this api for 1.20.5+ in favor of some nebulous future api. We may want to wait and see how that one works before committing to an implementation.

[sovdeeth]

Any updates here?

[cheeezburga]

Any updates here?

See here for a small discussion on it, but basically it just seems like its still very subject to change, so might give it some more time. Do you reckon I should just finalize this with how its currently implemented, and then do another PR to fix it at a later date when Paper releases a final version? Or just leave it for now?

[sovdeeth]

Given the support just isn't there in the API, I think waiting is the better choice.