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

Genenerate doc comments from Required Components #14800

Closed
killercup opened this issue Aug 17, 2024 · 7 comments
Closed

Genenerate doc comments from Required Components #14800

killercup opened this issue Aug 17, 2024 · 7 comments
Labels
A-ECS Entities, components, systems, and events C-Docs An addition or correction to our documentation C-Feature A new feature, making something new possible X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers
Milestone

Comments

@killercup
Copy link
Contributor

What problem does this solve or what need does it fill?

As a user of components with Required Components (#14791),
I want to see what other components are going to be inserted easily.

What solution would you like?

Adding the #[required(…)] attribute should also add a doc comment.
This will be shown by many IDEs when hovering (or similar) over the type.

Example

/// Makes entity behave like a button by adding sparkle effects on hover.
#[derive(Component)]
#[require(Node, UiImage)]
struct Button;

generates

/// Makes entity behave like a button by adding sparkle effects on hover.
///
/// # Automatically added components
///
/// When inserting this component,
/// [`Node`], and [`UiImage`]
//  are also inserted if not already present.
struct Button;

What alternative(s) have you considered?

  • Telling people to do this manually
  • Hoping that IDEs will do something else to show this

Additional context

#14791 is only proposed as of writing this.
This issue may become invalid if it not accepted in its current state.

@killercup killercup added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Aug 17, 2024
@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Aug 17, 2024
@alice-i-cecile alice-i-cecile added C-Docs An addition or correction to our documentation A-ECS Entities, components, systems, and events X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers S-Blocked This cannot move forward until something else changes and removed S-Needs-Triage This issue needs to be labelled labels Aug 17, 2024
@alice-i-cecile
Copy link
Member

A suggests attribute was also proposed and widely loved. We can probably do them in one PR.

@ItsDoot
Copy link
Contributor

ItsDoot commented Aug 17, 2024

If we want the doc comments on the derived struct itself, we would have to use an attribute proc macro. Derive macros can only generate additional AST, not edit the original. Unless... could we fake it by #[requires()] and #[suggests()] being both a derive macro attribute AND a attribute proc macro? Worth looking into.

@killercup
Copy link
Contributor Author

Derive macros can only generate additional AST, not edit the original.

Hm. Adding the docs to the Component impl would be next to useless as you'd only ever name the type and never the trait, right?

Unless... could we fake it by #[requires()] and #[suggests()] being both a derive macro attribute AND a attribute proc macro? Worth looking into.

That's the mad genius kinda thing that I'm into, but it might also be a bit surprising to users. I'd try it but see how people feel about it before shipping it in a release.

@cart
Copy link
Member

cart commented Aug 24, 2024

I was able to add this in the derive macro by adding a #[doc = ""] attribute to the Component impl. Obviously not ideal because its not directly on the type (and therefore won't show up in most IDE hovers). But it is pretty visible / usable from the generated docs:

image

github-merge-queue bot pushed a commit that referenced this issue Aug 27, 2024
## Introduction

This is the first step in my [Next Generation Scene / UI
Proposal](#14437).

Fixes #7272 #14800.

Bevy's current Bundles as the "unit of construction" hamstring the UI
user experience and have been a pain point in the Bevy ecosystem
generally when composing scenes:

* They are an additional _object defining_ concept, which must be
learned separately from components. Notably, Bundles _are not present at
runtime_, which is confusing and limiting.
* They can completely erase the _defining component_ during Bundle init.
For example, `ButtonBundle { style: Style::default(), ..default() }`
_makes no mention_ of the `Button` component symbol, which is what makes
the Entity a "button"!
* They are not capable of representing "dependency inheritance" without
completely non-viable / ergonomically crushing nested bundles. This
limitation is especially painful in UI scenarios, but it applies to
everything across the board.
* They introduce a bunch of additional nesting when defining scenes,
making them ugly to look at
* They introduce component name "stutter": `SomeBundle { component_name:
ComponentName::new() }`
* They require copious sprinklings of `..default()` when spawning them
in Rust code, due to the additional layer of nesting

**Required Components** solve this by allowing you to define which
components a given component needs, and how to construct those
components when they aren't explicitly provided.

This is what a `ButtonBundle` looks like with Bundles (the current
approach):

```rust
#[derive(Component, Default)]
struct Button;

#[derive(Bundle, Default)]
struct ButtonBundle {
    pub button: Button,
    pub node: Node,
    pub style: Style,
    pub interaction: Interaction,
    pub focus_policy: FocusPolicy,
    pub border_color: BorderColor,
    pub border_radius: BorderRadius,
    pub image: UiImage,
    pub transform: Transform,
    pub global_transform: GlobalTransform,
    pub visibility: Visibility,
    pub inherited_visibility: InheritedVisibility,
    pub view_visibility: ViewVisibility,
    pub z_index: ZIndex,
}

commands.spawn(ButtonBundle {
    style: Style {
        width: Val::Px(100.0),
        height: Val::Px(50.0),
        ..default()
    },
    focus_policy: FocusPolicy::Block,
    ..default()
})
```

And this is what it looks like with Required Components:

```rust
#[derive(Component)]
#[require(Node, UiImage)]
struct Button;

commands.spawn((
    Button,
    Style { 
        width: Val::Px(100.0),
        height: Val::Px(50.0),
        ..default()
    },
    FocusPolicy::Block,
));
```

With Required Components, we mention only the most relevant components.
Every component required by `Node` (ex: `Style`, `FocusPolicy`, etc) is
automatically brought in!

### Efficiency

1. At insertion/spawn time, Required Components (including recursive
required components) are initialized and inserted _as if they were
manually inserted alongside the given components_. This means that this
is maximally efficient: there are no archetype or table moves.
2. Required components are only initialized and inserted if they were
not manually provided by the developer. For the code example in the
previous section, because `Style` and `FocusPolicy` are inserted
manually, they _will not_ be initialized and inserted as part of the
required components system. Efficient!
3. The "missing required components _and_ constructors needed for an
insertion" are cached in the "archetype graph edge", meaning they aren't
computed per-insertion. When a component is inserted, the "missing
required components" list is iterated (and that graph edge (AddBundle)
is actually already looked up for us during insertion, because we need
that for "normal" insert logic too).

### IDE Integration

The `#[require(SomeComponent)]` macro has been written in such a way
that Rust Analyzer can provide type-inspection-on-hover and `F12` /
go-to-definition for required components.

### Custom Constructors

The `require` syntax expects a `Default` constructor by default, but it
can be overridden with a custom constructor:

```rust
#[derive(Component)]
#[require(
    Node,
    Style(button_style),
    UiImage
)]
struct Button;

fn button_style() -> Style {
    Style {
        width: Val::Px(100.0),
        ..default()
    }
}
```

### Multiple Inheritance

You may have noticed by now that this behaves a bit like "multiple
inheritance". One of the problems that this presents is that it is
possible to have duplicate requires for a given type at different levels
of the inheritance tree:

```rust
#[derive(Component)
struct X(usize);

#[derive(Component)]
#[require(X(x1))
struct Y;

fn x1() -> X {
    X(1)
}

#[derive(Component)]
#[require(
    Y,
    X(x2),
)]
struct Z;

fn x2() -> X {
    X(2)
}

// What version of X is inserted for Z?
commands.spawn(Z);
```

This is allowed (and encouraged), although this doesn't appear to occur
much in practice. First: only one version of `X` is initialized and
inserted for `Z`. In the case above, I think we can all probably agree
that it makes the most sense to use the `x2` constructor for `X`,
because `Y`'s `x1` constructor exists "beneath" `Z` in the inheritance
hierarchy; `Z`'s constructor is "more specific".

The algorithm is simple and predictable:

1. Use all of the constructors (including default constructors) directly
defined in the spawned component's require list
2. In the order the requires are defined in `#[require()]`, recursively
visit the require list of each of the components in the list (this is a
depth Depth First Search). When a constructor is found, it will only be
used if one has not already been found.

From a user perspective, just think about this as the following:

1. Specifying a required component constructor for `Foo` directly on a
spawned component `Bar` will result in that constructor being used (and
overriding existing constructors lower in the inheritance tree). This is
the classic "inheritance override" behavior people expect.
2. For cases where "multiple inheritance" results in constructor
clashes, Components should be listed in "importance order". List a
component earlier in the requirement list to initialize its inheritance
tree earlier.

Required Components _does_ generally result in a model where component
values are decoupled from each other at construction time. Notably, some
existing Bundle patterns use bundle constructors to initialize multiple
components with shared state. I think (in general) moving away from this
is necessary:

1. It allows Required Components (and the Scene system more generally)
to operate according to simple rules
2. The "do arbitrary init value sharing in Bundle constructors" approach
_already_ causes data consistency problems, and those problems would be
exacerbated in the context of a Scene/UI system. For cases where shared
state is truly necessary, I think we are better served by observers /
hooks.
3. If a situation _truly_ needs shared state constructors (which should
be rare / generally discouraged), Bundles are still there if they are
needed.

## Next Steps

* **Require Construct-ed Components**: I have already implemented this
(as defined in the [Next Generation Scene / UI
Proposal](#14437). However
I've removed `Construct` support from this PR, as that has not landed
yet. Adding this back in requires relatively minimal changes to the
current impl, and can be done as part of a future Construct pr.
* **Port Built-in Bundles to Required Components**: This isn't something
we should do right away. It will require rethinking our public
interfaces, which IMO should be done holistically after the rest of Next
Generation Scene / UI lands. I think we should merge this PR first and
let people experiment _inside their own code with their own Components_
while we wait for the rest of the new scene system to land.
* **_Consider_ Automatic Required Component Removal**: We should
evaluate _if_ automatic Required Component removal should be done. Ex:
if all components that explicitly require a component are removed,
automatically remove that component. This issue has been explicitly
deferred in this PR, as I consider the insertion behavior to be
desirable on its own (and viable on its own). I am also doubtful that we
can find a design that has behavior we actually want. Aka: can we
_really_ distinguish between a component that is "only there because it
was automatically inserted" and "a component that was necessary / should
be kept". See my [discussion response
here](#14437 (reply in thread))
for more details.

---------

Co-authored-by: Alice Cecile <[email protected]>
Co-authored-by: BD103 <[email protected]>
Co-authored-by: Pascal Hertleif <[email protected]>
@bushrat011899 bushrat011899 removed the S-Blocked This cannot move forward until something else changes label Aug 28, 2024
@killercup
Copy link
Contributor Author

could we fake it by #[requires()] and #[suggests()] being both a derive macro attribute AND a attribute proc macro? Worth looking into.

I just tested a custom macro crate that just exports

#[proc_macro_attribute]
pub fn require(attr: TokenStream, item: TokenStream) -> TokenStream {
    panic!("test");
}

Main finding:

use bevy::prelude::*;
use require_macro::require;

#[derive(Component)]
#[require(Transform)]
pub struct Foo;

– this does not call the macro!

Only when flipping the attributes like this:

#[require(Transform)]
#[derive(Component)]
pub struct Foo;

does it get interesting, but sadly also not in a way we want:

error[E0659]: `require` is ambiguous
 --> crates/test-required-docs/src/lib.rs:4:3
  |
4 | #[require(Transform)]
  |   ^^^^^^^ ambiguous name
  |
  = note: ambiguous because of a name conflict with a derive helper attribute
note: `require` could refer to the derive helper attribute defined here
 --> crates/test-required-docs/src/lib.rs:5:10
  |
5 | #[derive(Component)]
  |          ^^^^^^^^^
note: `require` could also refer to the attribute macro imported here
 --> crates/test-required-docs/src/lib.rs:2:5
  |
2 | use require_macro::require;
  |     ^^^^^^^^^^^^^^^^^^^^^^
  = help: use `crate::require` to refer to this attribute macro unambiguously

So for now, I don't see a way forward with this.

@alice-i-cecile
Copy link
Member

Alright, I'm going to close this as completed: I don't think we're going to do better than the initial implementation in #14971.

@killercup
Copy link
Contributor Author

Fair enough. Unless the whole Component derive becomes a proc macro (which would be surprising to users I bet), there is no easy way to get this. Next best thing will be editor integration I guess!

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 C-Feature A new feature, making something new possible X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers
Projects
None yet
Development

No branches or pull requests

5 participants