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.

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

Move clear_texture impl from wgpu-hal to wgpu-core #2046

Merged
merged 8 commits into from
Oct 13, 2021

Conversation

Wumpf
Copy link
Member

@Wumpf Wumpf commented Oct 9, 2021

Connections
Another small steps towards #1688 (texture zero init)

Description
Moves clear_texture implementation from wgpu-hal to wgpu-core.
Previously, the only backend profiting from this was Vulkan and it still limited what we could use it on, notably compressed textures did not work. The new impl is almost identical to the Metal & DX12 implementation previously -> less code to maintain.

collect_zero_buffer_copies_for_clear_texture may look awkwardly separated out, but is very useful in this form for my texture init wip branch where I can use this to bundle up operations that are then sent off a single wgpu-hal call

Testing
New test going through all texture formats and kinds and clearing them. No reading back yet, but verifies at least that backend impls are happy and nothing crashes. List of formats put in there should be useful for other upcoming "try this with all formats" tests.

@Wumpf Wumpf force-pushed the core-cleartexture branch from 7267512 to a28319b Compare October 11, 2021 18:51
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.

Looks great! Just a few notes

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.

Needs to be addressed before we merge

@Wumpf Wumpf requested a review from kvark October 12, 2021 19:14
@Wumpf
Copy link
Member Author

Wumpf commented Oct 12, 2021

@kvark good to go now I believe

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.

I'm sorry, it looks like one of my comments didn't show up, and still needs to be addressed.
bytes_per_row has to be aligned. See

let bytes_per_row_alignment = get_lowest_common_denom(

@Wumpf
Copy link
Member Author

Wumpf commented Oct 13, 2021

ohh another good catch. Makes me very happy we have to implement this monster only once now. Fixed!

@Wumpf Wumpf requested a review from kvark October 13, 2021 07:31
@kvark
Copy link
Member

kvark commented Oct 13, 2021

CI is unhappy. Otherwise, good to go!

@kvark kvark merged commit a28c528 into gfx-rs:master Oct 13, 2021
@Wumpf Wumpf deleted the core-cleartexture branch October 13, 2021 17:42
@Wumpf Wumpf mentioned this pull request Oct 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants