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

Create clamp expression #6567

Closed
wants to merge 13 commits into from
Closed

Conversation

Phill310
Copy link
Contributor

Description

Creates a clamp expression to clamp values between a min and a max.
This is my first time working with skript so any help/tips would be appreciated :)


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

@Moderocky
Copy link
Member

I think I'd rather fix the function return behaviour than make a syntax for this.

Copy link
Contributor

@Fusezion Fusezion left a comment

Choose a reason for hiding this comment

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

Skimmed over it and this is all I saw for now

src/main/java/ch/njol/skript/expressions/ExprClamp.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/expressions/ExprClamp.java Outdated Show resolved Hide resolved
@Fusezion
Copy link
Contributor

I think I'd rather fix the function return behaviour than make a syntax for this.

there is no bug in function return behavior is works how it's intended to

Co-authored-by: Fusezion <fusezionstream@gmail.com>
@Moderocky
Copy link
Member

there is no bug in function return behavior is works how it's intended to

And I'm saying the behaviour is poor and could be done better.

@AyhamAl-Ali
Copy link
Member

I don't much like this addition as Functions are better fit for this.

@AyhamAl-Ali AyhamAl-Ali added the up for debate When the decision is yet to be debated on the issue in question label Apr 15, 2024
@Fusezion
Copy link
Contributor

I don't much like this addition as Functions are better fit for this.

Same could be said about the ExprRound class for rounded [1:up|2:down] %numbers%. This would work as a function if functions actually supported isSingle checks which rolls into Moderocky's mention.

And I'm saying the behaviour is poor and could be done better.

It can't be done better when the function literally can't support set {_blah} to clamp(10.6, 1, 10) this isn't exclusive to clamp when round function has the same flaw.

Copy link
Contributor

@Fusezion Fusezion left a comment

Choose a reason for hiding this comment

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

Whoops missed some points

src/main/java/ch/njol/skript/expressions/ExprClamp.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/expressions/ExprClamp.java Outdated Show resolved Hide resolved
@Moderocky
Copy link
Member

It can't be done better when the function literally can't support set {_blah} to clamp(10.6, 1, 10) this isn't exclusive to clamp when round function has the same flaw.

Maybe I didn't make it clear, I am recommending we fix the issue with variable arity returns not being permitted in single changers.
This would make all such functions work.

@Moderocky Moderocky changed the base branch from master to dev/feature April 15, 2024 08:13
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.

Even if we fix the function, I don't think that means we can't have an expression as well if people want both, but I think it has room for improvement.

src/main/java/ch/njol/skript/expressions/ExprClamp.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/expressions/ExprClamp.java Outdated Show resolved Hide resolved
Fix examples
Check if getSingle is null
Copy link
Contributor

@Fusezion Fusezion left a comment

Choose a reason for hiding this comment

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

Just incase this is still approved to be added which I see no reason not to speaking there's already a round expression too. There should be test added as well.

src/main/java/ch/njol/skript/expressions/ExprClamp.java Outdated Show resolved Hide resolved
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.

Do you think we could have some tests for this? Tests make the world go round!

@Phill310
Copy link
Contributor Author

I was looking over the clamp function tests but got confused by these three.

assert clamp(1, 0, NaN value) is not clamp(1, 0, NaN value) with "(single ints) min < value < NaN"
assert clamp(1, NaN value, 2) is not clamp(1, NaN value, 2) with "(single ints) NaN < value < max"
assert clamp(NaN value, 1, 2) is not clamp(NaN value, 1, 2) with "(single ints) min < NaN < max"

Why are they getting compared to themselves? I feel like these should be checking if the output is set.

@Moderocky
Copy link
Member

Why are they getting compared to themselves? I feel like these should be checking if the output is set.

If I remember correctly this was one of the ways to check for 'not a number' because it didn't test equal to itself. I don't know if that's still the case anymore and we may have a better way.

@sovdeeth
Copy link
Member

Why are they getting compared to themselves? I feel like these should be checking if the output is set.

If I remember correctly this was one of the ways to check for 'not a number' because it didn't test equal to itself. I don't know if that's still the case anymore and we may have a better way.

yes we have isNaN() now!

@Phill310
Copy link
Contributor Author

Phill310 commented Apr 27, 2024

In that case should it become assert isNaN(clamp(1, 0, NaN value)) is true with "(single ints) min < value < NaN"?
Or do we want it to be false. I don't really understand what the old check was aiming for

@sovdeeth
Copy link
Member

In that case should it become assert isNaN(clamp(1, 0, NaN value)) is true with "(single ints) min < value < NaN"? Or do we want it to be false. I don't really understand what the old check was aiming for

yes exactly
The original test took advantage of the fact that NaN does not equal NaN, but any other value should equal itself, to test for NaN

@Phill310 Phill310 requested review from Moderocky and sovdeeth April 27, 2024 17:03
@Phill310 Phill310 requested a review from sovdeeth May 2, 2024 03:42
Co-authored-by: sovdee <10354869+sovdeeth@users.noreply.github.com>
@sovdeeth sovdeeth added the feature Pull request adding a new feature. label May 31, 2024
@sovdeeth
Copy link
Member

Do we still want to move ahead with this now that the function is fixed? I like having the clamped above/below syntax.

@Phill310 Phill310 closed this Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Pull request adding a new feature. up for debate When the decision is yet to be debated on the issue in question
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants