Fix super entity changer with location

[TheLimeGlass]

Description

• Adds INTERNAL change mode for the cases where the change mode gets applied again.
• Fixes modification of an entity' location not actually applying.
• Fixes ExprCoordinate's TriggerItem exiting a loop when a variable throws UnsupportedOperationException during conversion. (This currently still silently errors (not at the fault of the PR), this is bad. There are probably other parts of Skript that don't properly handle change, will address in a new issue)

Notes:

• Tests cannot be applied due to entity teleportation being a tick later.
• Local variables don't remember their source expression currently. Somewhere in the new conversion system, the source gets lost. This means the original issue of using a variable doesn't work, but using the raw expression last spawned wither will work now. Re-add local variable type hints #5457 adds a solution to this. Example is the source of the variable {_wither} should be the entity, but the source is always the converted Location.

---

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

[sovdeeth]

I don't really see the point of this change. Why does the entity changer need a special Internal mode to allow it to be teleported via changing the coords? Why not just have it implement SET and allow the exact same thing, AND setting the location directly. More to the point, is that even safe behavior at all? Teleportation isn't necessarily instant, affects things like passengers, and I think it's better done explicitly rather than hidden behind setting locations/coordinates.

[TheLimeGlass]

I don't really see the point of this change. Why does the entity changer need a special Internal mode to allow it to be teleported via changing the coords? Why not just have it implement SET and allow the exact same thing, AND setting the location directly. More to the point, is that even safe behavior at all? Teleportation isn't necessarily instant, affects things like passengers, and I think it's better done explicitly rather than hidden behind setting locations/coordinates.

Because using SET calls the changer again, resulting in an un-needed iteration, and to avoid possible concurrency within the changer. Other syntaxes will need to use this change mode they can also re-apply their similar methodology.

Skript has had this way of manipulating the underlying element of an expression since the beginning. See WrapperExpression, see blocks, see enchant, see lore and see this coordinate expression that has been trying to do it, but ultimately fails because it doesn't have the fixes that are present in this pull request.

RandomSK had a fix for this known coordinate expression issue back in 1.7

[sovdeeth]

Because using SET calls the changer again, resulting in an un-needed iteration, and to avoid possible concurrency within the changer. Other syntaxes will need to use this change mode they can also re-apply their similar methodology.

Skript has had this way of manipulating the underlying element of an expression since the beginning. See WrapperExpression, see blocks, see enchant, see lore and see this coordinate expression that has been trying to do it, but ultimately fails because it doesn't have the fixes that are present in this pull request.

RandomSK had a fix for this known coordinate expression issue back in 1.7

How would implementing SET result in an un-needed iteration? It would follow the exact same path you've made for INTERNAL.
I understand that changers can act on underlying expressions, i just don't see the point in adding INTERNAL. It's doing the exact same job as SET in the exact same way.

I still have concerns about a changer causing a teleportation, too.

[TheLimeGlass]

Because using SET calls the changer again, resulting in an un-needed iteration, and to avoid possible concurrency within the changer. Other syntaxes will need to use this change mode they can also re-apply their similar methodology.
Skript has had this way of manipulating the underlying element of an expression since the beginning. See WrapperExpression, see blocks, see enchant, see lore and see this coordinate expression that has been trying to do it, but ultimately fails because it doesn't have the fixes that are present in this pull request.
RandomSK had a fix for this known coordinate expression issue back in 1.7

How would implementing SET result in an un-needed iteration? It would follow the exact same path you've made for INTERNAL. I understand that changers can act on underlying expressions, i just don't see the point in adding INTERNAL. It's doing the exact same job as SET in the exact same way.

I still have concerns about a changer causing a teleportation, too.

It'll recollect all the expressions again

[sovdeeth]

Because using SET calls the changer again, resulting in an un-needed iteration, and to avoid possible concurrency within the changer. Other syntaxes will need to use this change mode they can also re-apply their similar methodology.
Skript has had this way of manipulating the underlying element of an expression since the beginning. See WrapperExpression, see blocks, see enchant, see lore and see this coordinate expression that has been trying to do it, but ultimately fails because it doesn't have the fixes that are present in this pull request.
RandomSK had a fix for this known coordinate expression issue back in 1.7

How would implementing SET result in an un-needed iteration? It would follow the exact same path you've made for INTERNAL. I understand that changers can act on underlying expressions, i just don't see the point in adding INTERNAL. It's doing the exact same job as SET in the exact same way.
I still have concerns about a changer causing a teleportation, too.

It'll recollect all the expressions again

Elaborate, please, I'm not sure what you mean.

[TheLimeGlass]

Elaborate, please, I'm not sure what you mean.

If the original intake change mode is ADD, and you use the SET again on the coordinate expression. It'll then go through the SET change mode of the coordinate expression, and then go to the ClassInfo changer.

INTERNAL goes from ADD on coordinate straight to the classinfo, no need to call SET on the coordinate expression again.

[sovdeeth]

SET isn't called again on the coord expression previously, either. The child expression (the entity, in this case) is what it's called on.
Previously: Coords ADD -> ExprEntity SET -> Entity SET
Now: Coords ADD -> ExprEntity INTERNAL -> Entity INTERNAL
The only difference is the name.

[TheLimeGlass]

SET isn't called again on the coord expression previously, either. The child expression (the entity, in this case) is what it's called on. Previously: Coords ADD -> ExprEntity SET -> Entity SET Now: Coords ADD -> ExprEntity INTERNAL -> Entity INTERNAL The only difference is the name.

Another example of proper usage of the INTERNAL changemode would be lore. You can add and remove lore using ItemMeta to add to the lore list, so after manipulation it would be beneficial to add a lore and not set the whole lore list again.

[sovdeeth]

SET isn't called again on the coord expression previously, either. The child expression (the entity, in this case) is what it's called on. Previously: Coords ADD -> ExprEntity SET -> Entity SET Now: Coords ADD -> ExprEntity INTERNAL -> Entity INTERNAL The only difference is the name.

Another example of proper usage of the INTERNAL changemode would be lore. You can add and remove lore using ItemMeta to add to the lore list, so after manipulation it would be beneficial to add a lore and not set the whole lore list again.

How does INTERNAL work there, too? does it suddenly act like ADD instead of SET? Either way, if you add to the lore list, you'll still have to call SET on the source expression since you're not sure if you got a copy or the true itemstack from the expression. I don't see the benefit there either. INTERNAL feels like an ambiguous mode that doesn't actually do anything new.

[TheLimeGlass]

How does INTERNAL work there, too? does it suddenly act like ADD instead of SET? Either way, if you add to the lore list, you'll still have to call SET on the source expression since you're not sure if you got a copy or the true itemstack from the expression. I don't see the benefit there either. INTERNAL feels like an ambiguous mode that doesn't actually do anything new.

INTERNAL allows for non user access in this case. We don't want the entity changer to be allowed to use SET with the location, they should be using the teleport effect, but when it comes to adding coordinates to a location, Skript was setup to allow the entity location to also be manipulated and then teleport said entity, but it doesn't work. So what this fix does is uses the INTERNAL change mode to do the teleportation, without allowing the user to set the entity location, while still keeping the underlying ability to add to a location.

Using SET with a location and the entity changer, would not be possible without allowing the user to also do so.

[sovdeeth]

How does INTERNAL work there, too? does it suddenly act like ADD instead of SET? Either way, if you add to the lore list, you'll still have to call SET on the source expression since you're not sure if you got a copy or the true itemstack from the expression. I don't see the benefit there either. INTERNAL feels like an ambiguous mode that doesn't actually do anything new.

INTERNAL allows for non user access in this case. We don't want the entity changer to be allowed to use SET with the location, they should be using the teleport effect, but when it comes to adding to a location, Skript was setup to allow the entity location to also be manipulated and then teleport said entity, but it doesn't work. So what this fix does is uses the INTERNAL change mode to do the teleportation, without allowing the user to set the entity location, while still keeping the underlying ability to add to a location.

Okay that makes sense. I'm still concerned about the suitability of a teleport changer at all, and I don't like that INTERNAL is a separate mode that doesn't say anything about what it does. I would rather see internal changers be flagged separately, like acceptsInternalChange or similar. I don't like having INTERNAL be so ambiguous and I don't think duplicating all the modes a la INTERNAL_SET would be a good option either.

[TheLimeGlass]

Okay that makes sense. I'm still concerned about the suitability of a teleport changer at all, and I don't like that INTERNAL is a separate mode that doesn't say anything about what it does. I would rather see internal changers be flagged separately, like acceptsInternalChange or similar. I don't like having INTERNAL be so ambiguous and I don't think duplicating all the modes a la INTERNAL_SET would be a good option either.

INTERNAL is only a SET, and well, it's for internal use only. It has JavaDocs and I don't see it be confusing. Similar to the internal only usage annotation.

All other changers boil down to a SET like you said originally about using SET after the ADD and REMOVE. So you don't need to add every other changer element when it's coming from those.

while event-entity is valid:
    add 1 to y-coordinate of event-entity
    wait a tick
    # kill entity at a y threshold

Is so much easier than needing to get and manipulate the location and then put through the teleport effect.

[sovdeeth]

Okay that makes sense. I'm still concerned about the suitability of a teleport changer at all, and I don't like that INTERNAL is a separate mode that doesn't say anything about what it does. I would rather see internal changers be flagged separately, like acceptsInternalChange or similar. I don't like having INTERNAL be so ambiguous and I don't think duplicating all the modes a la INTERNAL_SET would be a good option either.

INTERNAL is only a SET, and well, it's for internal use only. It has JavaDocs and I don't see it be confusing. Similar to the internal only usage annotation.

All other changers boil down to a SET like you said originally about using SET after the ADD and REMOVE. So you don't need to add every other changer element when it's coming from those.

But like you said earlier, even though the example of lore isn't great, an internal ADD method could be quite useful for some situations to prevent having to recreate something and call SET. I think it's more flexible and forward-thinking that way. If others disagree, I am fine with it being INTERNAL_SET, but just INTERNAL alone is way too ambiguous.

However, the core problem this is solving, setting entity locations, is still one i don't think should be solved like this. teleportations should be done knowing that the entity is being teleported, ie, using the teleport effect.