Skip to content

Commit 6ea08b4

Browse files
authored
gl: small fixes to push constant implementation and test (#9594)
- Add code for handling empty/null constant name - Use glUniform1ui() instead of glUniform1i() to workaround crash on macbook pro (m4). - Fully specify the constant name in the test (e.g. `pushConstantsF.red` instead of `red`). - Make sure that the uniform names are different between vertex and fragment in the test. This is due to different constant structs being defined between the two stages. FIXES=453757504
1 parent 5eb0f7d commit 6ea08b4

File tree

3 files changed

+23
-17
lines changed

3 files changed

+23
-17
lines changed

filament/backend/src/opengl/OpenGLDriver.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -437,7 +437,8 @@ void OpenGLDriver::setPushConstant(ShaderStage const stage, uint8_t const index,
437437
if (std::holds_alternative<bool>(value)) {
438438
assert_invariant(type == ConstantType::BOOL);
439439
bool const bval = std::get<bool>(value);
440-
glUniform1i(location, bval ? 1 : 0);
440+
// This must be the 'ui' version of glUniform1 due to a crash on M-series macbooks.
441+
glUniform1ui(location, bval ? 1 : 0);
441442
} else if (std::holds_alternative<float>(value)) {
442443
assert_invariant(type == ConstantType::FLOAT);
443444
float const fval = std::get<float>(value);

filament/backend/src/opengl/OpenGLProgram.cpp

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -254,8 +254,14 @@ void OpenGLProgram::initializeProgramState(OpenGLContext& context, GLuint progra
254254
mPushConstants.reserve(totalConstantCount);
255255
mPushConstantFragmentStageOffset = vertexConstants.size();
256256
auto const transformAndAdd = [&](Program::PushConstant const& constant) {
257-
GLint const loc = glGetUniformLocation(program, constant.name.c_str());
258-
mPushConstants.push_back({loc, constant.type});
257+
if (!constant.name.empty()) {
258+
GLint const loc = glGetUniformLocation(program, constant.name.c_str());
259+
mPushConstants.push_back({loc, constant.type});
260+
} else {
261+
// If the constant is not named, then we assume it will not be referenced in this
262+
// program.
263+
mPushConstants.push_back({ -1, constant.type });
264+
}
259265
};
260266
std::for_each(vertexConstants.cbegin(), vertexConstants.cend(), transformAndAdd);
261267
std::for_each(fragmentConstants.cbegin(), fragmentConstants.cend(), transformAndAdd);

filament/backend/test/test_PushConstants.cpp

Lines changed: 13 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -52,17 +52,17 @@ layout(push_constant) uniform Constants {
5252
float triangleScale;
5353
float triangleOffsetX;
5454
float triangleOffsetY;
55-
} pushConstants;
55+
} pushConstantsV;
5656
5757
layout(location = 0) in vec4 mesh_position;
5858
void main() {
59-
if (pushConstants.hideTriangle) {
59+
if (pushConstantsV.hideTriangle) {
6060
// Test that bools are written correctly. All bits must be 0 if the bool is false.
6161
gl_Position = vec4(0.0);
6262
return;
6363
}
64-
gl_Position = vec4(mesh_position.xy * pushConstants.triangleScale +
65-
vec2(pushConstants.triangleOffsetX, pushConstants.triangleOffsetY), 0.0, 1.0);
64+
gl_Position = vec4(mesh_position.xy * pushConstantsV.triangleScale +
65+
vec2(pushConstantsV.triangleOffsetX, pushConstantsV.triangleOffsetY), 0.0, 1.0);
6666
#if defined(TARGET_VULKAN_ENVIRONMENT)
6767
// In Vulkan, clip space is Y-down. In OpenGL and Metal, clip space is Y-up.
6868
gl_Position.y = -gl_Position.y;
@@ -76,12 +76,12 @@ layout(push_constant) uniform Constants {
7676
bool padding; // test correct bool padding
7777
float green;
7878
float blue;
79-
} pushConstants;
79+
} pushConstantsF;
8080
8181
precision mediump int; precision highp float;
8282
layout(location = 0) out vec4 fragColor;
8383
void main() {
84-
fragColor = vec4(pushConstants.red, pushConstants.green, pushConstants.blue, 1.0);
84+
fragColor = vec4(pushConstantsF.red, pushConstantsF.green, pushConstantsF.blue, 1.0);
8585
})";
8686

8787
void initPushConstants() {
@@ -91,20 +91,19 @@ void initPushConstants() {
9191

9292
gVertConstants.reserve(4);
9393
gVertConstants.resize(4);
94-
gVertConstants[pushConstantIndex.TRIANGLE_HIDE] = {"hideTriangle", backend::ConstantType::BOOL};
95-
gVertConstants[pushConstantIndex.TRIANGLE_SCALE] = {"triangleScale", backend::ConstantType::FLOAT};
96-
gVertConstants[pushConstantIndex.TRIANGLE_OFFSET_X] = {"triangleOffsetX", backend::ConstantType::FLOAT};
97-
gVertConstants[pushConstantIndex.TRIANGLE_OFFSET_Y] = {"triangleOffsetY", backend::ConstantType::FLOAT};
94+
gVertConstants[pushConstantIndex.TRIANGLE_HIDE] = {"pushConstantsV.hideTriangle", backend::ConstantType::BOOL};
95+
gVertConstants[pushConstantIndex.TRIANGLE_SCALE] = {"pushConstantsV.triangleScale", backend::ConstantType::FLOAT};
96+
gVertConstants[pushConstantIndex.TRIANGLE_OFFSET_X] = {"pushConstantsV.triangleOffsetX", backend::ConstantType::FLOAT};
97+
gVertConstants[pushConstantIndex.TRIANGLE_OFFSET_Y] = {"pushConstantsV.triangleOffsetY", backend::ConstantType::FLOAT};
9898

9999
gFragConstants.reserve(4);
100100
gFragConstants.resize(4);
101-
gFragConstants[pushConstantIndex.RED] = {"red", backend::ConstantType::FLOAT};
102-
gFragConstants[pushConstantIndex.GREEN] = {"green", backend::ConstantType::FLOAT};
103-
gFragConstants[pushConstantIndex.BLUE] = {"blue", backend::ConstantType::FLOAT};
101+
gFragConstants[pushConstantIndex.RED] = {"pushConstantsF.red", backend::ConstantType::FLOAT};
102+
gFragConstants[pushConstantIndex.GREEN] = {"pushConstantsF.green", backend::ConstantType::FLOAT};
103+
gFragConstants[pushConstantIndex.BLUE] = {"pushConstantsF.blue", backend::ConstantType::FLOAT};
104104
}
105105

106106
TEST_F(BackendTest, PushConstants) {
107-
SKIP_IF(Backend::OPENGL, "see b/453757504");
108107
SKIP_IF(SkipEnvironment(OperatingSystem::CI, Backend::VULKAN), "see b/453776664");
109108
SKIP_IF(Backend::WEBGPU, "Push constants not supported on WebGPU");
110109

0 commit comments

Comments
 (0)