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

Uniform num broken on Mac OS 12.6 arm64/Vulkan by #2883/#2885 #2964

Open
jdah opened this issue Oct 27, 2022 · 6 comments
Open

Uniform num broken on Mac OS 12.6 arm64/Vulkan by #2883/#2885 #2964

jdah opened this issue Oct 27, 2022 · 6 comments
Labels

Comments

@jdah
Copy link

jdah commented Oct 27, 2022

Describe the bug
Shaders which declare a uniform array uniform vec4 u_array[3] now report a num (from bgfx::getUniformInfo) of 1 instead of 3 as expected. I have tested in my own code (by reverting the change) and the issue came from #2883 (PR #2885) where regCount was replaced with num to fix array sizes.

To Reproduce

  1. declare an array uniform vec4 ...[n] somewhere
  2. check num with bgfx::getUniformUnfo
  3. num != n

Expected behavior
num == n

Additional context
Tested on Mac OS Monterey/12.6, M1 Pro/arm64, MoltenVK (Vulkan backend!), latest bgfx

will hopefully come back with further details/proposed fix.

@bkaradzic
Copy link
Owner

cc @cloudwu

@bkaradzic bkaradzic added the bug label Oct 27, 2022
@cloudwu
Copy link
Contributor

cloudwu commented Oct 28, 2022

I think the reason is that the num hasn't encoded into shader file correctly for Vulkan.

https://github.com/bkaradzic/bgfx/blob/master/tools/shaderc/shaderc_spirv.cpp#L655

It should be un.num = uint16_t(program->getUniformArraySize(ii)); like other backend :

metal : https://github.com/bkaradzic/bgfx/blob/master/tools/shaderc/shaderc_metal.cpp#L455
hlsl : https://github.com/bkaradzic/bgfx/blob/master/tools/shaderc/shaderc_hlsl.cpp#L373
glsl : https://github.com/bkaradzic/bgfx/blob/master/tools/shaderc/shaderc_glsl.cpp#L207

I read the history, and found the PR #2421 changed this behavior. (It's strange that this PR changed Vulkan backend only)
We can revert #2421 to solve this issue, but I think there would be a better solution @bkaradzic (to support uniform num > 255)


PR #2885 use num instead of regCount , so bgfx::getUniformUnfo() will returns 1 ( bx::max<uint16_t>(1, _num)) rather than 3.

But if you decalre an array of matrix uniform mat4 ...[3] (reverting PR #2885) , the regCount would be 12. it's not correct, too.

@bkaradzic
Copy link
Owner

but I think there would be a better solution @bkaradzic (to support uniform num > 255)

Yeah, I need to get some better way of handling this reflection, and figure out how to unit test it.

@bkaradzic
Copy link
Owner

@jdah What changes you needed to add to bgfx to build with MoltenVK?

@jdah
Copy link
Author

jdah commented Oct 28, 2022

None, IIRC - BGFX is set to target Vulkan and then you use the MoltenVK dynamic library (from (MoltenVK Path)/dylib/macOS/libMoltenVK.dylib) alongside the application

@rchen-adb
Copy link

Related to the uniform array size limitation, I think there's some slight inconsistent behavior between the Metal and Vulkan backend for predefined uniforms. In Metal, if your array is sized > 255, it is handled because the m_count value comes from the Metal reflection. In Vulkan, the value wraps around because it comes from regCount which is limited to uint8 here, since it is equal to Uniform::num.

Just mentioning this because currently you will see predefined arrays sized > 255 work fine on Metal but break in Vulkan. This happens ie if you define BGFX_CONFIG_MAX_BONES to be 256.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants