Skip to content

Rename methods and fields to get entities associated with observers to reduce confusion #19609

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

alice-i-cecile
Copy link
Member

@alice-i-cecile alice-i-cecile commented Jun 13, 2025

Objective

Our various methods to get entities associated with observers can be confusing, and don't distinguish well between the common and rare cases.

Solution

  1. On::observer -> On::observer_entity
  2. On::target -> On::entity
  3. Pointer::target -> Pointer::original_target
  4. ObserverTrigger::target -> ObserverTrigger::current_target

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 13, 2025
@alice-i-cecile alice-i-cecile requested review from Jondolf and Vrixyz June 13, 2025 02:04
@alice-i-cecile alice-i-cecile added this to the 0.17 milestone Jun 13, 2025
self.trigger.target
///
/// Note that if event bubbling is enabled for your event type, this may not be the entity that the event was originally targeted at.
/// Instead, this is the entity that the event is *currently* targeted at, which may have changed due to propagation.
Copy link
Member

Choose a reason for hiding this comment

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

💯 helpful

///
/// Note that if event bubbling is enabled for your event type, this may not be the entity that the event was originally targeted at.
/// Instead, this is the entity that the event is *currently* targeted at, which may have changed due to propagation.
pub fn entity(&self) -> Option<Entity> {
Copy link
Contributor

@Jondolf Jondolf Jun 13, 2025

Choose a reason for hiding this comment

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

I know the intent is to revisit #18710 after this PR, but these properties are so tightly related that I think we kinda need to consider their naming together. If current_target is entity, would target be something like original_entity?

I see that this was bikeshed in #19263, but personally, I'm also not a huge fan of the name entity.

  • It doesn't indicate what entity it is. In this case, there would be three options: the original target, the entity the event was propagated to, and the observer entity. Judging purely based on the entity name, without looking at docs or the other methods, which one is it? To me, this is not self-evident.
  • I would personally expect the "shorter" name, whether entity or target, to be the entity the event was originally targeting, not the entity the event was propagated to. If I trigger an event on A, and it is propagated to B, I would expect that the target was still A, and the event was just forwarded to be read by another observer. I was surprised to see that this is not the case.
  • Using the short name for the current target is opposite to web conventions where target is the original target and currentTarget is the element the event was bubbled to. I'm aware we don't care too much about following web conventions, but just noting that there is a disconnect, and I personally find the web's semantics to be clearer and more generally useful here.

So my personal preference would be to use target and current_target like proposed in #18710. Or current_target could also be something like observed or observer_target, but I'm not sure if that's clearer.

Aside 1: The lack of an easy way to get the original target is the reason I don't support bubbling collision events to rigid bodies in Avian yet. It's important to know which collider the event originally came from, and that cannot be determined with the current observer API.

Aside 2: It feels a bit weird to use a different naming here than in ObserverTrigger, feels like they should be named the same.

@notmd
Copy link
Contributor

notmd commented Jun 13, 2025

I think Pointer should not store target/currentTarget, thats the job of the Trigger since many other events can also be bubbled. About naming, I also prefer target and currentTarget just like in the web. Also consider removing deref to the event field, I found this quite confusing in practice.

@alice-i-cecile alice-i-cecile added X-Controversial There is active debate or serious implications around merging this PR S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 13, 2025
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-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Observer trigger fields rework Target::target and Target::target() return different entities
4 participants