common: Implement ShaderLocation#21
Conversation
german77
left a comment
There was a problem hiding this comment.
@german77 reviewed 2 files and made 5 comments.
Reviewable status: 2 of 5 files reviewed, 5 unresolved discussions (waiting on MonsterDruide1).
src/common/aglShaderLocation.cpp line 23 at r1 (raw file):
} UniformBlockLocation::~UniformBlockLocation() = default;
lazy. Move it to the header
Code quote:
UniformBlockLocation::~UniformBlockLocation() = default;src/common/aglShaderLocation.cpp line 31 at r1 (raw file):
const Shader& shader = program.getShader(type); const void* data = shader.getShaderBinary();
Suggestion:
const void* data = program.getShader(type).getShaderBinary();src/common/aglShaderLocation.cpp line 52 at r1 (raw file):
return -1; UniformBlock* uniformBlocks = shaderBinary->uniformBlocks;
Generated by the compiler
src/common/aglShaderLocation.cpp line 55 at r1 (raw file):
// TODO: probably an inlined search function returning UniformBlock* for (u32 i = 0; i < numUniformBlocks; i++) {
Suggestion:
for (u32 i = 0; i < shaderBinary->numUniformBlocks; i++) {
UniformBlock* uniformBlocks = shaderBinary->uniformBlocks;src/common/aglShaderLocation.cpp line 59 at r1 (raw file):
if (&uniformBlocks[i]) return uniformBlocks[i].location; return -1;
Suggestion:
break;Co-authored-by: tetraxile <tetraxile@proton.me>
737cfaa to
dd0e689
Compare
MonsterDruide1
left a comment
There was a problem hiding this comment.
@MonsterDruide1 made 5 comments.
Reviewable status: 2 of 5 files reviewed, 5 unresolved discussions (waiting on german77 and MonsterDruide1).
src/common/aglShaderLocation.cpp line 23 at r1 (raw file):
Previously, german77 (Narr the Reg) wrote…
lazy. Move it to the header
Done.
src/common/aglShaderLocation.cpp line 52 at r1 (raw file):
Previously, german77 (Narr the Reg) wrote…
Generated by the compiler
Done.
src/common/aglShaderLocation.cpp line 31 at r1 (raw file):
const Shader& shader = program.getShader(type); const void* data = shader.getShaderBinary();
Done.
src/common/aglShaderLocation.cpp line 55 at r1 (raw file):
// TODO: probably an inlined search function returning UniformBlock* for (u32 i = 0; i < numUniformBlocks; i++) {
Done.
src/common/aglShaderLocation.cpp line 59 at r1 (raw file):
if (&uniformBlocks[i]) return uniformBlocks[i].location; return -1;
Done.
german77
left a comment
There was a problem hiding this comment.
@german77 made 2 comments and resolved 3 discussions.
Reviewable status: 2 of 5 files reviewed, 2 unresolved discussions (waiting on MonsterDruide1).
src/common/aglShaderLocation.cpp line 52 at r1 (raw file):
Previously, MonsterDruide1 wrote…
Done.
No. The whole block that I selected can be deleted including the early return
src/common/aglShaderLocation.cpp line 55 at r1 (raw file):
Previously, MonsterDruide1 wrote…
Done.
No. See for loop boundary. Is a direct reference from the shaderBinary
MonsterDruide1
left a comment
There was a problem hiding this comment.
@MonsterDruide1 made 2 comments.
Reviewable status: 2 of 5 files reviewed, 2 unresolved discussions (waiting on german77 and MonsterDruide1).
src/common/aglShaderLocation.cpp line 52 at r1 (raw file):
Previously, german77 (Narr the Reg) wrote…
No. The whole block that I selected can be deleted including the early return
Ah, I didn't check the diff, just the suggestion view.
src/common/aglShaderLocation.cpp line 55 at r1 (raw file):
Previously, german77 (Narr the Reg) wrote…
No. See for loop boundary. Is a direct reference from the shaderBinary
Done.
german77
left a comment
There was a problem hiding this comment.
@german77 reviewed 3 files and all commit messages, and resolved 2 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on MonsterDruide1).
I felt like decompiling something today. @tetraxile recently posted some headers for these two classes into the discord, so why not take that as inspiration and persist them at the right location in the repo?
The
ShaderBinarystruct seems to be platform-specific - the Wii-specific repos useGX2variants there, which have a different layout. More research on the file format would need to be done, followed by properly adding that struct somewhere else - so I've kept it asTODOfor now.This change is