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

Event Context #7523

Draft
wants to merge 1 commit into
base: dev/feature
Choose a base branch
from

Conversation

TheAbsolutionism
Copy link
Contributor

@TheAbsolutionism TheAbsolutionism commented Jan 25, 2025

Description

First Intention: Improve EventValues Registration

This PR implements EventContext and EventValueContext that completely overwrites the EventValues class and changes how event values are created, stored and accessed.
When registering an event value, an EventContext will be created for the event and time state (if not already created) and create a new EventValueContext to be stored within the EventContext.
This allows for direct checking and grabbing, rather than the old system of looping all EventValueInfos and crosschecking the stored event class and stored event value class.
Variable names of the methods and stored data now reflect exactly what they are and how they are used.
When parsing a script that uses an event-value, the current system checks if an exact value and converter is present, if not, it will do 4 extensive loop checking ensuring a value can be grabbed. But will do it for each instance of the event-value.
Now, after the first instance, data will be stored in the correlating EventContext skipping the extensive checking for every instance afterwards.

Second Intention: Runtime Context (Replacing Event)

-deleted-

Third Intention: Original Event Value

With the implementation of the prior intentions, this brings along a little syntax update to ExprEventExpression allowing to get the original event value of when an event is called.
This is done by, when an Event is first triggered from SkriptEventHandler it grabs the EventContext correlating to the triggered Event. It's worth noting at this time that EventContext and EventValueContext are templates. When successfully grabbing the EventContext it is proceeded to be built which loops through the stored EventValueContexts, building each one, grabbing and storing the event-value if it contains only a singular converter (Meaning it wont error from "multiple values for this event").

Comments

This PR is marked as draft because I still have a lot of testing to do to ensure it's viability.
However, I wanted to atleast expose this implementation and idea to obtain feedback (granted it's not negative feedback) that way I can further solidify it.

Thank you for coming to my TED Talk.


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

@sovdeeth
Copy link
Member

sovdeeth commented Jan 25, 2025

My big concern is this continues tying Skript close to Bukkit, which is not ideal. The goal is to avoid tying skript to bukkit, meaning runtime context should not in any way depend on Events.

@sovdeeth
Copy link
Member

sovdeeth commented Jan 25, 2025

master...feature/context-overhaul#diff-b3182d8814ddd5d8ddb93f6b461679a680997ae4ddcaa4b3d6b87d24479cdc6bR28
May be useful to reference.

Personally, I would like to see a TriggerContext with an EventContext as a field within it, rather than the current behavior. That said, runtime context is a major thing pickle intends to be working on in the next few months, so you may want to coordinate with him about this.

@APickledWalrus
Copy link
Member

As sovde mentioned, a rework of the execution system is something I have on my radar for this year. For this kind of major effort/change, it is likely best to be led by a core team member.

@TheAbsolutionism
Copy link
Contributor Author

@sovdeeth @APickledWalrus
I will be repurposing this PR probably tomorrow. Undoing the changes from Second Intention

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants