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

Compound Conditionals API #7057

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

Conversation

sovdeeth
Copy link
Member

@sovdeeth sovdeeth commented Sep 8, 2024

Description

This PR adds a new interface, Conditional, which Conditions now implement. It's intended to provide more flexibility to evaluable elements within Skript that deal with trues and falses.

It deals in Kleeneans, allowing us to return Unknown from conditionals which are deemed invalid, or ambiguous, or otherwise. Currently, nothing takes advantage of this feature. Conditions simply forward the boolean result of check() into a Kleenean.

It comes with default methods for and, or, and not, which attempt to minimize evaluations wherever possible.

There is also the CompoundConditional class, which is the main reason for this PR. It represents a set of Conditionals all joined by an operator: a && b && c, or b || d, or the special case of NOT, where only one conditional is allowed: !a. The motivation for this ability is the upcoming raytracing PR, which will feature complex conditional cases due to the proposed ignore sections, thought it could be used to expand on existing SecConditional features, or in filtering.

Conditional also comes with a builder that will automatically generate a DNF (see here) CompoundConditional from a combination of ANDs, ORs, and NOTs:

Conditional.builder()
    .and(condA)
    .or(condB)
    .andNot(compoundCondC)
    .build()

Current issues: Since DNF takes advantage of the result of a condition being used in multiple places, this could lead to conditions being evaluated multiple times since there's no sharing of state over the hierarchy. May just want to scrap the DNF idea. A solution could be passing a Context down the evaluation tree which has a map of evaluated conditions.
Also some of the docs are outdated or missing, WIP.


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

@sovdeeth sovdeeth added the enhancement Feature request, an issue about something that could be improved, or a PR improving something. label Sep 8, 2024
@sovdeeth sovdeeth marked this pull request as ready for review September 9, 2024 04:28
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.

Really good concept, implementation looks solid. I have some thoughts about how the design could be tweaked, but it's just my own opinions. We can discuss further

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 think this is looking pretty good. I only have a few thoughts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

4 participants