Skip to content

Fix observers not removing themselves from ObservedBy.#24802

Open
andriyDev wants to merge 3 commits into
bevyengine:mainfrom
andriyDev:observers-despawn
Open

Fix observers not removing themselves from ObservedBy.#24802
andriyDev wants to merge 3 commits into
bevyengine:mainfrom
andriyDev:observers-despawn

Conversation

@andriyDev

Copy link
Copy Markdown
Contributor

Objective

Solution

  • When unregistering an observer, also go through the observer's targets and remove itself from those ObservedBy components.

Testing

  • Added two new tests.

@andriyDev andriyDev added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 29, 2026
@andriyDev andriyDev added this to the 0.19.1 milestone Jun 29, 2026
@github-project-automation github-project-automation Bot moved this to Needs SME Triage in ECS Jun 29, 2026
@andriyDev andriyDev force-pushed the observers-despawn branch from eb343f4 to c75bacf Compare June 29, 2026 06:53
Comment thread crates/bevy_ecs/src/observer/mod.rs Outdated
.spawn(Observer::new(|_: On<Hi>| {}).with_entities([target, target]))
.id();

// This is likely not desirable, but preventing this is not worth it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doc nit: this comment confused me ("preventing" felt wrong and your use of "this" twice wasn't clear and I had to re-read the comment multiple times).

Should you just update the // Observe the same entity multiple times. comment to say:

// Observe the same entity multiple times. It's likely not desirable to do this, but disallowing it is not worth it, so lets check this case too.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that the wording needs to change

I interpreted this differently as “It’s likely not desirable that ObservedBy contains the same observer entity multiple times"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I made it more clear!

Comment thread crates/bevy_ecs/src/observer/mod.rs
Comment thread crates/bevy_ecs/src/observer/mod.rs Outdated
@kfc35 kfc35 added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 29, 2026
@kfc35

kfc35 commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

adding waiting-on-author for some comment clarification in the test, but it looks good to me

@andriyDev andriyDev added S-Needs-Review Needs reviewer attention (from anyone!) to move forward S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 29, 2026
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-Bug An unexpected or incorrect behavior D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

Status: Needs SME Triage

Development

Successfully merging this pull request may close these issues.

Despawning an observer does not remove it from ObservedBy relations

4 participants