ExprItemAttribute (+ ExprEntityAttribute clean-up)

[Mwexim]

Description

First of all, this pull request cleans up the ExprEntityAttribute class and adds a small additional syntax option. The class was vulnerable for A LOT of NullPointerExceptions if I'm not mistaken, so that should be fixed now.

EDIT: See latest comment for up-to-date information.

It also adds the new ExprItemAttribute that can be used to change the attributes of items.

• You can set, add, remove, delete and reset a specific attribute for the whole item or when equipped to a specific EquipmentSlot.
• Currently, deleting and resetting will have the same behavior.
• When calculating the total attribute value, it will take the different operations into account. However, when changing the attribute you cannot use the modifiers since I didn't want to add another piece of syntax.

Marked as a draft because a lot of testing is needed and I'm currently having problems building the JAR. The code is also sometimes a bit hacky because Spigot's API for item attributes is not that in-depth.

I'm also looking for opinions on the syntax and the general implementation.

• Setting the item attribute will remove all other modifiers of the same attribute (taking the equipment slot into account) and then add its own attribute. Is this a good way to implement the behavior, because there is no 'setting' method present in the API.

---

Target Minecraft Versions: any
Requirements: N/A
Related Issues: #3734

[AyhamAl-Ali]

Nice PR! ⚡

Just a quick note we don't use the keyword closes with related issues so that the related issues don't close when this PR is merged, because we close the issues once a new release is out containing that feature/bug fix.

[ShaneBeee]

This looks good, but I do have a few issues with it.

1. This does not include the option for a uuid. I will have to research this but to my understanding if you have 2 tools/armor with the SAME modifier, both will not apply.
   Ex:
   lets say I have a leather chestplate adding 2 to max speed, and leather leggings adding 2 to max speed, because they're the same modifier (ie: same uuid) they won't stack... this is the reason for the use of the UUID, to make them different.
   I will do a little more research on this and get back to you.
   (Ok I did a bit of research. Bukkit chooses a random UUID if one is not provided - so I guess that is fine
   NOTE: This will NOT work in recipes if used as ingredients.... I had found this out the hard way in a plugin once, probably not a huge issue, but still)

2. You are only using the ADD_NUMBER operation and not giving the option for the other 2 operations

3. There is no mention of the attribute itself?!?! (this was condensed to focus on the point here)

	static {
 		register(ExprItemAttribute.class, Number.class, "attribute [value] [(in|of|for) [the] (1:main[ ]hand|2:off[ ]hand|3:boot[s]|3:shoe[s]|4:leg[ging][s]|5:chestplate[s]|6:helm[et][s]) [slot]]", "itemtypes");
 	}

 	private Expression<Attribute> attributes;
 	public boolean init(Expression<?>[] exprs, int matchedPattern, Kleenean isDelayed, ParseResult parseResult) {
 		attributes = (Expression<Attribute>) exprs[matchedPattern];
 		setExpr((Expression<ItemType>) exprs[matchedPattern ^ 1]);

 		if (parseResult.mark != 0)
 			equipmentSlot = EquipmentSlot.values()[parseResult.mark - 1];
 		return true;
 	}

I see this

attributes = (Expression<Attribute>) exprs[matchedPattern];

Yet it's not in the expression (pattern) anywhere

private static final String ATTRIBUTE_NAME = "skript"; <-- its been a while since I did attribute modifier stuff, but SHOULD the name always be the same? When happens when you have 2 different modifiers with the same name?
Ie: a modifier affecting speed and one affecting max health?

All this being said, It is a good PR, I just feel like it is missing some important things.
I attempted this in SkBee, but quickly gave up due to how much of a freaking mess it became.
So kudos for the attempt.

[Mwexim]

These are all issues that need to be tested thoroughly, that's why the pull request is currently a draft. I'll make sure to look into each remark.

2. You are only using the ADD_NUMBER operation and not giving the option for the other 2 operations

I take all three operations into account when calculating the modifier but decided against adding support to add specific operations to the changers for now to avoid complexity. I personally do not find them useful and if easy to add later on, I'll add them.

3. There is no mention of the attribute itself?!?! (this was condensed to focus on the point here)

	static {

 		register(ExprItemAttribute.class, Number.class, "attribute [value] [(in|of|for) [the] (1:main[ ]hand|2:off[ ]hand|3:boot[s]|3:shoe[s]|4:leg[ging][s]|5:chestplate[s]|6:helm[et][s]) [slot]]", "itemtypes");

 	}



 	private Expression<Attribute> attributes;

 	public boolean init(Expression<?>[] exprs, int matchedPattern, Kleenean isDelayed, ParseResult parseResult) {

 		attributes = (Expression<Attribute>) exprs[matchedPattern];

 		setExpr((Expression<ItemType>) exprs[matchedPattern ^ 1]);



 		if (parseResult.mark != 0)

 			equipmentSlot = EquipmentSlot.values()[parseResult.mark - 1];

 		return true;

 	}

I see this

attributes = (Expression<Attribute>) exprs[matchedPattern];

Yet it's not in the expression (pattern) anywhere

Yes, this was an oversight. I already fixed it but forgot to push.

private static final String ATTRIBUTE_NAME = "skript"; <-- its been a while since I did attribute modifier stuff, but SHOULD the name always be the same? When happens when you have 2 different modifiers with the same name?

Ie: a modifier affecting speed and one affecting max health?

Was thinking the same thing but Skript does not want to build on my computer so I'm having issues testing this PR at the moment. Will adress this if it creates issues.

[sovdeeth]

Do you still intend to work on this PR?

[Mwexim]

Do you still intend to work on this PR?

Not at the moment. Someone can take over or close this pull request, unless there are no plans to create a new one. In that case, you can leave this open. I might complete this when I have time in the next few weeks if no one takes over.

[sovdeeth]

Do you still intend to work on this PR?

Not at the moment. Someone can take over or close this pull request, unless there are no plans to create a new one. In that case, you can leave this open. I might complete this when I have time in the next few weeks if no one takes over.

Thanks for the quick response. I'll leave it open in that case.

[Mwexim]

I reworked the whole system because I didn't think it was satisfying to use. This required one compromise which adds a new AttributeModifier type.
I essentially added two expressions:

1. You can fetch and change all the attribute modifiers of a specific attribute of an item.
2. An expression that allows you to create new attribute modifier. It supports the three different operations and all the slots.

I still have some design questions that I'm not sure of:

• Version 1.21 adds a whole new API for AttributeModifiers which makes use of the NamespacedKey and the new EquipmentSlotGroup which allows more slot groups (like both hands, all armor pieces, etc.)
  Since this API is marked as Experimental, I decided not to add its functionality in this pull request. That said, all the other API methods are marked for removal (which I find very weird, why would you mark something for removal if the only alternative is experimental). However, I think Spigot has a track record of supporting deprecated methods longer than usual, so I guess this is fine. The changes that need to be made are minimal if the methods would end up to be deleted.
• I tried to make the syntax sound as natural as possible. Do you agree or should there be some changes?
• I've never added a custom type to Skript before. Is the string representation correct and good enough?

Even though I didn't add any unit testing, I did test the pull request thoroughly on my private server. Everything seems to work as planned. Not sure though what the procedure is for that.

[cheeezburga]

I much prefer this to the version I've been working on, looks really good! Just some small things here and there that I noticed on a real quick first glance.

[cheeezburga]

I feel like this should also have slot, like the other armour cases.

[cheeezburga]

I think technically the attribute modifiers don't modify the attributes of the item, but the attributes of the holder/wearer.

[Mwexim]

They can only be applied to the items but indeed change the attributes of the wearer. I will make a better description.

[cheeezburga]

Should this point to the minecraft.wiki page instead of the fandom one?

[Mwexim]

I was also hesitating adding an 'unofficial' source to the documentation. I'll search for it on the official wiki as well.

[cheeezburga]

Maybe add amount as an option with value?

Suggested change

	"[a] [new] [(plain|add:additive[[ly] scaling]|mult:multiplicative[[ly] scaling])] attribute [modifier] (named|with name[s]) %string% (and|with) value %number% [and] [(in|of|for) [the] (1:main[ ]hand|2:off[ ]hand|3:boot[s]|3:shoe[s]|4:leg[ging][s]|5:chestplate[s]|6:helm[et][s]) [slot]]"
	"[a] [new] [(plain|add:additive[[ly] scaling]|mult:multiplicative[[ly] scaling])] attribute [modifier] (named|with name[s]) %string% (and|with) (value|amount) %number% [and] [(in|of|for) [the] (1:main[ ]hand|2:off[ ]hand|3:boot[s]|3:shoe[s]|4:leg[ging][s]|5:chestplate[s]|6:helm[et][s]) [slot]]"

[cheeezburga]

Would being able to get the all attribute modifiers on an item be something useful?

[Mwexim]

Not really, since they hide information on the attribute type they were assigned to.

[sovdeeth]

I'd like to see less required information when creating a modifier, and for the options to be spread out over a few expressions instead of all in one.
For example, I think the name could be omitted and replaced by an auto generated one from Skript, so the user doesn't have to worry about that if they don't want to. Likewise, I think the valid slots could be its own effect/expression too:

set {_modifier2} to a new multiplicatively scaling attribute modifier named \"Speed\" with value 0.25 and for the boots slot # Gives a 25% speed boost, works multiplicatively with other modifiers

turns into

set {_modifier2} to a multiplicatively scaling attribute modifier with value 0.25
set name of {_modifier2} to "Speed"
set valid slots for {_modifier2} to boots # or 'limit {_modifier2} to the boots slot` or similar

[Mwexim]

I'd like to see less required information when creating a modifier, and for the options to be spread out over a few expressions instead of all in one.

For example, I think the name could be omitted and replaced by an auto generated one from Skript, so the user doesn't have to worry about that if they don't want to. Likewise, I think the valid slots could be its own effect/expression too:

set {_modifier2} to a new multiplicatively scaling attribute modifier named \"Speed\" with value 0.25 and for the boots slot # Gives a 25% speed boost, works multiplicatively with other modifiers

turns into

set {_modifier2} to a multiplicatively scaling attribute modifier with value 0.25

set name of {_modifier2} to "Speed"

set valid slots for {_modifier2} to boots # or 'limit {_modifier2} to the boots slot` or similar

I get where you're coming from but I'm not sure if this is the right approach. AttributeModifier is effectively an immutable data type, which means that these expressions would create a new instance each time they're changed.

I honestly don't know what Skript's conventions are on these things, but this could lead to issues if someone would change the modifier. Do they expect the items, to which the modifier has been applied, to have the changes of the modifier as well?

Either way, if others agree with adding multiple expressions, I have nothing against it really, but this would require somewhat weird changer shenanigans because the data type of the AttributeModifier expression itself is immutable.

[sovdeeth]

I get where you're coming from but I'm not sure if this is the right approach. AttributeModifier is effectively an immutable data type, which means that these expressions would create a new instance each time they're changed.

I honestly don't know what Skript's conventions are on these things, but this could lead to issues if someone would change the modifier. Do they expect the items, whom the modifier has been applied to, to have the changes of the modifier as well?

Either way, if others agree with adding multiple expressions, I have nothing against it really, but this would require somewhat weird changer shenanigans because the data type of the AttributeModifier expression itself is immutable.

Yeah I'd expect modifiers would be copy-only for the most part, meaning lots of recreation. For the user, though, this isn't really an issue outside of changing a modifier already on an item. If we can't easily work around it, I suppose this is fine, but I think there's a path forward where we could hide that complexity from the user without too much effort.

Note that we could also make an attribute wrapper class that just builds a final modifier when applied to an item, too.