[Merged by Bors] - Port bevy_ui to pipelined-rendering #2653

Davier · 2021-08-14T13:59:13Z

Objective

Port bevy_ui to pipelined-rendering (see #2535 )

Solution

I did some changes during the port:

separate color from the texture asset (as suggested here)
~~give the vertex shader a per-instance buffer instead of per-vertex buffer~~ (incompatible with batching)

Remaining features to implement to reach parity with the old renderer:

textures
TextBundle

I'd also like to add these features, but they need some design discussion:

cart · 2021-08-14T19:43:13Z

Brilliant! I'll take a look at this ASAP.

In regard to your second and third lists:

Fortunately we already have a draft MSAA implementation.

I'm cool with entirely ignoring the third list in the description. I'd prefer to not focus any time on adding new features to the current Bevy UI system, as thats next on our list of things to rework.

alice-i-cecile · 2021-08-16T15:39:30Z

For batching, perhaps coordinate with #2642?

cart · 2021-08-18T00:31:21Z

For batching, perhaps coordinate with #2642?

I'd honestly rather just punt it unless @Davier is extremely motivated. We should invest the smallest amount of time possible in "legacy stuff".

Davier · 2021-08-21T15:13:56Z

For batching, perhaps coordinate with #2642?

I've taken a lot of inspiration from that PR already, so implementing batching should be straightforward. I'm happy to do it (and any other point in the 3rd list) if you agree to review it, but I understand that you have a limited bandwidth.

I'd honestly rather just punt it unless @Davier is extremely motivated. We should invest the smallest amount of time possible in "legacy stuff".

I'm doing it to learn so it's fine even if we throw it away soon. Or is there is another part of the renderer that I could help with?

examples/README.md

cart · 2021-08-24T18:09:25Z

I would review it, but given that I haven't reviewed the current batching pr yet, maybe hold off until after. There are a few corner cases that I'm worried about that I haven't tested on the current impl.

Davier · 2021-10-16T13:49:05Z

Text and Text2d are now implemented. The ui_pipelined and text2d_pipelined examples seem to work correctly.
Should we start to review and merge now so that more people can try out the new renderer? Or wait for the other in-flight PRs to be merged (shaders as assets, sprite batching, MSAA, ...)?

Davier · 2021-11-24T14:55:56Z

Updated on top of pipelined-rendering. The first commit is a simple copy of the old bevy_ui and bevy_text to make reviewing changes easier.
I used the last changes in bevy_sprites, including batching.
I'll rebase on top of #3175 when it's merged.

Remaining issues/questions:

the background is not cleared with ClearColor if there is no 2d or 3d camera, in that case the UI is drawn on top of garbage (see Support for RenderPass that renders to WindowSwapChain before MainPass #3190).
the VisibleEntities component is required for a camera to work, even if the UI camera has no culling
should the UiPipeline specialization be removed since the key is empty? I kept it to make it easier to add new specializations in the future.
~~should the UI renderer use MSAA?~~ (it's not fit for 2D rendering as discussed on discord, we should consider other anti-aliasing methods such as TAA)

Possible future improvements:

clipping
texture atlases
opaque / transparent / alpha mask phases
use depth buffer to avoid overdraw
include flags in the vertex buffer and skip sampling the texture when possible (like for PBR)

cart · 2021-11-24T21:50:38Z

Changed the base branch to main because thats where development is happening now!

colepoirier

I approve, but someone with more bevy experience and/or more experience with the new renderer will need to do the final sign off. I don’t see any obvious problems but I don’t think I would be able to see a great deal of the problems if they do exist.

Davier

I did another full self review. I caught a few things and detailed a few others, hopefully it will help other reviewers.
I recommend selecting all but the first commit in the filters, to view what actually changed.

crates/bevy_asset/src/handle.rs

examples/ui/ui_pipelined.rs

pipelined/bevy_core_pipeline/src/lib.rs

Davier · 2021-11-28T09:22:55Z

pipelined/bevy_text2/src/text2d.rs

-    for (entity, mut draw, visible, text, global_transform, calculated_size) in query.iter_mut() {
-        if !visible.is_visible {
-            continue;
-        }
-


This should be updated now that we have Visibility and ComputedVisibility

Davier · 2021-11-28T09:35:23Z

pipelined/bevy_ui2/src/entity.rs

+    // FIXME there is no frustrum culling for UI
+    pub visible_entities: VisibleEntities,


I considered there is no need for frustum culling since every UI item is usually on-screen, so I didn't use Visibility, ComputedVisibility and VisibleEntities. VisibleEntities is only included here because the camera systems require it.

However I now realize we need a way to manually hide on-screen nodes, like it was possible with the Visible component. Instead of adding the full frustum culling like in bevy_sprite2, I think I'll just skip queuing when Style::display == Display::None.

In case of a scrollable element part of the UI items are likely outside of the bounds of the scrollable element and thus must be invisible.

I'm tackling that case in another (soon to be) PR for clipping, where I implement the Style::overflow property (it is currently commented out). I calculate how items are clipped in the queue_uinode fn, and skip those completely clipped. It instead might be possible to calculate it beforehand and setting a ComputedVisibility accordingly.

pipelined/bevy_ui2/src/lib.rs

pipelined/bevy_ui2/src/render/mod.rs

pipelined/bevy_ui2/src/render/render_pass.rs

pipelined/bevy_ui2/src/ui_node.rs

pipelined/bevy_ui2/src/update.rs

Davier · 2021-11-28T16:05:05Z

There is an issue with updating images (including text glyphs), like for sprites (see #3207). Whichever solution is chosen for sprite should also be applied for UI.

Davier · 2021-12-03T11:42:18Z

There is an issue with updating images (including text glyphs), like for sprites (see #3207). Whichever solution is chosen for sprite should also be applied for UI.

Fixed in a6f1581 (after rebasing on top of the fix for sprites). I chose to reuse SpriteAssetEvents, since the bevy_ui2 already depends on bevy_sprite2. Maybe that resource should be renamed to be more generic, or even placed in bevy_render2 or bevy_core_pipeline?

pipelined/bevy_ui2/src/ui_node.rs

TheRawMeatball · 2021-12-03T16:03:33Z

The last commit could likely be reverted, as in retrospect, I don't think the ControlNode s are a great idea, and were merged too hastily.

cart · 2021-12-09T03:41:49Z

I'm diving in to this now. Tip for reviewing: deleting the old bevy_ui and bevy_text and replacing them with the pipelined crates makes it much easier to review these changes because you can see the actual diffs.

mockersf · 2021-12-09T21:47:11Z

or in GitHub UI, there's is a dropdown when viewing file changes saying "changes from all commits" where you can select every commit but the first one (the url won't be valid if more commits are pushed)

cart · 2021-12-09T22:28:02Z

Just merged #3209. Lets resolve conflicts, make sure this plays nicely with the new clearing behavior, and address the remaining open comments. Then i think this is ready to go!

`ControlNode`s where added in bevyengine#2908 in main, while bevy_ui2 was being developped in the pipelined-rendering branch.

This reverts commit 3819bc5.

Davier · 2021-12-10T21:28:24Z

Unless there is more feedback on Visibility, everything is addressed. I only lightly tested the new modifications. I'll play a bit more with it tomorrow, but it already looks good to merge if you want to.

cart · 2021-12-10T21:57:26Z

Changes look good to me. I'm gonna think on the visibility situation for a bit, but I think this is probably good to merge!

cart · 2021-12-10T22:20:58Z

I'm fine with the choices you made regarding visibility. An empty VisibleEntities on the ui camera doesnt bother me and it leaves the doors open to future UI visibility work. The fact that UI elements dont have visibility / computed visibilty atm is also ok by me (for now).

cart · 2021-12-10T22:21:07Z

bors r+

# Objective Port bevy_ui to pipelined-rendering (see #2535 ) ## Solution I did some changes during the port: - [X] separate color from the texture asset (as suggested [here](https://discord.com/channels/691052431525675048/743663924229963868/874353914525413406)) - [X] ~give the vertex shader a per-instance buffer instead of per-vertex buffer~ (incompatible with batching) Remaining features to implement to reach parity with the old renderer: - [x] textures - [X] TextBundle I'd also like to add these features, but they need some design discussion: - [x] batching - [ ] separate opaque and transparent phases - [ ] multiple windows - [ ] texture atlases - [ ] (maybe) clipping

bors · 2021-12-10T22:46:23Z

Pull request successfully merged into main.

Build succeeded:

alice-i-cecile added A-Rendering Drawing game state to the screen A-UI Graphical user interfaces, styles, layouts, and widgets labels Aug 16, 2021

alice-i-cecile added this to the Bevy 0.6 milestone Aug 16, 2021

alice-i-cecile reviewed Aug 21, 2021

View reviewed changes

examples/README.md Outdated Show resolved Hide resolved

cart mentioned this pull request Sep 1, 2021

Renderer Rework: Initial Merge Tracking Issue #2535

Closed

64 tasks

alice-i-cecile added the S-Needs-Review label Sep 22, 2021

Davier force-pushed the ui_pipelined branch from 7d46fac to 6507ba9 Compare October 15, 2021 14:28

IceSentry mentioned this pull request Oct 15, 2021

[Merged by Bors] - Fix CI for android #2971

Closed

Davier force-pushed the ui_pipelined branch from 6507ba9 to 6e29a94 Compare October 16, 2021 12:38

Davier marked this pull request as ready for review October 16, 2021 13:49

Davier force-pushed the ui_pipelined branch from 415e8d3 to 1acf248 Compare November 24, 2021 14:37

cart changed the base branch from pipelined-rendering to main November 24, 2021 21:50

Davier force-pushed the ui_pipelined branch from 1acf248 to cc0d79c Compare November 25, 2021 20:25

colepoirier approved these changes Nov 27, 2021

View reviewed changes

Davier commented Nov 28, 2021

View reviewed changes

Davier force-pushed the ui_pipelined branch from 916c1ce to c8975e5 Compare December 3, 2021 11:36

TheRawMeatball reviewed Dec 3, 2021

View reviewed changes

pipelined/bevy_ui2/src/ui_node.rs Outdated Show resolved Hide resolved

Davier added 19 commits December 10, 2021 19:30

Copy bevy_ui and bevy_text in pipelined/

e465080

Remove old renderer

0fd6d5f

Render Text2d as sprites

a8239bf

Render UI and Text

a64d5ba

Derive Component and cleanup

9c13aa6

Reflect UI

5aec5f0

Reflect and register types

d1157cd

Fix doc test

27197dc

Update GpuImage when Image changes

d39a95e

Replace test_ui_z_system with its bevy_ui version

73e906d

Rename components with Ui prefix

fbf82b9

Make textures filterable

e542f7c

Re-add ControlNode for UI

1761185

`ControlNode`s where added in bevyengine#2908 in main, while bevy_ui2 was being developped in the pipelined-rendering branch.

Revert "Re-add ControlNode for UI"

8855148

This reverts commit 3819bc5.

Remove 2d camera in UI example

e8ab002

Remove direct dependency on wgpu

8c19df4

Rename ImageBindGroups to UiImageBindGroups

41ec574

Use DEFAULT_IMAGE_HANDLE in UiImage

67ae82e

Hide text nodes of size zero

5281c12

Davier force-pushed the ui_pipelined branch from 911952f to 5281c12 Compare December 10, 2021 20:59

bors bot changed the title ~~Port bevy_ui to pipelined-rendering~~ [Merged by Bors] - Port bevy_ui to pipelined-rendering Dec 10, 2021

bors bot closed this Dec 10, 2021

Davier deleted the ui_pipelined branch December 11, 2021 13:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Port bevy_ui to pipelined-rendering #2653

[Merged by Bors] - Port bevy_ui to pipelined-rendering #2653

Davier commented Aug 14, 2021 •

edited

Loading

cart commented Aug 14, 2021

alice-i-cecile commented Aug 16, 2021

cart commented Aug 18, 2021 •

edited

Loading

Davier commented Aug 21, 2021

cart commented Aug 24, 2021

Davier commented Oct 16, 2021

Davier commented Nov 24, 2021 •

edited

Loading

cart commented Nov 24, 2021

colepoirier left a comment

Davier left a comment

Davier Nov 28, 2021

Davier Nov 28, 2021

bjorn3 Nov 28, 2021

Davier Nov 28, 2021

Davier commented Nov 28, 2021

Davier commented Dec 3, 2021

TheRawMeatball commented Dec 3, 2021

cart commented Dec 9, 2021

mockersf commented Dec 9, 2021

cart commented Dec 9, 2021

Davier commented Dec 10, 2021

cart commented Dec 10, 2021 •

edited

Loading

cart commented Dec 10, 2021

cart commented Dec 10, 2021

bors bot commented Dec 10, 2021

		// FIXME there is no frustrum culling for UI
		pub visible_entities: VisibleEntities,

[Merged by Bors] - Port bevy_ui to pipelined-rendering #2653

[Merged by Bors] - Port bevy_ui to pipelined-rendering #2653

Conversation

Davier commented Aug 14, 2021 • edited Loading

Objective

Solution

cart commented Aug 14, 2021

alice-i-cecile commented Aug 16, 2021

cart commented Aug 18, 2021 • edited Loading

Davier commented Aug 21, 2021

cart commented Aug 24, 2021

Davier commented Oct 16, 2021

Davier commented Nov 24, 2021 • edited Loading

cart commented Nov 24, 2021

colepoirier left a comment

Choose a reason for hiding this comment

Davier left a comment

Choose a reason for hiding this comment

Davier Nov 28, 2021

Choose a reason for hiding this comment

Davier Nov 28, 2021

Choose a reason for hiding this comment

bjorn3 Nov 28, 2021

Choose a reason for hiding this comment

Davier Nov 28, 2021

Choose a reason for hiding this comment

Davier commented Nov 28, 2021

Davier commented Dec 3, 2021

TheRawMeatball commented Dec 3, 2021

cart commented Dec 9, 2021

mockersf commented Dec 9, 2021

cart commented Dec 9, 2021

Davier commented Dec 10, 2021

cart commented Dec 10, 2021 • edited Loading

cart commented Dec 10, 2021

cart commented Dec 10, 2021

bors bot commented Dec 10, 2021

Davier commented Aug 14, 2021 •

edited

Loading

cart commented Aug 18, 2021 •

edited

Loading

Davier commented Nov 24, 2021 •

edited

Loading

cart commented Dec 10, 2021 •

edited

Loading