-
Notifications
You must be signed in to change notification settings - Fork 66
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
Write only world query #71
base: main
Are you sure you want to change the base?
Conversation
Hmm. I'd really like to not use |
|
||
```rust | ||
app.add_plugins(DefaultPlugins::new()) | ||
.add_systems(PostUpdate, shake_ui.after(ui_layout_system)); |
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 isn't actually "fully correct" for this scenario. Making changes to the Transform
after ui_layout_system
does ensure it won't get stomped on by that system. And for the current Bevy systems written I do think it would work. But the computed node Transform is considered an output of the UiSystem::Layout
set. Anything trying to consume the "final computed layout " of a node should schedule relative to that. Therefore if you are modifying Transform (and affecting final layout), you should put your system in UiSystem::Layout
.
|
||
``` | ||
|
||
`WriteOnly<T>` is very much nothing more than a thin wrapper around `Mut`. |
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.
WriteOnly
seems like the wrong name for this:
- We already have the concept of "read only queries". Which means "you statically do not have permission to do anything but read". This is not the "write" equivalent of that.
- The goal of this feature is not to assert that we only write to a thing (and not read from it). It it is to assert that all values will be written to, with none skipped. Reading before or after the write is completely ok, provided you do the write.
WriteOnly
not only isn't accurate ... it encourages the user to think about it in the wrong way. If I saw WriteOnly in a query, I wouldn't think "i need to write every item in the query". I would think "i cannot read items in this query but I can still skip items I don't want to write".
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.
The no-read semantic is important. What makes ui_layout_system
handling of Transform
different from other systems is indeed that it overwrites the Transform
without leaving a trace of the existing value. Whether it does this to all the entities it access or only a subset is only partially relevant.
Reading the previous value disqualifies for WriteOnly
fn sys(mut q: Query<&mut Transform>) {
for t in &mut q {
t.translation.x += 1.0;
}
}
Here, a previous system that changes Transform
still has a visible effect after sys
runs. So we don't want to say that translation was overwritten.
While for the following:
fn sys(mut q: Query<&mut Transform>) {
for t in &mut q {
if t.translation.x > 10.0 {
continue;
}
*t = Transform::IDENTITY;
}
}
The previous system's value will be overwritten (for a subset of Transform
) If the user wrote a system that sets a Transform
that has a translation.x
smaller than 10.0, that value will never be read afterward. So it has no visible effects. A system that updates a value with no visible effect is most definitively a user error.
Our goal here is an error message similar to rust's unused-assignments
lint.
Now, take our 2nd code sample. It still reads from the Transform
. It reads the t.translation.x
in the conditional. Therefore it also disqualifies for WriteOnly
. Since it depends on the previous value. The only situation where WriteOnly
applies is:
fn sys(mut q: Query<WriteOnly<Transform>>) {
for t in &mut q {
t.set(Transform::IDENTITY);
}
}
This includes arbitrary query filters. But beyond that, if any trace of the existing value is read by sys
, or remains in the Transform
, it is a mistake to mark it as WriteOnly
On exhaustiveness
Now I said "if any trace of the existing value remains in the Transform
".
Well, precisely. It's still possible to leave as-is (without reading them) pre-existing values. However, we run the risk of false positives.
You could read a value on a 2nd component and use it to decide to write or not on the write-only component.
fn sys(mut q: Query<(&ShouldOverwrite, WriteOnly<Transform>)>) {
for (overwrite, t) in &mut q {
if !overwrite.should {
continue;
}
t.set(Transform::IDENTITY);
}
}
It's up to the user to decide if the risk is likely or worthwhile. I don't see any possibility of using WriteOnly
for optimization or anything beyond the error message. So it stays pretty much and mostly aimed at internal engine code and 3rd party plugin code.
A lot of games requires slight updates to UI positions after layouting. A lot of game animate UI elements (say: shift right the currently focused element, or give a very slight shifting effect for a camera movement effect, or even animated text). It's a requirement to be able to manipulate the position after the layouting output. Switching to a different component will still require a layout system to overwrite an existing positional component. This means that whether it's a Also, enabling back writes to An alternative is to define an additional component (say: But maybe |
Note that work on this was inspired by
|
WriteOnly
world querySummary
Add a
WriteOnly<T>
WorldQuery
.WriteOnly
acts like&mut
, but tells bevythat the value of component
T
is overwritten unconditionally by the system.Add the
B0006
error. This tells users that they are modifying a componentthat is later overwritten unconditionally.
RENDERED