rfcs icon indicating copy to clipboard operation
rfcs copied to clipboard

rc_event_queue

Open tower120 opened this issue 2 years ago • 13 comments

RENDERED

This is rather true RFC (request for comments), than specification, at least for now.

It is somewhat close to https://github.com/bevyengine/rfcs/pull/17 ... but "implementation" is different.

tower120 avatar Sep 09 '21 20:09 tower120

Thanks for creating this RFC - how events interact with 'asynchronous' systems is a real problem area and I'm glad to have more suggests in this space.

However there is a licensing issue which needs to be resolved before we can proceed - rc_event_queue currently does not have a license available.

Note for the author: Please provide a license for rc_event_queue. In general, I want us to avoid introducing new dependencies not available under MIT/Apache2.0, so I would recommend you use this combination of licenses - this is what bevy itself is licensed under. The easiest guide to doing so is C-permissive from the Rust API guidelines. Please note that bevy chooses not to include a copyright notice line in our MIT license following the lead of the Rust project, although I have no opinion for or against it for rc_event_queue (although if you made me choose I'd err on the side of not including one).

For other readers: I should strongly advise against reading the code of rc_event_queue until this is resolved out of an abundance of caution. This also applies to the docs linked in the RFC, which from my cursory reading (of the RFC) seem central to explaining how the proposed change would work.

DJMcNab avatar Sep 09 '21 21:09 DJMcNab

In general, I want us to avoid introducing new dependencies not available under MIT/Apache2.0, so I would recommend you use this combination of licenses - this is what bevy itself is licensed under.

What's a point of dual licensing? Isn't MIT more permissive than Apache2.0? Initially I was going to license just under MIT.

Please note that bevy chooses not to include a copyright notice line in our MIT license...

What is that mean?

tower120 avatar Sep 10 '21 05:09 tower120

What's a point of dual licensing? Isn't MIT more permissive than Apache2.0? Initially I was going to license just under MIT.

Apache-2.0 has extra rules regarding patents. IANAL, but I think it roughly prevents contributors from contributing code to which a patent they own applies without giving a patent grant to all users of the code. It is incompatible with GPL-2.0 according to some lawyers though, hence the dual license with MIT which is clearly compatible.

bjorn3 avatar Sep 10 '21 07:09 bjorn3

License added. If everything goes well - it should become cargo crate. So it can be just another dependency for project, like every else.

tower120 avatar Sep 10 '21 09:09 tower120

By the way, some cargo crates with MIT/APACHE2 have dependencies on MIT only crates (including bevy and rc_event_queue)... Is this ok?

tower120 avatar Sep 10 '21 09:09 tower120

https://law.stackexchange.com/questions/6081/can-i-bundle-mit-licensed-components-in-a-apache-2-0-licensed-project

tl;dr: yes

bjorn3 avatar Sep 10 '21 12:09 bjorn3

@alice-i-cecile You said something about that rc_event_queue could be useful for something UI specific. Did you mean Entity Events?

Cuurently, EventQueue must be Pinned, because Readers need to talk back when passing inter-chunk boundary, so its stored in heap, to have stable address.

But I think I can make it non-pinned, you'll just have to provide Reader an &EventQueue on each read session. Also, I think, it is possible to built-in one-two small chunks into EventQueue, thus having some form of small_vec.... Chunks can also grow log2 in size, like in VecDeuqe, instead of being fixed.

tower120 avatar Sep 10 '21 19:09 tower120

@alice-i-cecile You said something about that rc_event_queue could be useful for something UI specific. Did you mean Entity Events?

Ah right! Yes, per-entity events were particularly blocked on solving the questions around event lifespan, as I found they tended to have more unusual access patterns. And then in turn, I felt that storing events on entities might be quite useful for handling UI event dispatching.

Cuurently, EventQueue must be Pinned, because Readers need to talk back when passing inter-chunk boundary, so its stored in heap, to have stable address.

Interesting, that makes sense. FWIW, NonSend components should be quite feasible; we've just never had a pressing need for them before. The overhead of just doing that may be better than your non-pinned variant, but I'll leave that judgement call to you.

alice-i-cecile avatar Sep 10 '21 19:09 alice-i-cecile

Main overhead with Arc is construction time, everything else is mostly non-observable, at least for current Bevy Events use-case.

But if you really plan to put EventQueue into Component and add/remove components dynamically, that overhead is a thing. Considering small buffer optimization implementation, memory-wise it could be approx. 64byte + chunk size, non-heap (until fits small chunk), and you can move it around. As a price for that, you'll need to pass &EventQueue to get Reader::iter().

So I would say, realistically it would be 128-256 bytes on stack. Could that work for your cause?

tower120 avatar Sep 10 '21 20:09 tower120

I think I bring rc_event_queue to more-or-less mature state. I thought about "experimental integration" on top of current bevy's API.... But, it does not fit 1-to-1... For example Events::get_reader_current, iter_current_update_events, does not have sense with guaranteed delivery system. And Events::drain is not implementable with rc_event_queue (and I don't see the urgent need in it).

So .... I thought, what about having some new EventsV2 alongside with the current one? This way - new events will not be a braking change.... Is that acceptable?

tower120 avatar Oct 08 '21 18:10 tower120

So .... I thought, what about having some new EventsV2 alongside with the current one? This way - new events will not be a braking change.... Is that acceptable?

I would be hesitant to do that; this should work fine as an external crate if we want early usage feedback.

Keeping both in the engine leads to the sort of caveats and confusion you tend to see in e.g. Unity, where you have two very similar but subtly different tools for the job, whose usage is mixed throughout the code base.

alice-i-cecile avatar Oct 08 '21 20:10 alice-i-cecile

FYI @tower120, we're mostly just blocked on "decision-making attention": there are a large number of other pressing ECS needs that have been getting more attention right now (https://github.com/bevyengine/bevy/discussions/2801) and Cart is focusing on rendering to get 0.6 released <3

alice-i-cecile avatar Oct 08 '21 20:10 alice-i-cecile

I see... And API changes, like making EventReader::iter return Iterator, instead of DoubleEndedIterator, is out of the question. Right?

tower120 avatar Oct 08 '21 21:10 tower120