-
-
Notifications
You must be signed in to change notification settings - Fork 382
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
Arithmetic Rework #5279
Arithmetic Rework #5279
Conversation
…feature/arithmetic-rework
…feature/arithmetic-rework
src/main/java/org/skriptlang/skript/lang/arithmetic/Operator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/skriptlang/skript/lang/arithmetic/DifferenceInfo.java
Outdated
Show resolved
Hide resolved
src/main/java/ch/njol/skript/expressions/arithmetic/ArithmeticChain.java
Outdated
Show resolved
Hide resolved
src/main/java/ch/njol/skript/expressions/arithmetic/ExprArithmetic.java
Outdated
Show resolved
Hide resolved
src/main/java/ch/njol/skript/expressions/arithmetic/ExprArithmetic.java
Outdated
Show resolved
Hide resolved
src/main/java/org/skriptlang/skript/lang/arithmetic/Arithmetics.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks ok.
src/main/java/org/skriptlang/skript/lang/arithmetic/Arithmetics.java
Outdated
Show resolved
Hide resolved
src/main/java/org/skriptlang/skript/lang/arithmetic/Arithmetics.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Patrick Miller <apickledwalrus@gmail.com>
# Conflicts: # src/main/java/ch/njol/skript/lang/Variable.java
This pull request is changing so much. I don't believe this should be included in 2.7. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The non-number behavior is a lot better but still not quite right. It does work as expected for null values, but not when the variable is a string.
set {_y} to "2"
broadcast "%{_y} + 1 + 1% = 2" # broadcasts 1
broadcast "%1 + {_y} + 1% = 2" # broadcasts 2
The same thing occurs for vectors. It seems to depend on what the left operators is:
<none> = <0, 0, 0>
I've attached the full test file to this report.
arithmetic-test.txt
So far I've been testing assuming the null values should be the 0 for whatever they are, but for the following example I'm not sure what the answer should be:
{_vector} + {_number}
Should it be {_vector} + vector(0,0,0)
? or 0 + {_number}
? I'm not sure which would be preferable, and it gets worse with chained oop stuff. Perhaps any invalid operation should just return NaN or none and leave it as that. It'd be breaking, but it'd be consistent.
This is the same with like {_timespan} + 1
. Reading the code w/out context makes it seem like it should be a number, but it'll actually be a timespan.
src/main/java/ch/njol/skript/classes/data/DefaultOperations.java
Outdated
Show resolved
Hide resolved
src/main/java/ch/njol/skript/classes/data/DefaultOperations.java
Outdated
Show resolved
Hide resolved
src/main/java/ch/njol/skript/classes/data/DefaultOperations.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't find anything else
nice work tud
arithmetic-test.txt
src/main/java/ch/njol/skript/classes/data/DefaultOperations.java
Outdated
Show resolved
Hide resolved
# Conflicts: # src/main/java/ch/njol/skript/expressions/ExprVectorArithmetic.java
src/main/java/ch/njol/skript/expressions/arithmetic/ArithmeticChain.java
Show resolved
Hide resolved
src/main/java/ch/njol/skript/expressions/arithmetic/ArithmeticChain.java
Outdated
Show resolved
Hide resolved
src/main/java/ch/njol/skript/expressions/arithmetic/ArithmeticChain.java
Outdated
Show resolved
Hide resolved
src/main/java/ch/njol/skript/expressions/arithmetic/ArithmeticGettable.java
Show resolved
Hide resolved
src/main/java/org/skriptlang/skript/lang/arithmetic/OperationInfo.java
Outdated
Show resolved
Hide resolved
src/main/java/org/skriptlang/skript/lang/arithmetic/OperationInfo.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Patrick Miller <apickledwalrus@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's ready for primetime
# Conflicts: # src/main/java/ch/njol/skript/classes/ClassInfo.java # src/main/java/ch/njol/skript/expressions/ExprDifference.java # src/main/java/ch/njol/skript/lang/Variable.java
Description
This PR is a rework of my other arithmetic rework 😂 (#5203), it has the same premise but all the internal stuff is better. This rework also has a different better registration system that makes commutative operations possible, e.g. (type1 - type2) and (type2 - type1) operations, the old one only could support (type1 - type2) due to how it was coded.
closes #5203
Target Minecraft Versions: any
Requirements: none
Related Issues: #4253