Skip to content
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

Improve Components Hooks docs #12256

Closed
afonsolage opened this issue Mar 2, 2024 · 1 comment · Fixed by #12295
Closed

Improve Components Hooks docs #12256

afonsolage opened this issue Mar 2, 2024 · 1 comment · Fixed by #12295
Labels
A-ECS Entities, components, systems, and events C-Docs An addition or correction to our documentation

Comments

@afonsolage
Copy link
Contributor

afonsolage commented Mar 2, 2024

How can Bevy's documentation be improved?

As someone who didn't followed closely #10756 I found a little confusing the different between on_add and on_insert when just reading the examples and docs.

Even tho I'm very eager to use component hooks (and we are closer to observers than relations) I think the docs and example could be improved a little bit:

  1. ComponentHooks::on_add should state that spawning an entity will trigger on_add in the same way as on_remove says that despawning an entity counts as removing all of its components.

  2. ComponentHooks::on_insert should state that mutating a component will not trigger on_insert like replacing the component will. I think this is important because in ECS, until now, there is no perceptual difference for the user between mutating a component or just replacing it. Now there is and users should be aware of that.

/// Will trigger on_insert hook
fn update_health(
    mut evts: EventReader<Damage>, q: Query<&Health, With<Player>>, mut commands: Commands,
) {
    for &Damage(entity, hit) in evts.read() {
        if let Ok(Health(current)) = q.get(entity) {
            let new_health = (current - hit).min(0.0);
            commands.entity(entity).insert(Health(new_health));
        }
    }
}
/// Won't trigger on_insert hook
fn update_health_mut(mut evts: EventReader<Damage>, mut q: Query<&mut Health, With<Player>>) {
    for &Damage(entity, hit) in evts.read() {
        if let Ok(mut current) = q.get_mut(entity) {
            let new_health = (current.0 - hit).min(0.0);
            current.0 = new_health;
        }
    }
}
  1. component_hooks.rs example should avoid using "advanced" terms without an easy explanation for the user, like:
    // In order to register component hooks the component must:
    // - not belong to any created archetypes
    // - not already have a hook of that kind registered

In this case I think we should "translate" what not belogin to any created archtypes means, like "Can't be already inserted to any entity" or something like that.

  1. component_hooks.rs example should (maybe?) tell users that Events should be prefered whenever applicable, since hooks can be more performance intensive or at least, more limited (I'm not sure about this one, tho)

If that make sense, I can make a PR for that.

@afonsolage afonsolage added C-Docs An addition or correction to our documentation S-Needs-Triage This issue needs to be labelled labels Mar 2, 2024
@TrialDragon TrialDragon added A-ECS Entities, components, systems, and events and removed S-Needs-Triage This issue needs to be labelled labels Mar 2, 2024
@james-j-obrien
Copy link
Contributor

I agree with all these as improvements.

In regards to Events, they should be preferred in scenarios where they are suitable (whenever you can rely on a sync point to react). Hooks are in some ways more limited since you can only have one per component type and the ergonomics aren't as good as a regular system accessing events, hopefully those limitations will be addressed with observers.

github-merge-queue bot pushed a commit that referenced this issue Mar 4, 2024
# Objective

- Closes #12256 

## Solution

- Improved component hooks docs.
- Improved component hooks example docs.

---------

Co-authored-by: James Liu <[email protected]>
Co-authored-by: Alice Cecile <[email protected]>
spectria-limina pushed a commit to spectria-limina/bevy that referenced this issue Mar 9, 2024
# Objective

- Closes bevyengine#12256 

## Solution

- Improved component hooks docs.
- Improved component hooks example docs.

---------

Co-authored-by: James Liu <[email protected]>
Co-authored-by: Alice Cecile <[email protected]>
mtsr pushed a commit to mtsr/bevy that referenced this issue Mar 15, 2024
# Objective

- Closes bevyengine#12256 

## Solution

- Improved component hooks docs.
- Improved component hooks example docs.

---------

Co-authored-by: James Liu <[email protected]>
Co-authored-by: Alice Cecile <[email protected]>
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-Docs An addition or correction to our documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants