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

Improve ReaderId #28

Open
torkleyy opened this issue Dec 1, 2018 · 7 comments
Open

Improve ReaderId #28

torkleyy opened this issue Dec 1, 2018 · 7 comments

Comments

@torkleyy
Copy link
Member

torkleyy commented Dec 1, 2018

Right now, the event buffer will grow if a ReaderId is not advanced. This thread is meant to discuss potential solutions to this problem.

@Xaeroxe
Copy link
Contributor

Xaeroxe commented Dec 1, 2018

This was introduced during #15 and is somewhat intentional and inherent to that approach. The only way to handle this would be to accept that not all events will get through. Alternatively, if you know you're not going to be accepting events for a bit you can just drop your ReaderId

@torkleyy
Copy link
Member Author

torkleyy commented Dec 1, 2018

I know it is, yes. I'm just opening this issue since this has come up on Discord and I don't want to discuss it there for the obvious reasons.

@Xaeroxe
Copy link
Contributor

Xaeroxe commented Dec 17, 2018

Here's a possible solution, add a function ReaderId::sleep(&self) which stores in an Arc<AtomicBool> the value true. The EventChannel also keeps a copy of this Arc<AtomicBool> and when determining whether or not to grow it will ignore sleeping ReaderIds. When a sleeping ReaderId is used with EventChannel::read it's "woken up" reset to the current indexes, and no events are returned. Subsequent reads will return events.

@azriel91
Copy link
Member

azriel91 commented May 3, 2019

Hiya, is there an existing discussion somewhere about unregistering readers? I can't seem to find it (though I seem to recall one somewhere), but it's something I'd like to do.

@Xaeroxe
Copy link
Contributor

Xaeroxe commented May 3, 2019

All you got to do is drop it. If you drop the reader id then it's automatically unregistered.

I guess we should put that in documentation

@torkleyy
Copy link
Member Author

torkleyy commented May 3, 2019

Just did that ^^

@azriel91
Copy link
Member

azriel91 commented May 4, 2019

Copied from discord.

I have this use case / usage:

  • InputToControlInputSystem turns the generic InputEvent into the game specific ControlInputEvent
  • CharacterSelectionWidgetInputSystem takes ControlInputEvent, and based on the current widget state, might display the next character, or confirm the character selection
  • Upon character selection confirmation, a CharacterSelectionEvent::Confirm event is fired (different event channel)
  • The CharacterSelectionState listens for that last event, and does a transition

-- @azriel91

Let's say we want to introduce readers which sleep while their systems are not executing. In that case, we need to call some method on the system so it can disable e.g. the reader ids. Now, we don't have such a method. So we'd have to add on_pause to System, RunNow, Dispatcher, etc.
Tomorrow, somebody also wants on_resume - so we add yet another method
and we end up with System resembling State

-- @torkleyy

Been thinking about the issue, and in reference to "Tomorrow, somebody also wants on_resume", I became that guy for a bit. Thinking a bit more:

  • Systems should be simple (perform logic on data).
  • Events are a form of data communication between systems.

In the use case, the control input events should apply to the "active" systems. The EventChannel<ControlInputEvent> is long lived, and I think it should be -- it's a concept that applies to any active thing in the application's lifetime.

The thing that changes is whether a State is active, so the responsibility lies with the state to (somehow) disable / unregister the ReaderId. This may be one of:

  • Drop and recreate the dispatcher.
  • Somehow communicate with the System within the dispatcher to drop and get a new ReaderId.
    • Bad / hacky: Signal through a channel

    • Just call dispatcher.setup(&mut world.res) again? Still figuring this one out.

      This doesn't "just work" -- For States that are pushed, systems that live in a state specific dispatcher are still alive, so their ReaderId will cause the ringbuffer to grow.

      Introducing a teardown() as a complement to setup() is already creeping towards the next option.

    • Not great: impl on_pause / on_resume traits on Dispatcher and Systems

  • Something else I haven't thought of.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants