-
Notifications
You must be signed in to change notification settings - Fork 29
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
breaking out event_system into multiple event_handler systems #63
base: master
Are you sure you want to change the base?
Conversation
hey @salpalvv, this is cool! I've also been thinking about this problem, I think this is a pretty neat solution, one system definitely gets out of hand. One alternative would be to keep one single event system but break out the logic for handling each event type into separate functions, the downside is that doesn't play very nicely with the system data as you need to keep passing that around. Let me have a think about this and a closer look at the code but seems really promising.
Definitely in the last chapter because otherwise you'd have to backport to all other chapters which will be a pain. |
code/rust-sokoban-c03-05/Cargo.toml
Outdated
@@ -6,6 +6,8 @@ edition = "2018" | |||
|
|||
# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html | |||
|
|||
cargo-features = ["unstable"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we avoid using unstable? trying to keep the tutorial accessible to beginners and I worry that if we start going down the unstable path more unstable code would make its way in. I do agree that drain filter is the nicest most idiomatic API for this, but I would be happy with some clones/etc if it made it easier to explain to new folks. we can always put a little note in the text saying the "best" way to do this is unstable+drain, check it out here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm not sure what the cleanest implementation of it would look like. Like I said, this is just what I have done after not enjoying the large match in my project using the event_queue pattern.
And to be fair, it may be easier to read for some if it just collected filtered vec into a variable and then for loop iterate over that.
mod gameplay_state_system; | ||
mod input_system; | ||
mod rendering_system; | ||
pub mod box_placed_on_spot_event_handler_system; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice! this would be a good opportunity to explain public modules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, when I think about it, it's probably better not to have all the structs with the same names and just scoped behind files. If anyone did this and used ::*
, they would have duplicate names. I can probably just match it all up like you have.
@@ -1,9 +1,10 @@ | |||
mod event_system; | |||
mod gameplay_state_system; | |||
mod input_system; | |||
mod rendering_system; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: add a new line here
…k in the unmatched event enums
Ok, this isn't the prettiest but I got it working in stable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a pretty big change so, I can make any smaller, naming changes after you've taken a look at it and considered it. I left the unstable implementation if you think it's worth mentioning. I know performance isn't a big deal for this project but the extra new_events
vec in every system, push, and append, in a larger system may become a problem, especially if you really lean into the EventQueue pattern. Each system is also iterating over the entire queue so there's a performance hit to decouple it into many. I haven't actually benchmarked them both (drain
implementation vs drain_filter()
) but that might be something to look at later.
hey @salpalvv, been thinking about this, I agree that the performance implications of the dequeue/re-enqueue are not ideal. what if we use our custom implementation of drain_filter? that seems to me like best of both worlds, we are using the "best practice" and most performant solution, we can refer to the unstable implementation in the text and mention we are not using it as it's still unstable. When the drain_filter becomes stable we simply switch to using it. what do you think? I had a go at a custom drain_filter impl, seems to work, but did it in a bit of a rush so it might not be perfect. |
Hey,
I took the event_queue pattern and broke it out into multiple systems to handle each enum variant.
I'm doing this in my current project and it looks something like this.
I did have to use the unstable feature
drain_filter
so that the event_queue would only have the matched enums removed.I didn't know where to put the refactor so i just threw it into the last chapter.
Also, please excuse the poor naming of the systems.
This is more a conversation-starter PR but this pattern works nicely for me.
I didn't like dumping so much logic into a single EventSystem.
There is probably some way to avoid matching twice in each event handler system.