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

Write only world query #71

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

nicopap
Copy link
Contributor

@nicopap nicopap commented Jun 25, 2023

WriteOnly world query

Summary

Add a WriteOnly<T> WorldQuery. WriteOnly acts like &mut, but tells bevy
that the value of component T is overwritten unconditionally by the system.
Add the B0006 error. This tells users that they are modifying a component
that is later overwritten unconditionally.

RENDERED

@alice-i-cecile
Copy link
Member

Hmm. I'd really like to not use Transform for UI, solving one of these cases at least.


```rust
app.add_plugins(DefaultPlugins::new())
.add_systems(PostUpdate, shake_ui.after(ui_layout_system));
Copy link
Member

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

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

Copy link
Contributor Author

@nicopap nicopap Jun 27, 2023

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.

@nicopap
Copy link
Contributor Author

nicopap commented Jun 27, 2023

Hmm. I'd really like to not use Transform for UI, solving one of these cases at least.

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 Transform, or UiPosition, or whatever it is named, telling the user they are doing it wrong can still be valuable.

Also, enabling back writes to GlobalTransform and Visibility is still useful.

An alternative is to define an additional component (say: UiOffset) for and handle yourself the offsetting based on the value of that additional component. It would look like this

But maybe UiOffset only exists because bevy is incapable of telling users what went wrong with system ordering. And adding an additional layer of abstraction is not worth it if bevy has this capability.

@nicopap
Copy link
Contributor Author

nicopap commented Jun 27, 2023

Note that work on this was inspired by cuicui_fab.

cuicui_fab is fairly complex and I could write a small book about it. But write-only is extremely useful in it. And I've spent a couple weeks trying to migrate it to use bevy systems, and this is what spurred this proposal. Not that my work on cuicui_fab is dependent on this RFC. Quite the opposite in fact.

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.

3 participants