Event refactor #266
Event refactor #266
Conversation
|
Oh hell yeah, thanks for this!! |
No problem, Let me know if you see any issue. I ran |
Person-93
left a comment
Person-93
left a comment
There was a problem hiding this comment.
FYI - I'm not an official reviewer or maintainer or anything, so you don't have to respond to my comments if you don't want to.
| fn execute_commands( | ||
| event: On<ExecuteCommand>, | ||
| dialogue_runners: Query<&DialogueRunner>, | ||
| mut commands: Commands, | ||
| ) { | ||
| let dialogue_runner = dialogue_runners.get(event.entity).unwrap(); | ||
| let Some(mut command) = clone_command(dialogue_runner, &event) else { | ||
| return; | ||
| }; | ||
| let params = event.command.parameters.clone(); | ||
| let entity = event.entity; | ||
| commands.queue(move |world: &mut World| { | ||
| let task_finished_indicator = command.call(params.clone(), world); | ||
| if !task_finished_indicator.is_finished() { | ||
| get_dialogue_runner_mut(world, event.source).add_command_task(task_finished_indicator); | ||
| get_dialogue_runner_mut(world, entity).add_command_task(task_finished_indicator); | ||
| } |
There was a problem hiding this comment.
I think it might be simpler to just clone the ExecuteCommand, move that into the closure, and de-structure it. Then the dialogue_runners: Query<&DialogueRunner> parameter isn't even needed any more.
There was a problem hiding this comment.
yeah, that's certainly cleaner, I for sure do not need that query.
| pub struct AssertionPlugin; | ||
| impl Plugin for AssertionPlugin { | ||
| fn build(&self, app: &mut App) { | ||
| app.insert_resource(TriggeredEvents::<PresentLine>(Vec::new())) |
There was a problem hiding this comment.
How come we accumulate these at all still? I think just reacting to them as they come should be more idiomatic :)
There was a problem hiding this comment.
I made it so the predicate gets checked as the event is caught.
There is still a resource tracking the number of calls mostly for the case when we want to verify that an event wasn't called.
Only one test really cared about the accumulation serves_assets_after_loading so that one had to do things a bit differently.
janhohenheim
left a comment
janhohenheim
left a comment
There was a problem hiding this comment.
Lovely, thanks! Sorry for the delay haha
|
Merging when the lints are green :) |
Head branch was pushed to by a user without write access
Oops, forgot to run clippy |
Head branch was pushed to by a user without write access
|
Ahh! Sorry about all the churn here. I think I removed the logic to actually call the predicate to verify something, then forgot I did that. So all the test were passing without ever testing :) |
|
happens! :) Thanks a bunch again! |
3 participants
Trying to address #255 and #264
MessageswithEntityEvents.sourcetoentityin those events to be more consistent withEntityEventsdefaultsEntityEventsI am not 100% confident that my changes to
execution.rsare the 'correct' ones but it seems to work.Same thing with the changes I made to the tests.
This is a quite a breaking change anyone who was reading the messages would need to change to use observers.