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

Implement Veldrid vertex buffers #5595

Merged
merged 9 commits into from
Dec 19, 2022

Conversation

frenzibyte
Copy link
Member

Implementations mostly copied from GLVertexBuffer and siblings. Not too sure about VeldridIndexData, I've went with it just to have something working (in a similar manner to GLLinearIndexData), but it's completely up for refactor.

@@ -19,6 +19,7 @@ public class VertexMemberAttribute : Attribute
/// The type of each component of this vertex attribute member.
/// E.g. a <see cref="osuTK.Vector2"/> is represented by 2 **<see cref="VertexAttribPointerType.Float"/>** components.
/// </summary>
// todo: this should be replaced by an enum defined in o!f.
Copy link
Member Author

Choose a reason for hiding this comment

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

As per above, we shouldn't rely on using osuTK enums for o!f rendering code anymore, but seeing that changing the enum would be a breaking change, I've left a TODO for now.

@frenzibyte frenzibyte mentioned this pull request Dec 19, 2022
1 task
Comment on lines +297 to +301
if (!pipelineCache.TryGetValue(pipeline, out var instance))
{
pipelineCache[pipeline] = instance = Factory.CreateGraphicsPipeline(ref pipeline);
stat_graphics_pipeline_created.Value++;
}
Copy link
Contributor

@smoogipoo smoogipoo Dec 19, 2022

Choose a reason for hiding this comment

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

This is kinda scary. So we need a new graphics pipeline for every set of shaders/textures/framebuffers/etc, because they're immutable? The construction of these pipelines does not seem trivial.

It may be something that we'll need to have a dynamic cache for to expire older items, because I can foresee this cache getting quite large.

I suppose the cost is fine in the context that this is effectively a context switch, now at a CPU level just as much as it is at a GPU level.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not 100% sure about my statistics, but checking the "total pipelines created" one on o!f visual tests it interestingly doesn't get beyond 3 different pipeline states. So I felt it's sufficient now just for the time being.

@smoogipoo smoogipoo merged commit 574b6e8 into ppy:master Dec 19, 2022
@frenzibyte frenzibyte deleted the veldrid-renderer/vertices branch December 19, 2022 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants