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

use custom ser/de impl for TextureFormat #2908

Merged
merged 13 commits into from
Sep 29, 2022
Merged

use custom ser/de impl for TextureFormat #2908

merged 13 commits into from
Sep 29, 2022

Conversation

crowlKats
Copy link
Collaborator

No description provided.

crowlKats
crowlKats commented Jul 22, 2022
View reviewed changes
wgpu-types/src/lib.rs Outdated Show resolved Hide resolved
wgpu-types/src/lib.rs Show resolved Hide resolved
@cwfitzgerald
Copy link
Member

What's the status of this, I know there was a bunch of discussion about it.

jimblandy
jimblandy requested changes Sep 2, 2022
View reviewed changes
Copy link
Member

@jimblandy jimblandy left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This looks great - thank you for taking care of it!

I think all we need here are some tests. Please add two #[test] functions to wgpu-types/src/lib.rs, just after the the impl TextureFormat block, that compare the serializations and deserializations of wgpu_types::TextureFormat against the strings given in the WebGPU spec's definition of GPUTextureFormat.

@crowlKats
Copy link
Collaborator Author

ok, will do. will wait for #2954 to land first.
Honestly, i am not too happy about this PR as it feels extremely hacky, but then again, unsure how else this could be achieved

a1phyr
a1phyr suggested changes Sep 21, 2022
View reviewed changes
wgpu-types/src/lib.rs Outdated Show resolved Hide resolved
wgpu-types/src/lib.rs Outdated Show resolved Hide resolved
wgpu-types/src/lib.rs Outdated Show resolved Hide resolved
wgpu-types/src/lib.rs Outdated Show resolved Hide resolved
cwfitzgerald
cwfitzgerald approved these changes Sep 24, 2022
View reviewed changes
Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

Once CI isn't mad, G2G

@cwfitzgerald cwfitzgerald enabled auto-merge (squash) September 24, 2022 04:37
@crowlKats crowlKats requested review from a1phyr and jimblandy and removed request for a1phyr September 25, 2022 01:33
jimblandy
jimblandy approved these changes Sep 25, 2022
View reviewed changes
Copy link
Member

@jimblandy jimblandy left a comment

Choose a reason for hiding this comment

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

Thanks!

@crowlKats
Copy link
Collaborator Author

I am unsure whats going on with the errors

@jimblandy
Copy link
Member

Perhaps try updating your clippy?

@crowlKats
Copy link
Collaborator Author

crowlKats commented Sep 26, 2022 •
edited
Loading

@jimblandy clippy should be fixed now, but i was rather asking about the windows & linux tests that are failing

@cwfitzgerald
Copy link
Member

Has the ron serialization of traces changed? It's a parsing failure of the .ron traces in the test.

@a1phyr
Copy link
Contributor

a1phyr commented Sep 26, 2022

Has the ron serialization of traces changed?

Yes, texture formats were previously (de)serialized as enum variants and they are now (de)serialized as strings.

a1phyr
a1phyr approved these changes Sep 29, 2022
View reviewed changes
Copy link
Contributor

@a1phyr a1phyr left a comment

Choose a reason for hiding this comment

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

LGTM !

@cwfitzgerald cwfitzgerald merged commit 26f2239 into gfx-rs:master Sep 29, 2022
@crowlKats crowlKats deleted the serde_astc branch September 30, 2022 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@a1phyr a1phyr a1phyr approved these changes

@cwfitzgerald cwfitzgerald cwfitzgerald approved these changes

@jimblandy jimblandy jimblandy 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.
4 participants
@crowlKats @cwfitzgerald @jimblandy @a1phyr