Skript icon indicating copy to clipboard operation
Skript copied to clipboard

Introduces toggle effect & expression

Open Olyno opened this issue 4 years ago • 38 comments

Description

This pull request introduces the toggle effect and expression.

set {_var} to true
toggle {_var} # {_var} is now false

set {_var} to inverse of {_var} # {_var} is now true

Target Minecraft Versions: / Requirements: / Related Issues: /

Olyno avatar Jul 03 '21 15:07 Olyno

I have the same question I had with Delta’s PR initially: why would you add a new ChangeMode value? Toggling a boolean is essentially equal to setting the boolean to its opposite value.

If anyone can give me one type that could use toggling except booleans, then go ahead, but in my opinion this should just be added as a separate effect and use the set-operation to perform the action.

Mwexim avatar Jul 03 '21 18:07 Mwexim

why would you add a new ChangeMode value?

It's more intuitive and makes more sense than toggle the value manually. For example:

set flight mode to false # Bruh
toggle flight mode # This is good

Toggling a boolean is essentially equal to setting the boolean to its opposite value

Yes it is, but what's the most intuitive for users? Having a boolean "state" expression, or use a simple toggle?

in my opinion this should just be added as a separate effect and use the set-operation to perform the action

I did that in a first time, but try to reuse an effect in addons, this is just impossible or really tricky/hard to do. What we would like is a simple api for addons developers that anyone could use it easily. Adding a mode makes things easier than adding an effect, and trying to use this effect. I would like add too a changer is already an effect. Wouldn't be better to group up all changer effects at the same place? 😃

Olyno avatar Jul 03 '21 20:07 Olyno

You're understanding me wrong. What I'm saying is that the internal structure of this feature does not make sense to me.

  1. You're adding a new ChangeMode while toggling is essentially setting the value to its opposite: Ex. toggle flying state This should not be handled as 'ChangeMode.TOGGLE' but rather as 'ChangeMode.SET'.
  2. Only booleans can be toggled, unless someone can give me an example where another object could be toggled. The pattern should become toggle %booleans%, which essentially means that whenever a boolean expression can be set to something, it can also be set to its opposite, which is essentially what toggling means.

Mwexim avatar Jul 03 '21 21:07 Mwexim

The pattern should become toggle %booleans%

Yeah, but here is the problem: how do you manage that with addons?

Olyno avatar Jul 03 '21 22:07 Olyno

The pattern should become toggle %booleans%

Yeah, but here is the problem: how do you manage that with addons?

Addons that register boolean expressions can just accept SET as a valid change mode?

Mwexim avatar Jul 03 '21 22:07 Mwexim

Addons that register boolean expressions can just accept SET as a valid change mode?

Hey, I don't really get what you mean, why shouldn't TOGGLE be a mode? also do you mean the TOGGLE should be like set {variable} to toggle {variable} or what?

OGContent avatar Jul 03 '21 22:07 OGContent

Addons that register boolean expressions can just accept SET as a valid change mode?

Hey, I don't really get what you mean, why shouldn't TOGGLE be a mode? also do you mean the TOGGLE should be like set {variable} to toggle {variable} or what?

Not really. I am advocating for the removal of this ChangeMode because toggling equals setting a value to its opposite. This means that all boolean expressions that accept 'set' as a valid ChangeMode, can be toggled, since, again, toggling equals setting a value to its opposite.

Mwexim avatar Jul 04 '21 08:07 Mwexim

I think both ways are useful as a GhangeMode and as an expression.

because toggling equals setting a value to its opposite.

I don't think this is correct, it is technically setting to it's opposite but to do this manually you will need a work-around and it will be longer that just toggle ... or toggled ... which makes it much easier if you use it a lot.

AyhamAl-Ali avatar Jul 04 '21 08:07 AyhamAl-Ali

I think both ways are useful as a GhangeMode and as an expression.

because toggling equals setting a value to its opposite.

I don't think this is correct, it is technically setting to it's opposite but to do this manually you will need a work-around and it will be longer that just toggle ... or toggled ... which makes it much easier if you use it a lot.

You are not understanding me correctly but at this point I think it doesn’t matter anymore.

Mwexim avatar Jul 04 '21 09:07 Mwexim

You are not understanding me correctly but at this point I think it doesn’t matter anymore.

I would like to understand your opinion of course, let me know more. It does matter :)

AyhamAl-Ali avatar Jul 04 '21 09:07 AyhamAl-Ali

My point is that you can toggle using current SET mode like this

set flight state of player to false if flight state of player is true else true
# OR
if flight state of player is true:
  set flight state of player to false
else:
  set flight state of player to true

Which works perfectly but adding

toggle flight state of player
send "Flight state has been set to %toggle flight state of player%"
# OR
set flight state toggled {Flight::%uuid of player%}

Makes it easier in some cases if not many, Am I correct?

AyhamAl-Ali avatar Jul 04 '21 09:07 AyhamAl-Ali

@Mwexim's point is that toggle flight state of player does not need a new ChangeMode, but can be done with using get methods and the SET ChangeMode

TPGamesNL avatar Jul 04 '21 09:07 TPGamesNL

@Mwexim's point is that toggle flight state of player does not need a new ChangeMode, but can be done with using get methods and the SET ChangeMode

But with that it won't be possible to do toggle flight state of player because toggle is GhangeMode here right?

AyhamAl-Ali avatar Jul 04 '21 09:07 AyhamAl-Ali

But with that it won't be possible to do toggle flight state of player because toggle is ChangeMode here right?

Yes that would be possible, TOGGLE is not a ChangeMode in that code: https://pastebin.com/mEvXw429

TPGamesNL avatar Jul 04 '21 09:07 TPGamesNL

Yes that would be possible, TOGGLE is not a ChangeMode in that code: https://pastebin.com/mEvXw429

If that works well I think it is a better idea, because it will be easier and no need to add TOGGLE ChangeMode to all boolean type expressions manually.

AyhamAl-Ali avatar Jul 04 '21 09:07 AyhamAl-Ali

expression.change(e, new Boolean[] {!b}, ChangeMode.SET);

Oh okay, that's what I didn't understand, how we could do that without a mode. It seems to be a good way to do it. I'm gonna probably change for that (and adding a new expression if the community wants to)

Olyno avatar Jul 04 '21 14:07 Olyno

This pull request is not ready to be review due to some changes to do. I convert it to draft during this time.

Olyno avatar Jul 16 '21 09:07 Olyno

Hi everyone 👋🏻 Time to finish this pull request after months of inactivity. Hopefully this time is the right one!

Olyno avatar Oct 05 '22 00:10 Olyno

Testing scripts are failing @Olyno

AyhamAl-Ali avatar Feb 03 '23 01:02 AyhamAl-Ali

Any updates on this nice PR? :)

AyhamAl-Ali avatar Apr 25 '23 14:04 AyhamAl-Ali

Tests are failing due to

Failed: change modes (on 4 environments) {_list::} should be set to false and {_block} (on paper-1.17.1) {_list::} should be set to false and {_block} (on paper-1.18.2) {_list::} should be set to false and {_block} (on paper-1.19.3) {_list::} should be set to false and {_block} (on paper-1.19.4)

AyhamAl-Ali avatar Apr 26 '23 00:04 AyhamAl-Ali

Indeed, I fix that in the week

Olyno avatar Apr 26 '23 09:04 Olyno

Ready for review @AyhamAl-Ali

Olyno avatar May 01 '23 10:05 Olyno

Description example should adapt the new syntax change inverse|opposite

AyhamAl-Ali avatar May 01 '23 13:05 AyhamAl-Ali

I didn't understand your comment about the description, could you please explain it again?

Olyno avatar May 01 '23 17:05 Olyno

I didn't understand your comment about the description, could you please explain it again?

In this PR's description, you wrote this example

set {_var} to toggled {_var} # {_var} is now true

which should now be

set {_var} to inverse of {_var} # {_var} is now true

Due to the change in syntax in this PR

AyhamAl-Ali avatar May 01 '23 22:05 AyhamAl-Ali

Description is updated 👍🏻

Olyno avatar May 02 '23 19:05 Olyno

i think turn %objects% %boolean% should be included too

JakeGBLP avatar Oct 14 '23 00:10 JakeGBLP

i think turn %objects% %boolean% should be included too

Putting two inputs next to each other will create a parsing nightmare.

Moderocky avatar Oct 14 '23 09:10 Moderocky

i think turn %objects% %boolean% should be included too

Putting two inputs next to each other will create a parsing nightmare.

The syntax is also not the most sounding. turn player's flight mode yes doesn't make any sense

UnderscoreTud avatar Oct 14 '23 09:10 UnderscoreTud