Skip to content
This repository has been archived by the owner on Jun 28, 2024. It is now read-only.

Synchronisation simplification/overhaul based on vk-sync-rs #6

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

fu5ha
Copy link

@fu5ha fu5ha commented Sep 16, 2021

I think this fits with the ethos of sierra in simplifying Vulkan concepts while still remaining as flexible/usable as possible. We've been using similar sort of sync primitives at Embark for a little while now and it's been working quite well and feels nice. I know this is a pretty large change so feel free to give feedback or just say if it doesn't fit in your vision for the lib.

@zakarumych
Copy link
Collaborator

Thanks for taking time making this.

This looks interesting. I'll find time to review it thoroughly asap.

Copy link
Collaborator

@zakarumych zakarumych left a comment

Choose a reason for hiding this comment

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

Looks great!

But I feel we can do even better!

VertexShaderReadUniformBuffer,

/// Read as a sampled image/uniform texel buffer in a vertex shader
VertexShaderReadSampledImageOrUniformTexelBuffer,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are those mixed?


/// Defines all potential resource usages
#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
pub enum Access {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this enumeration contain all valid combinations of access type and stage?
If not, could it become a problem?

pub new_layout: Layout,
pub prev_access: Access,
pub next_access: Access,
pub old_layout: Option<SyncLayout>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here None would mean UNDEFINED. i.e. the Manual variant

pub old_layout: Option<Layout>,
pub new_access: AccessFlags,
pub new_layout: Layout,
pub prev_access: Access,
Copy link
Collaborator

Choose a reason for hiding this comment

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

What if multiple read accesses were performed before?

pub new_access: AccessFlags,
pub new_layout: Layout,
pub prev_access: Access,
pub next_access: Access,
Copy link
Collaborator

Choose a reason for hiding this comment

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

What if multiple read accesses are planned?

pub prev_access: Access,
pub next_access: Access,
pub old_layout: Option<SyncLayout>,
pub new_layout: SyncLayout,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of using SyncLayout we could offer calculation of optimal Layout in methods that build this type.

#[derive(Clone, Copy, Debug, Hash, PartialEq, Eq)]
pub struct BufferMemoryBarrier<'a> {
pub buffer: &'a Buffer,
pub offset: u64,
pub size: u64,
pub old_access: AccessFlags,
pub new_access: AccessFlags,
pub prev_access: Access,
Copy link
Collaborator

Choose a reason for hiding this comment

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

What if multiple read accesses were performed before?

pub old_access: AccessFlags,
pub new_access: AccessFlags,
pub prev_access: Access,
pub next_access: Access,
Copy link
Collaborator

Choose a reason for hiding this comment

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

What if multiple read accesses are planned?

@zakarumych
Copy link
Collaborator

It would be blast if both old flags-based and new approach would be available for the user.

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.

None yet

2 participants