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

Allow using no vertex buffers for render pipeline layouts #10307

Closed
wants to merge 1 commit into from

Conversation

JMS55
Copy link
Contributor

@JMS55 JMS55 commented Oct 29, 2023

Objective


Changelog

  • Specialized render pipelines will no longer crash on RenderPipelineDescriptors without any vertex buffer layouts.

@JMS55 JMS55 added the A-Rendering Drawing game state to the screen label Oct 29, 2023
@JMS55 JMS55 added this to the 0.13 milestone Oct 29, 2023
@JMS55 JMS55 requested review from nicopap and robtfm November 1, 2023 04:21
// Different MeshVertexBufferLayouts can produce the same final VertexBufferLayout
// We want compatible vertex buffer layouts to use the same pipelines, so we must "deduplicate" them
let vertex_key = descriptor.vertex.buffers.get(0).cloned();
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this clone have any negative performance consequences?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see a good way to avoid it, but it should be a fairly small clone?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To avoid the clone, I think we would have to wrap it in a Cow. Let me try that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't get it working, rip.

@JMS55
Copy link
Contributor Author

JMS55 commented Nov 5, 2023

Don't know if I'm going to go through with this PR, as the move to visibility buffers obviated the need for this.

@superdump
Copy link
Contributor

I think it’s a good change to make. Vertex buffers shouldn’t be required.

@JMS55
Copy link
Contributor Author

JMS55 commented Nov 11, 2023

Going to cancel this PR, as I think they should be required. I realized if you don't care about the vertex buffer layout part, you can just use the material pipeline specialization directly 😅.

@JMS55 JMS55 closed this Nov 11, 2023
@superdump
Copy link
Contributor

Again I disagree. Vertex buffers should not be required. You can render without any index or vertex buffer.

@JMS55
Copy link
Contributor Author

JMS55 commented Nov 11, 2023

Right, but what I realized is you don't have to go through SpecializedMeshPipelines in that case. You can just use MeshPipeline directly. Up to you, we can keep this if you want still.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants