Skip to content

Conversation

@ChristopherBiscardi
Copy link
Contributor

@ChristopherBiscardi ChristopherBiscardi commented Jul 2, 2024

and resources, including rc non-mesh fixes.

fixes #1386

I picked URLs that would work today so review could be made, assuming they'd need to be changed either before or after merging.

@ChristopherBiscardi ChristopherBiscardi force-pushed the binned-sorted-render-phases branch from 60eb963 to ddb512f Compare July 2, 2024 01:53
@alice-i-cecile alice-i-cecile added this to the Release v0.14 milestone Jul 2, 2024
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

LGTM in terms of technical writing and clarity.

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

On a technical note though, this new file should be in migration-guides, and you need to update the .toml file at the top of that folder to reflect the two removed and one added file :)

@ChristopherBiscardi
Copy link
Contributor Author

I don't see any other entries in the toml file with combined/multiple PR urls so I left it as the original PR which introduced the Sorted/Binned. The other two PRs are the resource move and the non-mesh/hanabi fix

@BD103
Copy link
Member

BD103 commented Jul 2, 2024

I'll take a look at this once I finish the migration guides. (10 more to go!)

@BD103 BD103 self-requested a review July 2, 2024 02:07
alice-i-cecile and others added 2 commits July 2, 2024 09:40
Co-authored-by: BD103 <59022059+BD103@users.noreply.github.com>
Co-authored-by: BD103 <59022059+BD103@users.noreply.github.com>
@alice-i-cecile alice-i-cecile enabled auto-merge July 2, 2024 13:41
@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review Ready for a maintainer to consider for merging label Jul 2, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jul 2, 2024
Copy link
Contributor

@djeedai djeedai left a comment

Choose a reason for hiding this comment

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

Looks like a good start, but I'm not sure this is enough to migrate code. The move to resources for example definitely requires seeing a before/after to understand how to transform one's code. Selecting the sorted or binned phase item was also the easy part; it's after to implement the binned one that problems start.

I think we need to explain:

  • what's binning? why is it better than sorting and when can you use it or not?
  • how do you come up with a bin key?
  • how do you query the phase item container from the resource and the camera view entity?
  • caveats of using non mesh (there's actually no binning!)


If you're looking for a quick migration, consider picking [`SortedPhaseItem`](https://docs.rs/bevy/0.14.0-rc.4/bevy/render/render_phase/trait.SortedPhaseItem.html) which requires the fewest code changes.

If you're looking for higher performance (and your phase doesn’t require sorting) you may want to pick [`BinnedPhaseItem`](https://docs.rs/bevy/0.14.0-rc.4/bevy/render/render_phase/trait.BinnedPhaseItem.html). Notably bins are populated based on `BinKey` and everything in the same bin is potentially batchable.
Copy link
Contributor

Choose a reason for hiding this comment

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

"is potentially batchable" or is actually batched (binned)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"potentially batchable" is the wording used in the original PR, so I deferred to that - bevyengine/bevy#12453


Examples of [`BinnedPhaseItem`s](https://docs.rs/bevy/0.14.0-rc.4/bevy/render/render_phase/trait.BinnedPhaseItem.html#implementors) include:

- Opaque3d
Copy link
Contributor

Choose a reason for hiding this comment

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

Link them all maybe? (sorry for the extra work 😅)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could do. I linked Shadow specifically because to someone less familiar it looks out of place next to things that are clearly 2d/3d forward/deferred/prepass related. The full list is already available at the "Examples of" links.

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Jul 2, 2024
@alice-i-cecile alice-i-cecile added S-Waiting-On-Author S-Ready-For-Final-Review Ready for a maintainer to consider for merging and removed S-Ready-For-Final-Review Ready for a maintainer to consider for merging S-Waiting-On-Author labels Jul 2, 2024
@alice-i-cecile
Copy link
Member

I'm going to merge this as is to avoid unduly burdening @ChristopherBiscardi, whose help I really appreciate here. I agree that these are important things to explain @djeedai, but IMO these point to a failing of the underlying documentation, and don't belong in a migration guide.

@alice-i-cecile alice-i-cecile enabled auto-merge July 2, 2024 18:32
@alice-i-cecile
Copy link
Member

Looks like the name of the file doesn't match somehow; see the failed CI. I'll take a look later unless someone beats me to it.

auto-merge was automatically disabled July 2, 2024 19:04

Head branch was pushed to by a user without write access

@ChristopherBiscardi
Copy link
Contributor Author

@alice-i-cecile merging the main branch into this branch caused the previously-removed file to be re-inserted into the toml file.

I re-removed it

@alice-i-cecile alice-i-cecile enabled auto-merge July 2, 2024 19:05
@alice-i-cecile
Copy link
Member

Thanks!

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jul 2, 2024
@ChristopherBiscardi
Copy link
Contributor Author

also @djeedai fwiw the length and content of this was informed by the length and content of other migration guides. I agree with @alice-i-cecile and do consider the new binned functionality to not just be a migration (migration would be to sorted, which is where "the previous functionality lives") but also an adoption of new functionality, thus I would expect implementing binning to require reading binning docs which should explain how to implement it (which they do not today imo, similar to most lower level rendering unfortunately).

The new custom_phase_item example does show the larger context and is linked in the migration so I leaned on that existing for people who need larger context around an example. The example does show how to get view entities, etc.

Merged via the queue into bevyengine:main with commit 1fc8745 Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Migration Guides C-Content S-Ready-For-Final-Review Ready for a maintainer to consider for merging

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0.14 Migration: Consolidate binned render phases changes

4 participants