Maybe improve design of application by using sync primitives

[LukasKalbertodt]

I'm a bit unhappy with the current structure: we pass an immutable reference of one module (not Rust module) to all other modules that need to use it. For example, the current main loop looks like:

try!(self.renderer.render(&self.world_view, &self.player.get_camera()));
let event_resp = self.event_manager
    .poll_events(vec![&mut CloseHandler, &mut self.player]);

We pass the world and the camera to the renderer, as well as all event receivers to the event manager. The renderer needs a whole lot more things in the future, like the weather system and the sky system; so we can expect the function signature of render() to increase in argument count. The event manager would also be nicer with methods like add_handler().

The problem here is that we, of course, can't pass immutable references (or in the case of the event manager: mutable ones) around while still mutating the object on our own. A possible solution would be to use an RwLock (taking into account that we will have threads in the future).

But the question is: what is more rusty, more efficient and the better design? The current design works and the borrow checker doesn't complain. The RwLock design also adds some complexity and introduces some overhead for locking. Maybe it's not that bad to have terrible long argument lists. I'm really not sure about this (?). Maybe at the very least we should wrap the World into an RwLock and give it to everyone?

I'm curious what you think, especially @jonas-schievink ...

[jonas-schievink]

If we really want to allow full access to everything from multiple threads, we can just use specs for the game. Using an ECS is overkill, though.

I don't think anything needs to be mutated by multiple threads, so an RwLock won't be necessary. When #57 is resolved, we'll just put a small loop that drains a Receiver and changes the World into the main loop. #58 can also be resolved by simply passing a &mut World.

Giving an add_handler method to the event manager also has the problem of running into borrow checker and lifetime inference problems, which I'd like to avoid for now, so we'd have to pass an Rc<RefCell<...>>. I think the current solution is good enough without requiring any major borrowck gymnastics.

[LukasKalbertodt]

Using an ECS is overkill, though.

I agree.

I don't think anything needs to be mutated by multiple threads, so an RwLock won't be necessary.

Whether or not we need an RwLock or Mutex does not depend on how many threads mutate the world, but how many threads have access to the world, right? If we only have one not-main thread (which doesn't access the world at all), then of course you're right and we can avoid the little overhead of atomic operations.

When #57 is resolved, we'll just put a small loop that drains a Receiver and changes the World into the main loop.

I agree, I also planned on doing exactly that.

#58 can also be resolved by simply passing a &mut World.

To what? I'm not sure I understand what you are suggesting. #58 is also about updating the view.

The world can potentially mutated at multiple spots in our program. One important example is the player's interaction with the world. Currently, when a click event is forwarded to the player via the EventHandler trait. The handle_event() methods receives &mut self, but nothing else. To mutate the world on a click we could

1. add a &mut World parameter to the handle_event() method
2. queue the click event within the player and pass &mut World to Player::update() to apply the event later
3. Save a RefCell<World> (or similar) within the player to allow mutating the world via &mut Player (even &Player ...)

I don't like (1.) because it again mixes different concerns and grows function signatures. I don't like (2.) because this makes our code more complex than it needs to be and everyone thinks Rust is annoying 😛

So again, I think that we should pass at least some inner-mutability-containers around to avoid those problems.

[jonas-schievink]

Well, we wouldn't need a queue of click events, just a bool flag (or a few)
would be enough. But other than that you're right.

[LukasKalbertodt]

This was tackled in #71. While we might want to improve the design in the future, I'll close this issue for now.