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

Make actions use a list of events #34881

Closed
wants to merge 8 commits into from

Conversation

Plykiya
Copy link

@Plykiya Plykiya commented Feb 4, 2025

About the PR

Made all actions accept a list of events rather than just a single event

Why / Balance

Fixes #34599

Technical details

The conversion from xActionEvent to BaseEvent was previously just done through BaseEvent => Event
Since it's now List<xActionEvent> to List, we do the conversion by creating a new empty List<BaseEvent>, add each xActionEvent one by one, and then return the filled List<BaseEvent>

Whenever the event was submitted for performAction(), the event submitted most often was the xActionEvent instead of the BaseEvent. It was implicitly converted from xActionEvent to BaseEvent. Now that it's a list, we can instead just submit the action's BaseEvents field, which does the convertion from List<xActionEvent> automatically.

Every yml action was updated from using a single event to a list of events. Nothing was optimized, it was just converted.

Media

Display of actions working:

2025-02-04.12-10-43.mp4

Requirements

Breaking changes

BaseActionComponents's BaseActionEvent BaseEvent field was updated to List<BaseActionEvent> BaseEvents, and their specific Events, such as WorldTargetActionEvent for example, were converted to also be a list, such as List<WorldTargetActionEvent> Events>.

SharedActionSystems PerformAction()'s parameter BaseActionEvent actionEvent was updated to List<BaseActionEvent> actionEvents. You can typically submit the BaseEvents field whenever this is asked for.

To convert from a singular Event to a List<BaseActionEvent>, create a new List<BaseActionEvent>() and .Add() it as an item.

Update all yml actions from event to events, and make it a list.

@github-actions github-actions bot added S: Needs Review Status: Requires additional reviews before being fully accepted size/M Denotes a PR that changes 100-999 lines. S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. labels Feb 4, 2025
@Plykiya
Copy link
Author

Plykiya commented Feb 4, 2025

I was looking at the D1 issues and saw the other issue involving actions and decided this would be more logical to work on first.

Being able to make these a list would make it a lot easier to clean these up and remove some code duplication along the way.

I'd like to do this but I don't actually know what could be cleaned up or deduplicated

Also if we wanted we could pull stuff like UseDelay or speech out of the BaseAction itself as datafields and make it less of a god class.

I'm not super certain on how to pull out the UseDelay and Speech, but I assume it would involve making them separate Action components? like making a ActionUseDelayComponent. I think I can do that.

@slarticodefast
Copy link
Member

@keronshb has plans for a bigger action refactor, not sure how well this fits into it.
If we want to merge this first we best do so quickly because it will create a huge amount of merge conflicts and breaking changes.

@Plykiya
Copy link
Author

Plykiya commented Feb 4, 2025

@keronshb has plans for a bigger action refactor, not sure how well this fits into it. If we want to merge this first we best do so quickly because it will create a huge amount of merge conflicts and breaking changes.

Ah, I see. If that's the case I'll avoid working on actions beyond this PR then. Hope it goes well.

@keronshb
Copy link
Contributor

keronshb commented Feb 4, 2025

I feel that this would cause too many conflicts on some upcoming PRs and stuff that's WIP for us to deal with that workload right now. I was trying to avoid something like this before the Wizard launch (we're close to it).

This also only implements half of what I'm planning on implementing for action events. It's not going to just be a sequence of events to take place back to back, but it would be checking if the first event was cancelled and then moving onto the next one.
It would be a huge step forward to getting doafters working on the action.

We can either close or put on DNM and then reopen/remove the tag sometime after Wizard is merged.

@keronshb keronshb added the S: DO NOT MERGE Status: Open item that should NOT be merged. DNM. Allows test to run unlike draft. label Feb 4, 2025
@Plykiya
Copy link
Author

Plykiya commented Feb 4, 2025

I feel that this would cause too many conflicts on some upcoming PRs and stuff that's WIP for us to deal with that workload right now. I was trying to avoid something like this before the Wizard launch (we're close to it).

This also only implements half of what I'm planning on implementing for action events. It's not going to just be a sequence of events to take place back to back, but it would be checking if the first event was cancelled and then moving onto the next one. It would be a huge step forward to getting doafters working on the action.

We can either close or put on DNM and then reopen/remove the tag sometime after Wizard is merged.

Does the wizard update come with the updates you're planning for action events? or is that going to be done separately?

@keronshb
Copy link
Contributor

keronshb commented Feb 4, 2025

I feel that this would cause too many conflicts on some upcoming PRs and stuff that's WIP for us to deal with that workload right now. I was trying to avoid something like this before the Wizard launch (we're close to it).
This also only implements half of what I'm planning on implementing for action events. It's not going to just be a sequence of events to take place back to back, but it would be checking if the first event was cancelled and then moving onto the next one. It would be a huge step forward to getting doafters working on the action.
We can either close or put on DNM and then reopen/remove the tag sometime after Wizard is merged.

Does the wizard update come with the updates you're planning for action events? or is that going to be done separately?

Done separately.

Right now I've been managing a team of contribs & maints to focus on different Wizard PRs.
I have a similar plan for Actions but I haven't been able to move it outside of the bulletpoint phase yet.

After Wiz I was planning on implementing the changes in bits (Sloth's Charges PR, then cut down similar things from actions and move them into comps) leading up to a full rework. This way it has a lot less disruptions on upcoming PRs and breaks down the review into faster & easier to review bites.

@Plykiya
Copy link
Author

Plykiya commented Feb 4, 2025

I feel that this would cause too many conflicts on some upcoming PRs and stuff that's WIP for us to deal with that workload right now. I was trying to avoid something like this before the Wizard launch (we're close to it).
This also only implements half of what I'm planning on implementing for action events. It's not going to just be a sequence of events to take place back to back, but it would be checking if the first event was cancelled and then moving onto the next one. It would be a huge step forward to getting doafters working on the action.
We can either close or put on DNM and then reopen/remove the tag sometime after Wizard is merged.

Does the wizard update come with the updates you're planning for action events? or is that going to be done separately?

Done separately.

Right now I've been managing a team of contribs & maints to focus on different Wizard PRs. I have a similar plan for Actions but I haven't been able to move it outside of the bulletpoint phase yet.

After Wiz I was planning on implementing the changes in bits (Sloth's Charges PR, then cut down similar things from actions and move them into comps) leading up to a full rework. This way it has a lot less disruptions on upcoming PRs and breaks down the review into faster & easier to review bites.

I see, well. I'll just close the PR and avoid working on things dealing with actions then. Thanks for the information.

@Plykiya Plykiya closed this Feb 4, 2025
@Plykiya Plykiya deleted the action-improvements branch February 4, 2025 22:40
@deltanedas
Copy link
Contributor

plans for a bigger action refactor

gee imagine if that was done already so action components didnt use inheritance... oh wait

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S: DO NOT MERGE Status: Open item that should NOT be merged. DNM. Allows test to run unlike draft. S: Needs Review Status: Requires additional reviews before being fully accepted S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. size/M Denotes a PR that changes 100-999 lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Actions should have a list of events
4 participants