iolivia / rust-sokoban

Rust Sokoban book and code samples
https://sokoban.iolivia.me
MIT License
155 stars 29 forks source link

breaking out event_system into multiple event_handler systems #63

Open salpalvv opened 4 years ago

salpalvv commented 4 years ago

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.

iolivia commented 4 years ago

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.

I didn't know where to put the refactor so i just threw it into the last chapter.

Definitely in the last chapter because otherwise you'd have to backport to all other chapters which will be a pain.

salpalvv commented 4 years ago

Ok, this isn't the prettiest but I got it working in stable. I just took your idea of appending the new vec while iterating through the drain(..), if they don't match the Event, they get tossed back into the event_queue. Not as pretty as drain_filter but I also left in the unstable implementation underneath for reference.

iolivia commented 4 years ago

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. gist: https://gist.github.com/rust-play/5176f1d24bfac73688995f051de4f721 playground: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=59760662550df7c528f63b6fd180290c