Skip to content

Run observers before hooks for on_replace and on_remove#16499

Merged
alice-i-cecile merged 2 commits intobevyengine:mainfrom
nakedible:hook-observer-remove-order
Dec 6, 2024
Merged

Run observers before hooks for on_replace and on_remove#16499
alice-i-cecile merged 2 commits intobevyengine:mainfrom
nakedible:hook-observer-remove-order

Conversation

@nakedible
Copy link
Copy Markdown
Contributor

@nakedible nakedible commented Nov 25, 2024

Objective

Solution

  • Trivially swaps ordering of hooks and observers for all call sites where they are triggered for on_replace or on_remove

Testing

  • Just CI

Migration Guide

The order of hooks and observers for on_replace and on_remove has been swapped. Observers are now run before hooks. This is a more natural ordering where the removal ordering is inverted compared to the insertion ordering.

bushrat011899
bushrat011899 approved these changes Nov 25, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@bushrat011899 bushrat011899 left a comment

Choose a reason for hiding this comment

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

This makes sense to me conceptually. Nice work!

@nakedible nakedible marked this pull request as ready for review November 25, 2024 00:59
@nakedible
Copy link
Copy Markdown
Contributor Author

nakedible commented Nov 25, 2024

The fix pull runs now hooks after observers, but observers are run in the same order as on insertion, not in reverse order. Flecs says observer ordering is undefined, and I also think that having undefined ordering is better than saying observers are called in insertion order (or reverse insertion order). So I believe it's fine that the observers are not run in reverse order by this pull.

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Nov 25, 2024
@nakedible
Copy link
Copy Markdown
Contributor Author

nakedible commented Nov 25, 2024

If guaranteed observer ordering is added (in a separate pull), then that pull should also add functionality to call the observers in reverse order, and this reverse order should be used for OnReplace and OnRemove observers.

@nakedible nakedible mentioned this pull request Nov 25, 2024
Merged
4 tasks
@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Nov 25, 2024
@nakedible
Copy link
Copy Markdown
Contributor Author

nakedible commented Dec 4, 2024

@alice-i-cecile Two approving community reviews. Not certain of the process, but -S-Needs-Review +S-Ready-For-Final-Review?

@alice-i-cecile alice-i-cecile added 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-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Dec 5, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Dec 5, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 5, 2024
@nakedible nakedible force-pushed the hook-observer-remove-order branch from 16dc842 to 72fc32b Compare December 5, 2024 23:50
@nakedible
Copy link
Copy Markdown
Contributor Author

nakedible commented Dec 5, 2024

The reason this failed the merge queue is exactly because my other pull added checks for the order of hooks vs. observers :-) the only fix needed is to fix the testcase, which is exactly correct.

@nakedible
Copy link
Copy Markdown
Contributor Author

nakedible commented Dec 5, 2024 

assertion `left == right` failed
  left: ["OrdA hook on_add", "OrdA observer on_add", "OrdA hook on_insert", "OrdA observer on_insert", "OrdB hook on_add", "OrdB observer on_add", "OrdB hook on_insert", "OrdB observer on_insert", "OrdB command on_add", "OrdA observer on_replace", "OrdB observer on_replace", "OrdA hook on_replace", "OrdB hook on_replace", "OrdA observer on_remove", "OrdB observer on_remove", "OrdA hook on_remove", "OrdB hook on_remove"]
 right: ["OrdA hook on_add", "OrdA observer on_add", "OrdA hook on_insert", "OrdA observer on_insert", "OrdB hook on_add", "OrdB observer on_add", "OrdB hook on_insert", "OrdB observer on_insert", "OrdB command on_add", "OrdA hook on_replace", "OrdB hook on_replace", "OrdA observer on_replace", "OrdB observer on_replace", "OrdA hook on_remove", "OrdB hook on_remove", "OrdA observer on_remove", "OrdB observer on_remove"]

@nakedible
Copy link
Copy Markdown
Contributor Author

nakedible commented Dec 6, 2024

@alice-i-cecile Mind bumping this back into the merge queue?

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Dec 6, 2024
Merged via the queue into bevyengine:main with commit 912da04 Dec 6, 2024
@nakedible nakedible deleted the hook-observer-remove-order branch December 6, 2024 00:45
ecoskey pushed a commit to ecoskey/bevy that referenced this pull request Jan 6, 2025
@nakedible @ecoskey
Run observers before hooks for on_replace and on_remove (bevyengi…
715ab26 
…ne#16499)

# Objective

- Fixes bevyengine#16498 

## Solution

- Trivially swaps ordering of hooks and observers for all call sites
where they are triggered for `on_replace` or `on_remove`

## Testing

- Just CI

---

## Migration Guide

The order of hooks and observers for `on_replace` and `on_remove` has
been swapped. Observers are now run before hooks. This is a more natural
ordering where the removal ordering is inverted compared to the
insertion ordering.
@AlephCubed AlephCubed mentioned this pull request Jun 11, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@bushrat011899 bushrat011899 bushrat011899 approved these changes

@BenjaminBrienen BenjaminBrienen BenjaminBrienen approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide 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

Milestone

0.16

Development

Successfully merging this pull request may close these issues.

Hook and observer ordering for on_remove/on_replace should be inverted

4 participants

@nakedible @bushrat011899 @BenjaminBrienen @alice-i-cecile