Skip to content

[Merged by Bors] - update wireframe rendering to new renderer#3193

Closed
jakobhellermann wants to merge 2 commits intobevyengine:mainfrom
jakobhellermann:wireframes
Closed

[Merged by Bors] - update wireframe rendering to new renderer#3193
jakobhellermann wants to merge 2 commits intobevyengine:mainfrom
jakobhellermann:wireframes

Conversation

@jakobhellermann
Copy link
Copy Markdown
Contributor

@jakobhellermann jakobhellermann commented Nov 26, 2021

Updates the wireframe rendering initialliy implemented in #562 to the new renderer.
It lives in bevy_pbr2 instead of bevy_render2 because that way it can reuse the MeshPipeline.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Nov 26, 2021
jakobhellermann
jakobhellermann commented Nov 26, 2021
View reviewed changes
pipelined/bevy_pbr2/src/wireframe.rs
descriptor.vertex.shader = self.shader.clone_weak();
descriptor.fragment.as_mut().unwrap().shader = self.shader.clone_weak();
descriptor.primitive.polygon_mode = PolygonMode::Line;
descriptor.depth_stencil.as_mut().unwrap().bias.slope_scale = 1.0;
Copy link
Copy Markdown
Contributor Author

@jakobhellermann jakobhellermann Nov 26, 2021

Choose a reason for hiding this comment

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

I have no idea if 1.0 is a good value here but it looks way better than without.

grafik
grafik

@jakobhellermann
Copy link
Copy Markdown
Contributor Author

jakobhellermann commented Nov 26, 2021

This isn't actually usable right now because the POLYGON_MODE_LINE feature needs to be enabled, but it works once the wgpu feature selection is ported as well.

@jakobhellermann jakobhellermann added A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible S-Needs-Review and removed S-Needs-Triage This issue needs to be labelled labels Nov 26, 2021
@cart cart added this to the Bevy 0.6 milestone Nov 29, 2021
@cart cart mentioned this pull request Nov 29, 2021
Closed
64 tasks
@PixelDust22
Copy link
Copy Markdown
Contributor

PixelDust22 commented Dec 2, 2021

I would recommend having wireframe in bevy_render2 instead of bevy_pbr2 because one might want to use bevy_render2 without pbr. Creating a new pipeline instead of reusing MeshPipeline in bevy_pbr2 would be a better option imo.

@folke folke mentioned this pull request Dec 9, 2021
Closed
bors bot pushed a commit that referenced this pull request Dec 9, 2021
@folke
Ported WgpuOptions to bevy_render2 (#3282)
4c91613 
# Objective

The new renderer does not support any options yet for wgpu. These are needed for example for rendering wireframes (see #3193).

## Solution

I've ported WgpuOptions to bevy_render2.

The defaults match the defaults that were used before this PR (meaning, some specific options when target_arch = wasm32).

Additionally, I removed `Auto` from WgpuBackends and added `Primary`. The default will use primary or GL based on the target_arch.
@mockersf
Copy link
Copy Markdown
Member

mockersf commented Dec 10, 2021

@jakobhellermann should this be updated now that #3282 has been merged, exposing WgpuOptions?

@jakobhellermann
Copy link
Copy Markdown
Contributor Author

jakobhellermann commented Dec 10, 2021

@jakobhellermann should this be updated now that #3282 has been merged, exposing WgpuOptions?

The PR is unblocked in that regard, but I'm not sure it can reasonably do anything with the WgpuOptions. Since the WireframePlugin is added after the RenderPlugin it cannot affect the creation of the renderer features.

The old implementation doesn't solve this and requires the user to enable the POLYGON_MODE_LINE feature themselves.

@jakobhellermann
Copy link
Copy Markdown
Contributor Author

jakobhellermann commented Dec 10, 2021

The other remaining issue is whether this should live in render2 or pbr2. I agree with @Neo-Zhixing that render2 is the better location, but to do that properly we need something like #3120 because otherwise we don't know where in the vertex buffer the position attribute is.

@cart
Copy link
Copy Markdown
Member

cart commented Dec 10, 2021

Yeah I agree that render2 is probably the best place for it and #3120 might help. I see no real harm in merging as-is though. We can try to move it later after we sort out the vertex attribute situation.

@cart
Copy link
Copy Markdown
Member

cart commented Dec 10, 2021

bors r+

bors bot pushed a commit that referenced this pull request Dec 10, 2021
@jakobhellermann
update wireframe rendering to new renderer (#3193)
58474d7 
Updates the wireframe rendering initialliy implemented in #562 to the new renderer.
It lives in `bevy_pbr2` instead of `bevy_render2` because that way it can reuse the `MeshPipeline`.
@bors
Copy link
Copy Markdown
Contributor

bors bot commented Dec 10, 2021

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title update wireframe rendering to new renderer [Merged by Bors] - update wireframe rendering to new renderer Dec 10, 2021
@bors bors bot closed this Dec 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible

Projects

None yet

Milestone

Bevy 0.6

Development

Successfully merging this pull request may close these issues.

4 participants

@jakobhellermann @PixelDust22 @mockersf @cart