Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduces toggle effect & expression #4154

Open
wants to merge 104 commits into
base: dev/feature
Choose a base branch
from

Conversation

Olyno
Copy link
Contributor

@Olyno Olyno commented Jul 3, 2021

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: /

@Mwexim
Copy link
Contributor

Mwexim commented Jul 3, 2021

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.

Copy link
Member

@TPGamesNL TPGamesNL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expression's javadocs for change and acceptChange should be updated

@Olyno
Copy link
Contributor Author

Olyno commented Jul 3, 2021

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? 😃

@Mwexim
Copy link
Contributor

Mwexim commented Jul 3, 2021

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.

@Olyno
Copy link
Contributor Author

Olyno commented Jul 3, 2021

The pattern should become toggle %booleans%

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

@Mwexim
Copy link
Contributor

Mwexim commented Jul 3, 2021

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?

@OGContent
Copy link

OGContent commented Jul 3, 2021

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?

@Mwexim
Copy link
Contributor

Mwexim commented Jul 4, 2021

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.

@AyhamAl-Ali
Copy link
Member

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.

@Mwexim
Copy link
Contributor

Mwexim commented Jul 4, 2021

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.

@AyhamAl-Ali
Copy link
Member

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
Copy link
Member

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?

@TPGamesNL
Copy link
Member

@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

@AyhamAl-Ali
Copy link
Member

@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?

@TPGamesNL
Copy link
Member

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

@AyhamAl-Ali
Copy link
Member

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.

@Olyno
Copy link
Contributor Author

Olyno commented Jul 4, 2021

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)

@TPGamesNL TPGamesNL added the enhancement Feature request, an issue about something that could be improved, or a PR improving something. label Jul 6, 2021
@Olyno
Copy link
Contributor Author

Olyno commented Jul 16, 2021

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

@Olyno Olyno marked this pull request as draft July 16, 2021 09:34
@pauserratgutierrez

This comment was marked as off-topic.

@sovdeeth

This comment was marked as off-topic.

@pauserratgutierrez

This comment was marked as off-topic.

@sovdeeth

This comment was marked as off-topic.

Copy link
Member

@Moderocky Moderocky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good

@sovdeeth sovdeeth added the feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. label Apr 4, 2024
Copy link
Member

@APickledWalrus APickledWalrus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mainly formatting/style recommendations.

src/main/java/ch/njol/skript/effects/EffToggle.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffToggle.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffToggle.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffToggle.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffToggle.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffToggle.java Outdated Show resolved Hide resolved
Comment on lines 112 to 115
List<Object> filteredValues = toggledValues.stream().filter(
obj -> ChangerUtils.acceptsChange(toggledExpr, ChangeMode.SET, obj.getClass())
).collect(Collectors.toList());
toggledExpr.change(event, filteredValues.toArray(), ChangeMode.SET);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might need to note a behavior change here. For changing something like a list, I believe the indexes will now be lost, which was not prior behaviors. We might choose to only rewrite the list if it contains a boolean?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you're talking about the list, are you talking about the toggledExpr or the filteredValues, or even both?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we have a list like {_values::*} with:
{_values::pickle::name} = "APickledWalrus"
{_values::pickle::likesCake} = true
if we then do toggle {_values::*}, the indices such as name and likesCake will be lost (replaced by 1 and 2).
This is probably unavoidable. However, if we have a list like
{_values::pickle::name} = "APickledWalrus"
{_values::pickle::githubName} = "APickledWalrus"
if we then do toggle {_values::*}, there are no booleans to toggle, meaning we should not update the list (and cause it to lose its indices)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the code, don't i already do it?

https://github.com/SkriptLang/Skript/pull/4154/files#diff-4cdf11cdcad1ff81963a3380536868678c5781162395d093ecd42b33c71add01R74

Also, tests are failing, did i misunderstand what you meant?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There may have been some confusion, sorry.

Given a list containing "one" and "two", doing toggle {_list::*} should not modify the list since there is nothing to toggle (meaning the indexes will be preserved).

However, given a list containing "one", "two" and true, doing toggle {_list::*} should modify the list since the third value is a boolean (meaning the indexes will be lost).

I think the fix you made is correct, but the test is wrong.

Working with the testing you added, you should be testing:

set {_list::myFirstIndex::myFirstValue} to "hello"
set {_list::mySecondIndex::mySecondValue} to "world"
toggle {_list::*}
assert {_list::myFirstIndex::myFirstValue} is "hello" with "{_list::myFirstIndex::myFirstValue} should be toggled to 'hello'"
assert {_list::mySecondIndex::mySecondValue} is "world" with "{_list::mySecondIndex::mySecondValue} should be toggled to 'world'"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: shouldn't we throw an error instead? If the user tries to toggle strings, it shouldn't work. If the variable list includes string, but also boolean, shouldn't we throw an error? Note that i don't think we should throw an error if all values are togglables.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, we can't really do runtime errors. I don't think it is just an issue to ignore the values and only change the ones that are actually togglable. You could also opt to not make any changes if there is an invalid type.

The main point still stands, though. If the list contains only blocks, we do not want to actually call the change method as it will reset the indices.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you confirm me the commit 8bc1de4 fixes this unexpected behavior?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks reasonable
you may want to check out #7120 changeInPlace utility method

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would need a usage example please. That being said, i will wait for this PR to be merged before doing any change regarding this change.

In terms of organization, either the changeInPlace PR is merged before this one, and so i will apply the change here, or this PR is merged before, and so i will apply the change in the other PR.

In any case, we need to merge one of them to move on the other.

Olyno and others added 9 commits June 3, 2024 19:35
Co-authored-by: Patrick Miller <apickledwalrus@gmail.com>
Co-authored-by: Patrick Miller <apickledwalrus@gmail.com>
Co-authored-by: Patrick Miller <apickledwalrus@gmail.com>
Co-authored-by: Patrick Miller <apickledwalrus@gmail.com>
Co-authored-by: Patrick Miller <apickledwalrus@gmail.com>
@APickledWalrus APickledWalrus removed the feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. label Jul 1, 2024
@APickledWalrus APickledWalrus mentioned this pull request Sep 5, 2024
1 task
@sovdeeth sovdeeth added the 2.10 Targeting a 2.10.X version release label Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.10 Targeting a 2.10.X version release enhancement Feature request, an issue about something that could be improved, or a PR improving something.
Projects
None yet
Development

Successfully merging this pull request may close these issues.