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

Update docs of set_if_neq and replace_if_neq #12919

Merged
merged 4 commits into from Apr 15, 2024

Conversation

RobWalt
Copy link
Contributor

@RobWalt RobWalt commented Apr 10, 2024

Objective

  • This PR adds more flexible versions of set_if_neq and replace_if_neq to only compare and update certain fields of a components which is not just a newtype
  • Update docs of set_if_neq and replace_if_neq #12919 (comment) gave a good solution to the original problem, so let's update the docs so that this is easier to find

Solution

  • Add set_if_neq_with and replace_if_neq_with which take an accessor closure to access the relevant field

In a recent project, a scenario emerged that required careful consideration regarding change detection without compromising performance. The context involves a component that maintains a collection of Vec<Vec2> representing a horizontal surface, alongside a height field. When the height is updated, there are a few approaches to consider:

  1. Clone the collection of points to utilize the existing set_if_neq method.
  2. Inline and adjust the set_if_neq code specifically for this scenario.
  3. (Consider splitting the component into more granular components.)

It's worth noting that the third option might be the most suitable in most cases.

A similar situation arises with the Bevy internal Transform component, which includes fields for translation, rotation, and scale. These fields are relatively small (Vec3 or Quat with 3 or 4 f32 values), but the creation of a single pointer (usize) might be more efficient than copying the data of the other fields. This is speculative, and insights from others could be valuable.

Questions remain:

  • Is it feasible to develop a more flexible API, and what might that entail?
  • Is there general interest in this change?

There's no hard feelings if this idea or the PR is ultimately rejected. I just wanted to put this idea out there and hope that this might be beneficial to others and that feedback could be valuable before abandoning the idea.

In one of my personal projects I stumbled across a case where I wanted
to be cautious with the change detection while also not sacraficing any
performance.

My use case: I basically have a horizontal surface which is saved as a
collection of `Vec<Vec2>` in a component. The component also includes a
height field. Now when the height is updated there only have two (three)
options:

- The collection of points can be cloned so the existing `set_if_neq` is
  usable
- The `set_if_neq` code can be inlined and adjusted for this special case
- (The component can be split up into two more granular components)

I have to acknowledge that the third point here is probably the way to
go in most cases, but I was curious:

- if it would be possible to have a more flexible API and how that would
  look like
- if you are interested in this change in general

No hard feelings if we just end up rejecting this PR. I'm also totally
fine with this. But maybe it's useful to anyone and I just wanted to
hear your feedback first before tossing this back into the void and
forgetting about it.
@SkiFire13
Copy link
Contributor

SkiFire13 commented Apr 10, 2024

When the height is updated, there are a few approaches to consider:

Note that there's another alternative which is also very similar to the approach proposed in the PR. You can use map_unchanged to perform the mapping step and then call set_if_neq on the mapped Mut.

In fact I'm pretty sure (haven't actually tried though) that you can implement set_if_neq_with as self.map_unchanged(accessor_f).set_if_neq(value)

@SolarLiner SolarLiner added C-Enhancement A new feature A-ECS Entities, components, systems, and events labels Apr 10, 2024
@RobWalt
Copy link
Contributor Author

RobWalt commented Apr 11, 2024

Thanks for the hint, I didn't know that! Then maybe that should be the way to go and there's no real need for set_if_neq_with. It might make sense to update the docs to point other people to this solution here, though. I'll do that once I find the time

@RobWalt RobWalt changed the title Add a more flexible version of set_if_neq Update docs of set_if_neq and replace_if_neq Apr 12, 2024
@alice-i-cecile
Copy link
Member

@SkiFire13 can I get your review here?

crates/bevy_ecs/src/change_detection.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/change_detection.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/change_detection.rs Outdated Show resolved Hide resolved
@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Apr 15, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 15, 2024
@alice-i-cecile
Copy link
Member

Thank you both: this is a helpful note.

Merged via the queue into bevyengine:main with commit e7ab656 Apr 15, 2024
28 checks passed
@RobWalt RobWalt deleted the feat/set_if_neq_with branch April 15, 2024 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Enhancement A new feature S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants