Skip to content

Fixes #18740, added the missing documentation for B parameter on Trigger. #18914

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Fixes #18740, added the missing documentation for B parameter on Trigger. #18914

wants to merge 1 commit into from

Conversation

mhsalem36
Copy link
Contributor

Objective

Solution

  • Rewrote the documentation of Trigger and described how the B parameter in Trigger is used.

Testing

  • No testing needed since it is documentation.

@alice-i-cecile alice-i-cecile added this to the 0.17 milestone Apr 24, 2025
@alice-i-cecile alice-i-cecile added C-Docs An addition or correction to our documentation 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 Apr 24, 2025
MrGVSV
MrGVSV reviewed Apr 24, 2025
View reviewed changes
crates/bevy_ecs/src/observer/mod.rs
Comment on lines +31 to +33
/// See [Trigger::propagate] for more information. It also provides a [Bundle] type representing a set of
/// components associated with the triggered event. This would be useful to access the components or data
/// that are related to the event.
Copy link
Member

Choose a reason for hiding this comment

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

Part of the confusion I was having with the B parameter is that it doesn't seem to filter its contained components by default. Instead, I believe there is some extra step needed when defining the observer such that it can actually be used. I think pointing users to that method(s) and explaining how to use it could be a bit more useful.

crates/bevy_ecs/src/observer/mod.rs
Comment on lines +31 to +33
/// See [Trigger::propagate] for more information. It also provides a [Bundle] type representing a set of
/// components associated with the triggered event. This would be useful to access the components or data
/// that are related to the event.
Copy link
Member

Choose a reason for hiding this comment

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

We should probably also mention how this bundle parameter will filter out entities that don't contain all components in the bundle (it could be the other way around, I forget haha)

crates/bevy_ecs/src/observer/mod.rs
Comment on lines +29 to +30
/// A type that encapsulates information about a triggered [Event] during a specific [Observer] run. It includes
/// the [Event] data, the corresponding [Entity] that triggered it, and details about event propagation.
Copy link
Member

Choose a reason for hiding this comment

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

Two minor nitpicks:

  • The links should probably continue to use the backticks to indicate to the user that these are in fact traits/types
  • Normally we recommend an empty line after the first sentence/summary

@alice-i-cecile alice-i-cecile 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 May 5, 2025
@alice-i-cecile alice-i-cecile moved this to Observer overhaul in Alice's Work Planning May 5, 2025
@alice-i-cecile alice-i-cecile mentioned this pull request Jun 11, 2025
Merged
github-merge-queue bot pushed a commit that referenced this pull request Jun 11, 2025
@alice-i-cecile @ChristopherBiscardi @Jondolf
Thoroughly document the current state of observers (#19590)
12f8f9c 
# Objective

The documentation for observers is not very good. This poses a problem
to users, but *also* causes serious problems for engine devs, as they
attempt to improve assorted issues surrounding observers.

This PR:

- Fixes #14084.
- Fixes #14726.
- Fixes #16538.
- Closes #18914, by attempting to solve the same issue.

To keep this PR at all reviewable, I've opted to simply note the various
limitations (some may call them bugs!) in place, rather than attempting
to fix them. There is a huge amount of cleanup work to be done here: see
https://github.com/orgs/bevyengine/projects/17.

## Solution

- Write good module docs for observers, offering bread crumbs to the
most common methods and techniques and comparing-and-contrasting as
needed.
- Fix any actively misleading documentation.
- Try to explain how the various bits of the (public?!) internals are
related.

---------

Co-authored-by: Chris Biscardi <chris@christopherbiscardi.com>
Co-authored-by: Joona Aalto <jondolf.dev@gmail.com>
@alice-i-cecile alice-i-cecile moved this from Observer overhaul to Observers - Completed in Alice's Work Planning Jun 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MrGVSV MrGVSV MrGVSV left review comments

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Docs An addition or correction to our documentation D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged
Projects
Alice's Work Planning
Status: Observers - Completed
Milestone
0.17
Development

Successfully merging this pull request may close these issues.

Missing documentation for B parameter on Trigger
3 participants
@mhsalem36 @MrGVSV @alice-i-cecile