Relationship::on_replace docs are confusing

[SteveAlexander]

I'm confused by the docs for on_replace.

https://docs.rs/bevy/latest/bevy/ecs/relationship/trait.Relationship.html#method.on_replace

Register a ComponentHook that will be run when this component is about to be dropped, such as being replaced (with .insert) or removed.

So I asked in Discord, #ecs

Can I expect it to be run whenever a component is removed, regardless of whether it's being replaced?

I came to this because I'm implementing a custom Relationship, and I saw that trait Relationship provides on_insert and on_replace. So is on_remove not needed, as on_replace runs anyway, regardless of whether the component is actually being replaced?

Tim kindly answered

Yes, it is run when the component is removed, and yes the naming here is very confusing
As far as I can tell the actual on_remove hook is useless for any kind of bookeeping

So, I think the docs for this method should be improved so they're less confusing. I still don't understand what it's for / how it works in enough detail to propose better docs.

An alternative would be to choose a better name for the method. Perhaps on_removing would be clearer, as it expresses "yes it's being removed" and "the removal is kind of in-progress, so you can still get at stuff"

[gwafotapa]

I would say the documentation of the method is good but it is poorly named. How can removing something for good count as replacing it ? on_remove would be a much better name (since replacing something implies you have to remove it first) but is already taken and I don't know what I would rename the actual one with. The best I can come up right now is on_replace_or_remove.

By the way, I think the confusion is exacerbated by other parts of this documentation page. For instance, the documentation of on_insert says

Register a ComponentHook that will be run when this component is added (with .insert) or replaced.

That led me to believe at first that on_insert would not run if a component was directly inserted with Commands::spawn

What cleared things up for me are the comments from the example of World::register_component_hooks which are great.

[SteveAlexander]

@gwafotapa what do you think about on_removing as a name for this hook? The important thing about on_replace is not so much that it represents replacing a component (although that might be happening) but that the hook is called before the removal actually occurs. So, on_removing while removal / replacement is in progress, and on_remove when the component has been removed.

Another renaming thought -- maybe on_remove should be renamed on_removed as it is called when the component has been removed. The past tense "removed" helps make this clear.

[gwafotapa]

maybe on_remove should be renamed on_removed as it is called when the component has been removed. The past tense "removed" helps make this clear.

That is not correct. on_remove is also called before removing the component.

I don't feel that on_removing carries the meaning it happens before removal. If you want to go this way I would prefer something like before_removal. But I don't really mind the name not capturing the timing. It makes sense when you think about it and people may even naturally expect it. I'm more concerned with the name describing what it actually does.

[SteveAlexander]

@gwafotapa Thanks, I see that you're right. In case anyone reading is less familiar with this code, and is curious where to look...

bevy_ecs/src/bundle/remove.rs in BundleRemover::remove the very first thing that happens is

• replace observers
• replace triggers
• remove observers
• remove triggers
  get called, in that order.

Similarly for bevy_ecs/src/world/entity_refs.rs in EntityWorldMut::despawn_with_caller, except that additionally handles despawn events/triggers first.

For on_replace, it's also run in bevy_ecs/src/bundle/insert.rs in BundleInserter::insert.

It seems to me that these are the place where the behaviour of on_replace gets implemented, and also how it is different from on_remove. I'll study this code a bit more, and then see if there are changes to the docs that would make things clearer for me.

Also, @gwafotapa recommended reading the code in examples/ecs/component_hooks.rs around line 66. Linking here to make it easy to view.

https://github.com/bevyengine/bevy/blob/main/examples/ecs/component_hooks.rs