Clean up the primitive ID textured shader, resolve the WebGL TODO #162
Conversation
facebook-github-bot
added
the
CLA Signed
| Cr::Containers::ArrayView<std::uint32_t> objectIds) { | ||
| const Mn::Vector2i textureSize{ | ||
| gfx::PrimitiveIDTexturedShader::PrimitiveIDTextureWidth, | ||
| int(objectIds.size() + |
There was a problem hiding this comment.
| int(objectIds.size() + | |
| int(std::ceil(float(objectIds.size()) / gfx::PrimitiveIDTexturedShader::PrimitiveIDTextureWidth)) |
There was a problem hiding this comment.
Well. I'm usually going over code and replacing ceil(float()) with the pure-integer solution, not the other way around 😆
Are you sure this would work correctly for values that have more than ~23 bits of precision? One PLY file has 9M of primitives, which is 24 bits. I'd trust the integer solution there instead ;)
There was a problem hiding this comment.
I pretty much always prefer ceil(float()) for readability :) -- The precision definitely isn't an issue here. The precision just needs to be good enough to separate things by gfx::PrimitiveIDTexturedShader::PrimitiveIDTextureWidth (which, since it's a power of 2, can be perfectly represented a float).
There was a problem hiding this comment.
But this is entirely a personal preference, I am fine with it either way :)
Before it was a 32-bit float texture inflated to nearest largest power-of-two, which used more than twice the memory needed. Now it's just 16-bit uints and the texture has only one dimension POT'd. Moreover, instead of doing texture() and having to pass texture size to the shader, the shader now uses texelFetch(), which also resolves the WebGL TODO.
The definitions were absolutely inconsistent with what the shader actually wanted, no wonder they were totally useless.
|
Pushed the typo fix, since it's a non-trivial change I'd like to have at least one thorough review. And since this fixes the WebGL TODO, any chance somebody could test that? :) |
|
Oh, this is not merged yet? Can somebody give me one more approval, please? |
…cebookresearch#162) * Save uniform location instead of retrieving it again every time. * This comment does more harm than good. * Rework how the primitive ID texture is used. Before it was a 32-bit float texture inflated to nearest largest power-of-two, which used more than twice the memory needed. Now it's just 16-bit uints and the texture has only one dimension POT'd. Moreover, instead of doing texture() and having to pass texture size to the shader, the shader now uses texelFetch(), which also resolves the WebGL TODO. * Properly define and actually use shader attributes. The definitions were absolutely inconsistent with what the shader actually wanted, no wonder they were totally useless. * We can directly use the packed colors, no need to inflate them 4 times.
Motivation and Context
Follow-up to #151, doing a cleanup pass over the
PrimitiveIDTexturedShader. There was a few outdated assumptions about how GPUs work, unnecessarily large used data types and inconsistent attribute definitions. In short:texelFetch()instead oftexture()-- that way we sidestep all coordinate rounding, filtering etc., making the texel access faster, and we don't need to pass texture size to the shader. That nicely resolves the WebGL TODO as well.usampler2Dinstead ofsampler2D, avoiding more roundingHow Has This Been Tested
Tests pass locally. Unfortunately the PLY files can't be tested on the CI, so the green tick doesn't mean it works everywhere -- if you can, please try it out locally with MP3D data as well. The
FRLInstanceMeshDataupdate is done with best intentions in mind but as far as I understood it's a dead code.All in all, this update could have a significant effect on rendering performance of PLY files -- didn't measure, tho.