Skip to content

add polygon_mode: PolyonMode to RasterizationStateDescriptor to allow drawing wireframes #921

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
merged 1 commit into from
Sep 9, 2020
Merged

add polygon_mode: PolyonMode to RasterizationStateDescriptor to allow drawing wireframes #921

merged 1 commit into from
Sep 9, 2020

Conversation

manuel-woelker
Copy link
Contributor

Description
This adds polygon_mode: PolyonMode to RasterizationStateDescriptor to allow drawing in wireframes and point mode

I added hal::Features::NON_FILL_POLYGON_MODE to the wishful_features in instance.rs. I don't know what side effects this might have.

Testing
Tested locally on my Windows machine with Vulkan backend.

monocodus[bot]
monocodus bot suggested changes Sep 5, 2020
View reviewed changes
Copy link
Contributor

@monocodus monocodus bot left a comment

Choose a reason for hiding this comment

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

This is an autogenerated code review.

Checker summary (by rust_clippy):
The tool has found 0 warnings, 1 errors.

@kvark
Copy link
Member

kvark commented Sep 5, 2020

Thank you for the PR!

This capability is not a part of upstream API: https://gpuweb.github.io/gpuweb/
See the investigation in gpuweb/gpuweb#137

It means that we can only expose it as a native-only extension.

@manuel-woelker
Copy link
Contributor Author

Would this mean integrating it with the extension mechanism implemented in #690?

Is there anything else I should be aware of?

@kvark
Copy link
Member

kvark commented Sep 8, 2020

You'd add a bit to

wgpu/wgpu-types/src/lib.rs

Line 148 in 7303131

pub struct Features: u64 {
within ALL_NATIVE range.

@kvark
Copy link
Member

kvark commented Sep 8, 2020

And you'd only expose it if gfx-hal exposes NON_FILL_POLYGON_MODE for this adapter

@manuel-woelker
Copy link
Contributor Author

Thanks for the info, I'll give it a shot!

monocodus[bot]
monocodus bot suggested changes Sep 9, 2020
View reviewed changes
Copy link
Contributor

@monocodus monocodus bot left a comment

Choose a reason for hiding this comment

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

This is an autogenerated code review.

Checker summary (by rust_clippy):
The tool has found 0 warnings, 1 errors.

kvark
kvark approved these changes Sep 9, 2020
View reviewed changes
Copy link
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

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

Nice!
Could you squash it please?

@manuel-woelker
Copy link
Contributor Author

I added the feature flag, trying to mirror the other flags.

I pushed a branch of wgpu-rs that modifies the cube example to test line fill mode here:
https://github.com/manuel-woelker/wgpu-rs/blob/fb-polygon-mode/examples/cube/main.rs

Let me know if I missed anything!

I see the build is failing due to the new LabeledContextError, I'll rebase and squash.

@manuel-woelker manuel-woelker force-pushed the fb-polygon-mode branch 2 times, most recently from e1a7357 to 0c24eb5 Compare September 9, 2020 19:16
cwfitzgerald
cwfitzgerald reviewed Sep 9, 2020
View reviewed changes
wgpu-types/src/lib.rs
@@ -611,6 +641,7 @@ impl Default for CullMode {
pub struct RasterizationStateDescriptor {
pub front_face: FrontFace,
pub cull_mode: CullMode,
pub polygon_mode: PolygonMode,
Copy link
Member

Choose a reason for hiding this comment

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

This member should have a comment on it saying it can only be non-fill with the feature on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I added the docs.

kvark
kvark approved these changes Sep 9, 2020
View reviewed changes
Copy link
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

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

Thank you!
bors r+

@bors
Copy link
Contributor

bors bot commented Sep 9, 2020

Build succeeded:

@bors bors bot merged commit 2972212 into gfx-rs:master Sep 9, 2020
@manuel-woelker
Copy link
Contributor Author

Thanks for the great and "realtime" feedback!

@kvark
Copy link
Member

kvark commented Sep 9, 2020

Please join us for even more real time feedback on "#wgpu:matrix.org"!

@kvark kvark mentioned this pull request Sep 11, 2020
Merged
bors bot added a commit to gfx-rs/wgpu-rs that referenced this pull request Sep 11, 2020
@bors @kvark
Merge #559
e798278 
559: Update wgpu with the polygon modes support, use in the cube example r=cwfitzgerald a=kvark

Uses gfx-rs/wgpu#921, cc @manuel-woelker

Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
@PixelDust22 PixelDust22 mentioned this pull request Sep 21, 2020
Merged
@kvark kvark mentioned this pull request Nov 6, 2020
Merged
kvark added a commit to kvark/wgpu that referenced this pull request Jun 3, 2021
@bors @kvark
[rs] Merge gfx-rs#559
70732bb 
559: Update wgpu with the polygon modes support, use in the cube example r=cwfitzgerald a=kvark

Uses gfx-rs#921, cc @manuel-woelker

Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
kvark added a commit to kvark/wgpu that referenced this pull request Jun 3, 2021
@bors @kvark
[rs] Merge gfx-rs#921
6e680d9 
921: Update wgpu to eadaa1b r=kvark a=kvark

Picks up gfx-rs#1419, gfx-rs#1417, and gfx-rs#1403

Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cwfitzgerald cwfitzgerald cwfitzgerald left review comments

@monocodus monocodus[bot] monocodus[bot] requested changes

@kvark kvark kvark approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@manuel-woelker @kvark @cwfitzgerald