Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 2 commits into from
Closed

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

wants to merge 2 commits into from

Conversation

jakobhellermann
Copy link
Contributor

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
Contributor Author

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
Contributor Author

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
@Neo-Zhixing
Copy link
Contributor

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
Member

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

@jakobhellermann
Copy link
Contributor Author

@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
Contributor Author

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
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
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
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 @Neo-Zhixing @mockersf @cart