Skip to content

[Merged by Bors] - Document the new pipelined renderer#3094

Closed
dataphract wants to merge 5 commits intobevyengine:pipelined-renderingfrom
dataphract:ku95/renderer-documentation
Closed

[Merged by Bors] - Document the new pipelined renderer#3094
dataphract wants to merge 5 commits intobevyengine:pipelined-renderingfrom
dataphract:ku95/renderer-documentation

Conversation

@dataphract
Copy link
Member

@dataphract dataphract commented Nov 9, 2021 •
edited
Loading

This is a squash-and-rebase of @ku95's documentation of the new renderer onto the latest pipelined-rendering branch.

Original PR is #2884.

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Docs An addition or correction to our documentation S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Nov 9, 2021
@alice-i-cecile alice-i-cecile mentioned this pull request Nov 9, 2021
Closed
@alice-i-cecile alice-i-cecile added this to the Bevy 0.6 milestone Nov 9, 2021
cart
cart reviewed Nov 12, 2021
View reviewed changes
Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

Great work! Just a couple of small "doc philosophy" changes in a couple of places.

pipelined/bevy_render2/src/render_component.rs Outdated
}
}

/// Implements [`ExtractComponent`] for all asset handles.
Copy link
Member

@cart cart Nov 12, 2021

Choose a reason for hiding this comment

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

I think we should do a pass to remove "obvious" docs like this that just rephrase type information (especially for trait impls like this). I think they have net negative value because they take up space

pipelined/bevy_render2/src/render_component.rs Outdated
<C::Filter as WorldQuery>::Fetch: FilterFetch,
<C::Query as WorldQuery>::Fetch: ReadOnlyFetch,
{
/// Specifies all required ECS data.
Copy link
Member

@cart cart Nov 12, 2021

Choose a reason for hiding this comment

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

Re-documenting trait impl functions with copies of the source docs has net-negative value in my opinion:

  1. It requires updating every implementation whenever the source trait changes
  2. It just takes up space.

We should only add documentation to specific impls when it actually adds new context.

Can you take a pass and remove cases like this? (and in cases like this where the main Trait docs are missing, move the docs there)

@dataphract dataphract force-pushed the ku95/renderer-documentation branch from 2b7b6f1 to 93d27d2 Compare November 13, 2021 06:20
@cart
Copy link
Member

cart commented Nov 16, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented Nov 16, 2021

Merge conflict.

@cart
Copy link
Member

cart commented Nov 16, 2021

bors r+

bors bot pushed a commit that referenced this pull request Nov 16, 2021
@kurtkuehnert @dataphract @cart
Document the new pipelined renderer (#3094)
1076a8f 
This is a squash-and-rebase of @ku95's documentation of the new renderer onto the latest `pipelined-rendering` branch.

Original PR is #2884.

Co-authored-by: dataphract <dataphract@gmail.com>
Co-authored-by: Carter Anderson <mcanders1@gmail.com>
@bors
Copy link
Contributor

bors bot commented Nov 16, 2021

Pull request successfully merged into pipelined-rendering.

Build succeeded:

@bors bors bot changed the title Document the new pipelined renderer [Merged by Bors] - Document the new pipelined renderer Nov 16, 2021
@bors bors bot closed this Nov 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cart cart cart left review comments

Assignees

No one assigned

Labels

A-Rendering Drawing game state to the screen C-Docs An addition or correction to our documentation S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

None yet

Milestone

Bevy 0.6

Development

Successfully merging this pull request may close these issues.

4 participants

@dataphract @cart @alice-i-cecile @kurtkuehnert