-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Generalized and atomic staging utils #17701
base: main
Are you sure you want to change the base?
Conversation
both cold and staged are behind locks, so this is redundant
This offers more than just atomic staging options now.
The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we put this in bevy_platform_support? I think it's likely more fitting there, and I'm nervous about adding anything new to bevy_utils, since we're trying to eliminate it.
Ah. I wasn't aware of that. I can absolutely move it. |
I'd personally prefer if it stayed in |
Hopefully this also fixes no_std
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should fix the alloc
issue you were having before and also allow using most of the functionality here even without alloc
enabled.
Mostly optimizing docs for better diff views in the future. Great idea. Co-authored-by: Zachary Harrold <zac@harrold.com.au>
This opens up the possibility that invalid data is created. For example, if a thread crashes while actively staging a change, there might be an incomplete staged change. However, in the context that this will be used, that seems unlikely, and this behavior gives more flexibility, etc.
I'll defer to @alice-i-cecile as to judging whether this is properly motivated, since I don't feel I have the full context here. Some thoughts, having skimmed but not really reviewed:
|
Agreed. This is definitely tricky to determine. The end goal is to enable
Totally agree here too. The simplicity of RwLock is appealing. I haven't bothered to test it yet because when this idea was first discussed on discord Cart was pretty sure we should avoid solutions where registering a new component would block old components from being read.
I wish
That's my thoughts at least. Believe me, I wouldn't be making this PR if I saw an easier way, but I don't. But if anyone else wants to save the day here, I'm all ears. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor fixes for compile-check-no-std
Definitely worth documenting this thought process either in the PR description or even the documentation on the module itself. Knowing exactly what niche this is trying to fill helps justify its existence, and will inform future maintainers about why certain tradeoffs have been made. |
There's a few things I'm trying to clean up at the moment, and then I'll go hands off for a bit. Making the example showed me some areas to improve. |
Great, thanks for the thorough response. Seems like we should def proceed with this then. |
@bushrat011899 I'm satisfied with where it's at at the moment. You had a lot of great suggestions in the last round. Sorry it took so long to do them. Looking forward to more! |
Objective
The goal here is to provide a general alternative to a
RwLock
for rarely changed collections. When a change is made, it stages it in temporary storage without actually modifying the data. Then, at user defined points, the changes are applied to the data, draining the staged changes.This has come up a lot regarding components, ex: #17569, but the same treatment has been discussed for reflection and other areas where a type has to be registered once and only once, on demand. This allows that functionality to be put on separate threads, which could unblock a lot of additional ideas, like assets as entities and read-only queries.
Further, it may be useful to put these "StageOnWrite" data structures in an Arc, so that has been implemented too. This could unlock benefits like not having to translate component ids when spawning scenes or cloning entities, but more on that in future PRs and discussions.
There's a lot of things this PR benefits but it should be evaluated as a stand-alone addition for these utilities. Still, those goals and use cases did influence the design, so keep that in mind.
Solution
StagedChanges
trait that tracks changes to a "target" data structure,Cold
.StageOnWrite
andAtomicStageOnWrite
.Testing
I did create a test module with an example, but the potential for bugs here is mostly in systems using this feature, so I kept it kind of minimal for now.