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

Add support for bgra8unorm-storage #4228

Merged
merged 13 commits into from
Oct 13, 2023
Merged

Conversation

nical
Copy link
Contributor

@nical nical commented Oct 10, 2023

Checklist

  • Run cargo clippy.
  • Add change to CHANGELOG.md. See simple instructions inside file.

Connections

This is a continuation of the work that started in #3634

Bugzilla: https://bugzilla.mozilla.org/show_bug.cgi?id=1829895

Description

Resolves #3359

Testing

There is a basic test in here.
The CTS also has some coverage for this although firefox hasn't updated to a version of the CTS that contains the test (but will soon).

@nical nical requested a review from a team October 10, 2023 15:30
@nical nical requested a review from a team as a code owner October 10, 2023 15:30
@nical nical marked this pull request as draft October 10, 2023 15:30
@nical
Copy link
Contributor Author

nical commented Oct 10, 2023

Currently blocked on gfx-rs/naga#2550

@nical nical force-pushed the bgra8unorm-storage branch 3 times, most recently from b69ce5d to 33b8227 Compare October 10, 2023 16:38
@nical nical force-pushed the bgra8unorm-storage branch 2 times, most recently from 38ea158 to 392b127 Compare October 11, 2023 11:21
@nical nical marked this pull request as ready for review October 11, 2023 11:22
wgpu-core/src/device/resource.rs Outdated Show resolved Hide resolved
wgpu-hal/src/vulkan/adapter.rs Outdated Show resolved Hide resolved
wgpu-hal/src/vulkan/adapter.rs Outdated Show resolved Hide resolved
tests/tests/bgra8unorm_storage.rs Show resolved Hide resolved
wgpu-hal/src/vulkan/adapter.rs Outdated Show resolved Hide resolved
Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

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

A few remaining things.

wgpu-types/src/lib.rs Outdated Show resolved Hide resolved
player/tests/data/buffer-copy.ron Outdated Show resolved Hide resolved
player/tests/data/clear-buffer-texture.ron Outdated Show resolved Hide resolved
player/tests/data/zero-init-buffer.ron Outdated Show resolved Hide resolved
@nical nical force-pushed the bgra8unorm-storage branch from e92e6c0 to 0386572 Compare October 12, 2023 12:11
Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

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

Looks good!

@nical nical enabled auto-merge (squash) October 12, 2023 12:55
@nical nical force-pushed the bgra8unorm-storage branch from 0386572 to 981dc24 Compare October 12, 2023 13:02
@nical nical force-pushed the bgra8unorm-storage branch from 40dd6c0 to bc3cdec Compare October 13, 2023 09:24
@teoxoy
Copy link
Member

teoxoy commented Oct 13, 2023

@nical the feature is missing from

was brought up by @crowlKats in #3634 (comment)

@teoxoy teoxoy disabled auto-merge October 13, 2023 09:34
@teoxoy
Copy link
Member

teoxoy commented Oct 13, 2023

I thought changes to deno require approval from @gfx-rs/deno but it seems my review is enough to land this. I remember this wasn't the case previously to adding the wgpu team to the codeowners file (#4162).

cc @cwfitzgerald

@crowlKats
Copy link
Collaborator

good catch, apologies for not having reviewed this.
Also, I believe previously PRs could be merged without my approval, its just that I was more attentive and actually left approvals or required changes relatively quickly

@nical
Copy link
Contributor Author

nical commented Oct 13, 2023

Alright, this should be ready to land (note: I disabled the feature with vulkan on mac, because of the odd side effect causing the occlusion_query test to fail, I can't spend the time investigating that in the foreseeable future).

@teoxoy
Copy link
Member

teoxoy commented Oct 13, 2023

I reran the mac job that was failing https://github.com/gfx-rs/wgpu/actions/runs/6496021322/job/17672724374 and it ran successfully. I think it was just flakey (#4235 (comment)).

@nical
Copy link
Contributor Author

nical commented Oct 13, 2023

Ok let's try one last time

@nical nical force-pushed the bgra8unorm-storage branch from ca0760a to d7bab55 Compare October 13, 2023 09:57
@nical
Copy link
Contributor Author

nical commented Oct 13, 2023

Ok it passed, let's merge.

@nical nical enabled auto-merge (squash) October 13, 2023 10:03
@nical nical merged commit ff306d2 into gfx-rs:trunk Oct 13, 2023
23 checks passed
@nical nical deleted the bgra8unorm-storage branch October 13, 2023 10:24
@cwfitzgerald
Copy link
Member

I thought changes to deno require approval from https://github.com/orgs/gfx-rs/teams/deno but it seems my review is enough to land this. I remember this wasn't the case previously to adding the wgpu team to the codeowners file

@teoxoy wgpu has never required any reviews at all to land things, just CI passing - this is a policy I've kept to allow good-faith quick fixes without needing the delay of getting other people involved.

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.

Support bgra8unorm-storage extension
5 participants