ExprCopy + CondCanCopy

[Absolutionism]

Problem

There was no expression to grab copies of objects. There was no way to check if an object was copyable or not.

Solution

This PR adds ExprCopy allowing an expression usage of grabbing copies of objects. If an object can not be copied/cloned then that object will be excluded. Alternatively, users can opt in to return the original object if it can not be copied, by adding with fallback

This PR adds CondCanCopy allowing users to check if an object can be copied or not. Which would further help debugging and if they need to use with fallback when using ExprCopy

Testing Completed

ExprCopy.sk and CondCanCopy.sk

Supporting Information

N/A

---

Completes: #7874 Related: none

[Fusezion]

is there any major benefit to using the condition than always using with fallback?

[sovdeeth]

A few concerns here

• the tests hardly ever test that the copy actually copied the orginal. Except for the vector, they don't check if the copies are actually separate. (edit: I missed that itemstack does too, but the tests do still seem a bit anemic)
• This seems very limited in scope, since set {_x} to y is identical to set {_x} to copy of y. There's use in set <not a variable> to a copy of y but that's pretty niche for most cases.

There was no expression to grab copies of objects. There was no way to check if an object was copyable or not.

Can you elaborate why this was a problem? What issues were present, what did you need a copy for? I know it's kind of intended for item components, but if you have the components using a cloner, they'll be constantly copied whenever they're added to a variable regardless, which would cause some issues when using them.

I think for this to actually be useful, it needs to copy things that don't normally get copied. You could do this via additions to the cloner like shouldAlwaysClone() or something like that, but I think as is this expression adds little value

[Absolutionism]

the tests hardly ever test that the copy actually copied the orginal. Except for the vector, they don't check if the copies are actually separate. (edit: I missed that itemstack does too, but the tests do still seem a bit anemic)

I have tests for ItemStack, Location and Vector for checking that the objects are actually separate. Unfortunately can't do it for BlockData because there is currently no generic blockdata modifier.

This seems very limited in scope, since set {_x} to y is identical to set {_x} to copy of y. There's use in set to a copy of y but that's pretty niche for most cases.

Although internally Skript already basically does this when setting a variable, users may not know nor realize this. By exposing this functionality, users can safely ensure themselves that they are indeed grabbing and modifying a copy. Plus with the addition of the condition, users can debug what and what can not be copied through Skript itself and possibly even attempt to make their own copies.

Can you elaborate why this was a problem? What issues were present, what did you need a copy for?

Including all things stated in the previous reply section. There was no way of getting a copy using an expression, and as you said, Skripts internal cloning usage would not be done for setting non variables. Sure the EffCopy is still available to users, but would just be creating an extra line when it can be done in one,

I know it's kind of intended for item components, but if you have the components using a cloner, they'll be constantly copied whenever they're added to a variable regardless, which would cause some issues when using them.

Though not tested, I believe this would solely be the problem of the already existing functionality and not caused by my PR.

I think for this to actually be useful, it needs to copy things that don't normally get copied. You could do this via additions to the cloner like shouldAlwaysClone()

I can definitely add this to help mitigate the problem caused by Skript.

but I think as is this expression adds little value

Truthfully, I wish you would've voiced these concerns when we were discussing it in Discord yesterday.

[sovdeeth]

Truthfully, I wish you would've voiced these concerns when we were discussing it in Discord yesterday.

Honestly I didn't have these concerns until I saw the feature as a whole, though i did mention the idea of having a cloner that's not normally used outside of an explicit copy syntax. Sorry if it feels like an about face, I didn't mean to mislead you or anything.

Including all things stated in the previous reply section. There was no way of getting a copy using an expression, and as you said, Skripts internal cloning usage would not be done for setting non variables. Sure the EffCopy is still available to users, but would just be creating an extra line when it can be done in one,

I'm still not really sure what problems it solves. I see two things so far:

• It's not really clear to the user when Skript clones something and when it stays the same, so an explicit copy mechanic could help.
• Setting non-variables to something doesn't necessarily copy it. (This generally should be handled by the changed expression imo)

I think trying to address those two is good, but I think that scope's a bit limiting and doesn't really help the end user much. I don't really ever see people trying to get help with copying thing and I struggle to see what actual use cases users need this for that set {_x} to y doesn't fill. Like set {_item} to tool of player, there's no real need for to copy of tool... and I think it muddies the waters for users who may think that without it, it doesn't copy. EffCopy was mainly created to allow a variable list to be copied with indices rather than primarily a way to copy values, and this doesn't really fill the same role.

I don't know, I'm not totally against this, I just am struggling to come up with any concrete scenarios where it'd be useful. The one case I can come up with is setting metadata doesn't set it to a clone, so there could be aliasing issues there, but that seems more a flaw in the metadata expression than something that should be solved by adding a copy expression.

[Efnilite]

I'm not sure when this would be used. Do you have any example use cases? I think that, for the end user, passing expressions by reference is less intuitive than passing by value. Therefore, the solution to this problem is to make all expressions return cloned objects. I feel like this would only lead users into unnecessarily copying things when it's not required.

The fact that not all objects can be cloned also doesn't help. If this were to be added, there should at least be a list of types you can clone with this expression in the description.

[Absolutionism]

Do you have any example use cases?

https://github.com/SkriptLang/Skript/pull/7194