Skip to content
This repository was archived by the owner on Jun 18, 2021. It is now read-only.

expose wgt serde feature#285

Closed
gzp79 wants to merge 1 commit intogfx-rs:masterfrom
gzp79:gzp
Closed

expose wgt serde feature#285
gzp79 wants to merge 1 commit intogfx-rs:masterfrom
gzp79:gzp

Conversation

@gzp79
Copy link
Copy Markdown
Contributor

@gzp79 gzp79 commented Apr 27, 2020

#284
Add serde feature (wgt/serde)

Copy link
Copy Markdown
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.

This looks fine in principle. The problem is having one more feature to test with.
Let's see if #284 discussion leads to this as the only solution.

default = []
# Make Vulkan backend available on platforms where it is by default not, e.g. macOS
vulkan = ["wgc/gfx-backend-vulkan"]
serde = ["wgt/serde"]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are there any types you'd like to serialize that are in wgpu-rs as opposed to wgpu-types? Having serde feature would conflict with depending on servo in this case.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

also, this would probably need to include "wgc/serde" as well

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@kvark assuming you meant serde instead of servo, I think we can do have both serde as a feature and a dependency using the same strategy we use in wgpu-types (rust-lang/api-guidelines#180 (comment))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fortunately, I don't think this is going to be a problem, see gfx-rs/wgpu#619

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Are there any types you'd like to serialize that are in wgpu-rs as opposed to wgpu-types? Having serde feature would conflict with depending on servo in this case.

In my usecase I have a material description file that is very close to the pipeline. Like required primitive_topology, some settings for the color_stage, vertex attribute that can be turned into
RenderPipeline. Without the serde feature I'd have to duplicate all these types and write some wrappers and/or From implementations.

To be more specific:
geometry read from file (ex FBX, obj etc) has a vertex format desc, like: Position(Float3, offset), Normal(Float3, offset), etc.
Material(pipeline) has shader descriptions, binding info, uniforms, etc.

In the "engine" there is a map from (material + vertex_format) -> RenderPipeline. During the population of this map the pipeline requirements and vertex formats are matched and thus the same geometry can be used for multiple pipeline. And the connection (binding) b/n the pipeline and geometry is mostly automatic:

let m = materials::<VertexPos2Normal3>::create("Z-pass").
let command_buffer = m.render(encoder, geometry_with_pos2_norm3);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I've wanted to (de)serialize BindGroupLayoutEntry.
Although I wonder why it isn't part of wgpu-types in the first place.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we may ask @grovesNL . In general, only those types go into wgpu-types that can be re-used directly for the Web target.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

wgpu-rs defines its own BindGroupLayoutEntry, so we shouldn't need to share the BindGroupLayoutEntry from wgpu-core

@kvark kvark mentioned this pull request Apr 30, 2020
@bors bors bot closed this in 449924b May 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants