-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
[Impeller] Vulkan framebuffer fetch via VK_ARM_RASTERIZATION_ORDER_ATTACHMENT_ACCESS #48458
Changes from 18 commits
9180bfb
a9fd144
6921c1b
5c395cb
b6cb2cb
1869497
b428f5f
808d257
4e79b47
a1153d9
76c4336
d5bff60
cef3581
371fbdd
55d111d
f5949b8
8dfb942
da822f4
4501eb3
7bf4115
b9ef69b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,9 @@ | |
#include <impeller/conversions.glsl> | ||
#include <impeller/types.glsl> | ||
|
||
// Warning: if any of the constant values or layouts are changed in this | ||
// file, then the hard-coded constant value in | ||
// impeller/renderer/backend/vulkan/binding_helpers_vk.cc | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thinking out aloud on how to address this issue (in a separate patch later of course): We should be able to import a single header file from both a C++ TU and a glsl header. The same macro expansion capabilities as the C pre-processor are available in impellerc as well. As the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe the default impellerc shaders GN rule set also add the header include paths during the impellerc invocation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a nit; But I am just nervous about "please keep these in sync manually" type comments. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that won't work in this case, because its not that we're setting to binding 64 or w/e, that is just the first available binding in the fragment stage. So adding a new binding to the vertex stage might also change that? Not sure. What I would really like is to have separate GLLS, Vulkan, and Metal headers. As this would also solve the problem of conditionally using float16. |
||
uniform FrameInfo { | ||
mat4 mvp; | ||
float src_y_coord_scale; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its a bit of a mistake to assume that all bindings are in the set index zero. We should fix that separately as that affects all instances of
DescriptorSetLayout
in the generated header. For example, if we were to manually specifylayout(set=2, binding=n)
, we'd keel over.In the same vein, I think its a mistake to assume that we can only have one input attachment and at index zero. I'd also put the input_attachment_index in the descriptor set layout. The previous attempt at this patch did the same.
You should be able to reproduce this by manually specifying the input_attachment_index to anything other than zero and checking the validity of the setup.
For the descriptor set index, we can file a separate followup issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think practically speaking we can't really do bindings any other way. The actual render_pass_vk logic can only handle descriptors at index set zero.
Perhaps it would be possible with some refactoring, but it seems really in the weeds of what will yield a performance improvement.