-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
make update systems concise on standard_widgets example #22332
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
Conversation
40b0d24 to
a2bc6a5
Compare
alice-i-cecile
left a comment
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.
Not a huge fan for an example. It is shorter, but I think it's harder to read and understand.
I agree. I think this kinda illustrates the unknowns and general difficulties in examples especially in such a large crate.
All in all it feels like examples kinda refers to a lot of different things at once and everyone has some idea of what an example should be without a consensus of what an example is. Sorry for the rambling on a random PR 🙃 |
|
I think the existing example serves as an excellent illustration of why RemovedComponents is painful: not only can you not use regular change detection to detect removed components, you can't even use RemovedComponents in combination with a query that also uses other change detection (like added or changed) - instead, you have to use two independent queries. While it's possible to paper over these differences, the cost is increasing abstraction that makes the code harder to understand. The right solution, of course, is to make component removal work like a regular query filter. This is difficult, however: it's hard for Bevy to filter on a component that no longer exists, which means that you have to keep around some auxiliary data, a journal of removals (which is what RemovedComponents basically does). Building this journal isn't free, and it needs to be kept around long enough to be polled by systems that want to know about removals. With regular change detection, we can poll the last change tick of a component at any point in the future, even across frame boundaries, but obviously we can't keep the journal of removed items indefinitely. RemovedComponents keeps the costs of building the journal low by building it sequentially; however, this means that it has to be read sequentially as well, making it unsuitable as a query filter. To solve this, we would need a fast random-access set membership test. For example, if the removed components journal was a hash set, this would work, but would make building the journal more expensive (a cost which gets paid even if the journal is never read). Bloom filters might be one way to have both fast building and fast querying of the journal, but this would make performance highly non-deterministic, which I don't think Bevy users would appreciate very much. |
|
Thank you for sharing. Agree, a use case should provide different method paths for users to learn the system, how to optimize code should be perceived and practiced by users themselves.
// BAD
let chained = updated
.iter()
.chain(removed_depressed.read())
.chain(removed_disabled.read());// GOOD
// should check `removed_depressed` and `removed_disabled` first
if !removed_depressed.is_empty() {
} else if !removed_disabled.is_empty() {
} else {}Sorry to waste your vacation. |
|
I solve it now in two ways: let removed_depressed_is_empty = removed_depressed.is_empty();
let mut removed_depressed_iter = removed_depressed.read();
let removed_disabled_is_empty = removed_disabled.is_empty();
let mut removed_disabled_iter = removed_disabled.read();
let changed_is_empty = changed_buttons.is_empty();
let mut changed_iter = changed_buttons.iter();
let iter = std::iter::from_fn(move || {
if !removed_depressed_is_empty {
return removed_depressed_iter.next();
}
if !removed_disabled_is_empty {
return removed_disabled_iter.next();
}
if !changed_is_empty {
return changed_iter.next();
}
None
}); observe(|_: On<Remove, Pressed>| {
}), |
Objective
Solution
updated,removed_checked/removed_depressed,removed_disabled.