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

Arithmetic Rework #5203

Closed

Conversation

UnderscoreTud
Copy link
Member

@UnderscoreTud UnderscoreTud commented Nov 11, 2022

Description

This PR reworks the arithmetic system and expression, it allows addons (and skript) to very easily register their own custom arithmetic type; making the arithmetic expression more versatile. How you register an arithmetic is quite similar to how changers register, examples from the current registered arithmetics here, and they work similarly. An operator is given to an arithmetic then that arithmetic can return an array of classes to determine what can be used as the second value of the operation (same as Changers).

Note:
This is change may break addons that use the current arithmetic system, I tried to make it backwards compatible but there are no guarantees that it won't break them

Some examples of what can be done using this new system:

The current registered arithmetic types are:


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

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.

Overall, looks really solid! Great work.

There will probably be a few other things, but I'm going to ask around for some input first + see how it looks after current requested changes are made :)

@APickledWalrus APickledWalrus added the feature Pull request adding a new feature. label Nov 11, 2022
@Fusezion
Copy link
Contributor

So this just crossed my mind, order of operations are still done in order with this system right so 1 + 3 * 2 returns 7 not 8

@UnderscoreTud
Copy link
Member Author

Yes, they are still in order

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.

Looking really good now!

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.

I'm thinking this is okay now - another review or two is definitely necessary

Excellent work!

Also, I think we should try to add some tests if possible (not sure what currently exists)

@Moderocky
Copy link
Member

This should have comprehensive (if not exhaustive) tests written. :)

@UnderscoreTud UnderscoreTud marked this pull request as draft December 2, 2022 08:45
@UnderscoreTud
Copy link
Member Author

converted to draft while I fix some bugs with the new system

@UnderscoreTud UnderscoreTud mentioned this pull request Dec 26, 2022
@UnderscoreTud UnderscoreTud deleted the enhancement/arithmetic branch February 13, 2023 10:53
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants