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
Spir-V pipeline improvements #7659
Conversation
@@ -290,7 +291,6 @@ static public SPIRVCompileResult compileGLSLToSPIRV(String shaderSource, ES2ToES | |||
// Set (1) |- Binding(0) |_ U4 | |||
// |_ U5 | |||
// | |||
HashMap<Integer,SetEntry> setBindingMap = new HashMap<Integer,SetEntry>(); |
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.
This is not needed any longer since we run the optimization pass. (I should remove the comment above as well)
if (!material) | ||
{ | ||
dmResource::Release(params.m_Factory, (void*)resources.m_VertexProgram); | ||
dmResource::Release(params.m_Factory, (void*)resources.m_FragmentProgram); | ||
return dmResource::RESULT_DDF_ERROR; | ||
} |
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.
Avoids a hard crash when the program can't be created
required uint64 name_hash = 2; | ||
required ShaderDataType type = 3; | ||
optional uint32 element_count = 4 [default=1]; | ||
optional uint32 set = 5 [default=0]; | ||
optional uint32 binding = 6 [default=0]; | ||
} | ||
|
||
message Shader | ||
{ | ||
required Language language = 1; | ||
optional bytes source = 2; | ||
repeated ResourceBinding uniforms = 4; | ||
repeated ResourceBinding attributes = 5; | ||
optional bool variant_texture_array = 6 [default = false]; | ||
repeated ResourceBinding inputs = 5; | ||
repeated ResourceBinding outputs = 6; | ||
optional bool variant_texture_array = 7 [default = false]; |
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.
Changed the order a bit here, but it's build/engine only so should be fine. Editor uses bob for building so no changes should be needed for editor either
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.
Looks good.
Although if possible, I'd like to move the input/output check to the pipeline.
if (ddf->m_Outputs.m_Count > 0) | ||
{ | ||
shader->m_Outputs = new ShaderResourceBinding[ddf->m_Outputs.m_Count]; | ||
shader->m_OutputCount = ddf->m_Outputs.m_Count; | ||
|
||
for (uint32_t i=0; i < ddf->m_Outputs.m_Count; i++) | ||
{ | ||
ShaderResourceBinding& res = shader->m_Outputs[i]; | ||
res.m_Binding = ddf->m_Outputs[i].m_Binding; | ||
res.m_Set = ddf->m_Outputs[i].m_Set; | ||
res.m_Type = ddf->m_Outputs[i].m_Type; | ||
res.m_NameHash = ddf->m_Outputs[i].m_NameHash; | ||
res.m_Name = strdup(ddf->m_Outputs[i].m_Name); |
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.
Not sure if it's worth it but these two segments could be put into a function. it's an edge case I think.
if (!input_found) | ||
{ | ||
dmLogError("Input of fragment shader '%s' not written by vertex shader", fragment_module->m_Inputs[i].m_Name); | ||
return false; | ||
} |
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 feel like this check should be in the shader build pipeline?
At runtime we don't change the vertex/fragment programs, only the whole material. So this situation shouldn't come up at runtime?
@@ -2250,11 +2278,11 @@ namespace dmGraphics | |||
HashState64 program_hash; | |||
dmHashInit64(&program_hash, false); | |||
|
|||
if (vertex_module->m_AttributeCount > 0) | |||
if (vertex_module->m_InputCount > 0) |
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.
This if-statement is redundant. The for-loop does the job.
Improved the SPIR-V shader pipeline:
Fixes #5674
PR checklist