[Merged by Bors] - bevy_reflect: Improved documentation #7148

MrGVSV · 2023-01-10T07:05:35Z

Objective

bevy_reflect can be a moderately complex crate to try and understand. It has many moving parts, a handful of gotchas, and a few subtle contracts that aren't immediately obvious to users and even other contributors.

The current README does an okay job demonstrating how the crate can be used. However, the crate's actual documentation should give a better overview of the crate, its inner-workings, and show some of its own examples.

Solution

Added crate-level documentation that attempts to summarize the main parts of bevy_reflect into small sections.

This PR also updates the documentation for:

Reflect
FromReflect
The reflection subtraits
Other important types and traits
The reflection macros (including the derive macros)
Crate features

Open Questions

~~Should I update the docs for the Dynamic types? I was originally going to, but I'm getting a little concerned about the size of this PR 😅~~ Decided to not do this in this PR. It'll be better served from its own PR.
Should derive macro documentation be moved to the trait itself? This could improve visibility and allow for better doc links, but could also clutter up the trait's documentation (as well as not being on the actual derive macro's documentation).

TODO

~~Document Dynamic types (?)~~ I think this should be done in a separate PR.
Document crate features
Update docs for GetTypeRegistration
Update docs for TypeRegistration
Update docs for derive_from_reflect
Document reflect_trait
Document impl_reflect_value
Document impl_from_reflect_value

Changelog

Updated documentation across the bevy_reflect crate
Removed #[module] helper attribute for Reflect derives (this is not currently used)

Migration Guide

Removed #[module] helper attribute for Reflect derives. If your code is relying on this attribute, please replace it with either #[reflect] or #[reflect_value] (dependent on use-case).

crates/bevy_reflect/bevy_reflect_derive/src/lib.rs

crates/bevy_reflect/src/array.rs

crates/bevy_reflect/src/lib.rs

crates/bevy_reflect/src/list.rs

alice-i-cecile

This is an incredibly useful introduction and well-written. I've left some feedback to improve wording and add clarity at various points.

nfagerlund

This is awesome. I bounced off a previous attempt to understand how this machinery worked, and I feel like I learned a lot from reading this.

Here's a few straightforward typo fixes.

crates/bevy_reflect/src/lib.rs

alice-i-cecile

Really outstanding. I'm very excited to see one of the most arcane bits of the engine get a very practical and approachable set of docs.

crates/bevy_reflect/src/array.rs

crates/bevy_reflect/src/from_reflect.rs

MinerSebas · 2023-02-02T13:55:25Z

crates/bevy_reflect/bevy_reflect_derive/src/lib.rs

+/// The `#[reflect(ignore)]` attribute is shared with the [`#[derive(Reflect)]`](Reflect) macro and has much of the same
+/// functionality in that it marks a field to be ignored by the reflection API.
+///
+/// The only major difference is that using it with this derive requires that the field implements [`Default`].


I wanted to resolve the Fixme here: https://github.com/bevyengine/bevy/blob/main/crates/bevy_asset/src/handle.rs#L119

Initially, I was confused why it works, as when expanding the derive, the Default of Handle<T> was used.

Reading the following from the Reflect documentation of this PR removed the confusion:

/// * `#[reflect(Default)]` will register the `ReflectDefault` type data as normal. /// However, it will also affect how certain other operations are performed in order /// to improve performance and/or robustness. /// An example of where this is used is in the [`FromReflect`] derive macro, /// where adding this attribute will cause the `FromReflect` implementation to create /// a base value using its [`Default`] implementation avoiding issues with ignored fields.

I don't know how to properly write this down here, but probably something like this:

Suggested change

/// The only major difference is that using it with this derive requires that the field implements [`Default`].

/// The only major difference is that using it with this derive requires that the field implements [`Default`] or the struct to have a `#[reflect(Default)]`.

Actually #[reflect(Default)]'s special casing is likely to be removed by #7317. Ignored fields will use Default anyways, I’m pretty sure. If not— or if they want a custom default— they can mark the ignored field with #[reflect(default)].

So I’m not sure we need to strictly change this.

Nilirad

@MrGVSV Here are some observations. Mostly you can find the things we discussed earlier on Discord. There are additional thoughts, but some of them are relatively unprocessed, so feel free to skip them if you struggle to make sense of them.

Crate-level docs. The first paragraph is quite obscure. Can be done better. Introduces jargon (metaprogramming) that seems not necessary to understand the topic.
Limitations. Readers should be able to get a pretty solid idea of what this crate can't do compared with languages with full reflection support.
Crate-level docs are too cluttered. The culprit seems to be the insertion of item specific documentation. Some amount of overview is acceptable until the topic is covered by the Bevy book. However, the main page should explain how the main elements of the crate are connected together and rely on linking to items for detail.
- Grouping into modules. Some items (the various dynamic types, and the Reflect subtraits) can be grouped into modules. This will make room for documentation and removes the need to manually list items.
The name “Reflect subtraits” doesn't give the idea. I think it's possible to find a better name for those.

crates/bevy_reflect/bevy_reflect_derive/src/lib.rs

crates/bevy_reflect/src/lib.rs

Carter0 · 2023-02-14T23:31:30Z

crates/bevy_reflect/src/lib.rs

+//! # struct MyStruct {
+//! #   foo: i32
+//! # }
+//! let original: Box<dyn Reflect> = Box::new(MyStruct {


Why are you wrapping everything in Box::new() for the examples?

This is mainly so that we could strongly type everything as Box<dyn Reflect>. My hope in doing so was to:

Show that this is purely Reflect doing these things

Show that reflection data is normally passed around as trait objects

If this is too confusing/verbose, though, we can remove it!

I think it's fine, I think I understand now. We are doing dynamic dispatch here and I am assuming that we can't do that unless we have allocated a specific amount of memory allocated already.

Sorta. The Box isn't required here. It's just to be explicit in showing that it's the struct as a dyn Reflect object that we care about.

crates/bevy_reflect/src/lib.rs

Carter0

I loved this, thank you so much for doing it! I think stuff like this is what makes open source stuff so great.

I think that it's worth considering what the goal of this documentation here is. If the goal of the documentation is to explain what's going on here to the complete beginner, then I would give it a 5/10. I for sure know a lot more than I did before, but I think a "getting started" blog post or a practical example would go a lot farther.

If the goal is to provide more context to someone who knows a little, maybe the basics, but not a lot, then I would give it a 10/10. I think that this is invaluable to someone who wants to make sure their understanding is correct, or to someone who wants to learn a little more.

Thanks for writing this up! I appreciate it :)

MrGVSV · 2023-02-18T17:05:03Z

I think that it's worth considering what the goal of this documentation here is. If the goal of the documentation is to explain what's going on here to the complete beginner, then I would give it a 5/10. I for sure know a lot more than I did before, but I think a "getting started" blog post or a practical example would go a lot farther.

If the goal is to provide more context to someone who knows a little, maybe the basics, but not a lot, then I would give it a 10/10. I think that this is invaluable to someone who wants to make sure their understanding is correct, or to someone who wants to learn a little more.

Great points. Yeah this is not meant as a "getting started" guide or tutorial, just an explanation of the API and how it works together at a high level. I'd say that yeah it's meant for those who know the basics of programming and reflection (or maybe just needs a refresher).

The hope is that we eventually have a book that goes into more detail (in Bevy as a whole even)— complete with better examples and use cases. But until then, this should be enough to get started either as a user of bevy_reflect or a contributor.

Thanks for writing this up! I appreciate it :)

Thanks for the review and feedback! I'm glad it's helping out already 😄

MrGVSV · 2023-02-18T17:20:34Z

Crate-level docs. The first paragraph is quite obscure. Can be done better. Introduces jargon (metaprogramming) that seems not necessary to understand the topic.

Yeah it's difficult to fully express what reflection is without getting too deep in the weeds. My intention here, though, is to give a quick refresher for those that are somewhat familiar (and/or give some topics for newcomers to research on their own).

I try my best to explain the jargon (i.e. roughly defining "metaprogramming" as "using information about the program to affect the program"), but there are probably places that are lacking. I'm open to suggestions!

Limitations. Readers should be able to get a pretty solid idea of what this crate can't do compared with languages with full reflection support.

That's probably a good idea. Maybe I'll add a section at the bottom for a list of basic limitations when compared to reflection in things like C# and JavaScript? 🤔

Crate-level docs are too cluttered. The culprit seems to be the insertion of item specific documentation. Some amount of overview is acceptable until the topic is covered by the Bevy book. However, the main page should explain how the main elements of the crate are connected together and rely on linking to items for detail.

Grouping into modules. Some items (the various dynamic types, and the Reflect subtraits) can be grouped into modules. This will make room for documentation and removes the need to manually list items.

I think these two points can come as part of the bevy_reflect reorganization efforts we discussed in Discord. It will be easier to split out this PR's crate-level docs when we have modules that are better suited to particular topics.

This can be done in a followup PR (and likely for the 0.11 release).

The name “Reflect subtraits” doesn't give the idea. I think it's possible to find a better name for those.

Yeah, the current terminology is pretty vague. So far, the best replacement term we have is "kind" (suggested in #7075 as ReflectKind due to usage in ReflectRef et al.). I think we should defer this diction decision for after the 0.10 release. If we go with "kind" (or even som other term), it will make more sense with the crate reorganization. Thoughts?

Co-authored-by: Alice Cecile <[email protected]> Co-authored-by: Nick Fagerlund <[email protected]>

alice-i-cecile · 2023-02-18T20:25:56Z

Unless you have objections, I'd like to merge this now. This is a dramatic improvement, and further changes will be easier to discuss in dedicated threads.

alice-i-cecile · 2023-02-18T20:41:43Z

bors r+

# Objective `bevy_reflect` can be a moderately complex crate to try and understand. It has many moving parts, a handful of gotchas, and a few subtle contracts that aren't immediately obvious to users and even other contributors. The current README does an okay job demonstrating how the crate can be used. However, the crate's actual documentation should give a better overview of the crate, its inner-workings, and show some of its own examples. ## Solution Added crate-level documentation that attempts to summarize the main parts of `bevy_reflect` into small sections. This PR also updates the documentation for: - `Reflect` - `FromReflect` - The reflection subtraits - Other important types and traits - The reflection macros (including the derive macros) - Crate features ### Open Questions 1. ~~Should I update the docs for the Dynamic types? I was originally going to, but I'm getting a little concerned about the size of this PR 😅~~ Decided to not do this in this PR. It'll be better served from its own PR. 2. Should derive macro documentation be moved to the trait itself? This could improve visibility and allow for better doc links, but could also clutter up the trait's documentation (as well as not being on the actual derive macro's documentation). ### TODO - [ ] ~~Document Dynamic types (?)~~ I think this should be done in a separate PR. - [x] Document crate features - [x] Update docs for `GetTypeRegistration` - [x] Update docs for `TypeRegistration` - [x] Update docs for `derive_from_reflect` - [x] Document `reflect_trait` - [x] Document `impl_reflect_value` - [x] Document `impl_from_reflect_value` --- ## Changelog - Updated documentation across the `bevy_reflect` crate - Removed `#[module]` helper attribute for `Reflect` derives (this is not currently used) ## Migration Guide - Removed `#[module]` helper attribute for `Reflect` derives. If your code is relying on this attribute, please replace it with either `#[reflect]` or `#[reflect_value]` (dependent on use-case). Co-authored-by: Gino Valente <[email protected]>

bors · 2023-02-18T21:04:42Z

Pull request successfully merged into main.

Build succeeded:

# Objective `bevy_reflect` can be a moderately complex crate to try and understand. It has many moving parts, a handful of gotchas, and a few subtle contracts that aren't immediately obvious to users and even other contributors. The current README does an okay job demonstrating how the crate can be used. However, the crate's actual documentation should give a better overview of the crate, its inner-workings, and show some of its own examples. ## Solution Added crate-level documentation that attempts to summarize the main parts of `bevy_reflect` into small sections. This PR also updates the documentation for: - `Reflect` - `FromReflect` - The reflection subtraits - Other important types and traits - The reflection macros (including the derive macros) - Crate features ### Open Questions 1. ~~Should I update the docs for the Dynamic types? I was originally going to, but I'm getting a little concerned about the size of this PR 😅~~ Decided to not do this in this PR. It'll be better served from its own PR. 2. Should derive macro documentation be moved to the trait itself? This could improve visibility and allow for better doc links, but could also clutter up the trait's documentation (as well as not being on the actual derive macro's documentation). ### TODO - [ ] ~~Document Dynamic types (?)~~ I think this should be done in a separate PR. - [x] Document crate features - [x] Update docs for `GetTypeRegistration` - [x] Update docs for `TypeRegistration` - [x] Update docs for `derive_from_reflect` - [x] Document `reflect_trait` - [x] Document `impl_reflect_value` - [x] Document `impl_from_reflect_value` --- ## Changelog - Updated documentation across the `bevy_reflect` crate - Removed `#[module]` helper attribute for `Reflect` derives (this is not currently used) ## Migration Guide - Removed `#[module]` helper attribute for `Reflect` derives. If your code is relying on this attribute, please replace it with either `#[reflect]` or `#[reflect_value]` (dependent on use-case). Co-authored-by: Gino Valente <[email protected]>

MrGVSV added C-Docs An addition or correction to our documentation A-Reflection Runtime information about types labels Jan 10, 2023

alice-i-cecile added this to the 0.10 milestone Jan 10, 2023