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

Dynamic rendering #92

Merged
merged 21 commits into from
Apr 12, 2024
Merged

Dynamic rendering #92

merged 21 commits into from
Apr 12, 2024

Conversation

johannesugb
Copy link
Member

Opened a preliminary pull request for reviewing

include/avk/attachment.hpp Outdated Show resolved Hide resolved
include/avk/graphics_pipeline.hpp Outdated Show resolved Hide resolved
src/avk.cpp Outdated Show resolved Hide resolved
src/avk.cpp Outdated Show resolved Hide resolved
src/avk.cpp Outdated Show resolved Hide resolved
Changing local variables naming to lowerCamelCase

Co-authored-by: Johannes Unterguggenberger <[email protected]>
// which would denote how to use the attachment - use this attachment as stencil, depth or color
if(is_depth_format(currAttachment.format()))
{
if(depthAttachment.has_value())
Copy link
Collaborator

Choose a reason for hiding this comment

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

One more issue I wanted to discuss. There is no way (at least if I understood the code correctly) to have separate depth and stencil attachments.

For example suppose I have two D24S8 attachments and I'd like to use the first one as depth and the second one as stencil. Because the stencil/depth attachment state is directly deduced from the format of the attachment and both of them have combined format there is no way to know which one to use as which.

I was thinking of adding an optional flag that would mark the attachment as being either depth only or stencil only, do you agree with that?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, more options are always a good thing. Please proceed as you have planned!

Copy link
Collaborator

Choose a reason for hiding this comment

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

This would get resolved by the change I propose in this comment. I could use the dynamic_usage of each attachment to alleviate this issue.

- removed dynamic_rendering_attachment
- added declare_dynamic(_for) functions to avk::attachment which create dynamic rendering attachments
- changed graphics_pipeline_t::renderpass_pointer() back to renderpass_reference() which now returns a reference wrapper
Copy link
Member Author

@johannesugb johannesugb left a comment

Choose a reason for hiding this comment

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

Looks very good! :)

Please make sure to add MSAA resolve, and please look into multiview (and add it if it can be added easily---don't if it turns out to be a very complicated endeavour)

include/avk/attachment.hpp Outdated Show resolved Hide resolved
include/avk/attachment.hpp Outdated Show resolved Hide resolved
include/avk/attachment.hpp Outdated Show resolved Hide resolved
include/avk/graphics_pipeline.hpp Outdated Show resolved Hide resolved
@MatejSakmary
Copy link
Collaborator

So I added an initial implementation of MSAA, however I am not very happy with it as is. I tried to repurpose subpass_usage for this, however I believe it should be it's own configure structure. Subpass usage(s) have a bunch of other stuff that are not relevant at all to what I am trying to use it for.

Instead I would propose something like "dynamic_usage" which would store usage flag (ie. color, depth, stencil + resolve information). What is your opinion on this? I feel (again) that the usage of subpasses might be misleading, however you might like it as it is in the spirit of what we ended up with in regards to the attachments.

Lastly I have a question, why does subpass_usage not have color resolve mode? It only has depth or stencil ones.

@johannesugb johannesugb merged commit 2e437e0 into development Apr 12, 2024
17 checks passed
@johannesugb johannesugb deleted the dynamic_rendering branch April 12, 2024 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants