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

rc_event_queue #32

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

rc_event_queue #32

wants to merge 4 commits into from

Conversation

tower120
Copy link

@tower120 tower120 commented Sep 9, 2021

RENDERED

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

It is somewhat close to #17 ... but "implementation" is different.

@DJMcNab
Copy link
Member

DJMcNab commented Sep 9, 2021

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.

@tower120
Copy link
Author

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?

@bjorn3
Copy link

bjorn3 commented Sep 10, 2021

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.

@tower120
Copy link
Author

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

@tower120
Copy link
Author

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

@bjorn3
Copy link

bjorn3 commented Sep 10, 2021

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

tl;dr: yes

@tower120
Copy link
Author

tower120 commented Sep 10, 2021

@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.

@alice-i-cecile
Copy link
Member

@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.

@tower120
Copy link
Author

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 tower120 marked this pull request as ready for review October 6, 2021 15:50
@tower120
Copy link
Author

tower120 commented Oct 8, 2021

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?

@alice-i-cecile
Copy link
Member

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
Copy link
Member

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 (bevyengine/bevy#2801) and Cart is focusing on rendering to get 0.6 released <3

@tower120
Copy link
Author

tower120 commented Oct 8, 2021

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

@cart cart removed the enhancement label Feb 24, 2022
@alice-i-cecile alice-i-cecile mentioned this pull request Nov 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants