HLMS PBR BRDF

[rujialiu]

Am I missing anything?

One small thing that we discussed privately: should we disable the "minimal mip of filtered cubemap should be 16x16" thing for IblSpecular?

For people who are interested, dark_sylinc said:
"Regarding the mipmap thing: I limited them to 16x16 because that's what latest Filamente (git) was doing by default. I don't what's the 'proper' or 'default' behavior. Perhaps limiting it to 16x16 is a regression in filament, or 1x1 was a bug."

And I replied:
"I think limiting to 16x16 is not good because when you have a row of spheres as I do, the visual roughness is "clampped" i.e. there is no difference between spheres with high roughness! that's clearly not what I want..."

In short: if you want the visual appreance "varies linearly when you change roughness linearly", disable the limit by commenting out a single line:

Code: Select all

diff --git a/Components/Hlms/Pbs/src/Cubemaps/OgreParallaxCorrectedCubemapBase.cpp b/Components/Hlms/Pbs/src/Cubemaps/OgreParallaxCorrectedCubemapBase.cpp

index 2e7e0a25..3ada3e3d 100644
--- a/Components/Hlms/Pbs/src/Cubemaps/OgreParallaxCorrectedCubemapBase.cpp
+++ b/Components/Hlms/Pbs/src/Cubemaps/OgreParallaxCorrectedCubemapBase.cpp
@@ -123,7 +123,7 @@ namespace Ogre
     uint8 ParallaxCorrectedCubemapBase::getIblNumMipmaps( uint32 width, uint32 height )
     {
         uint8 numMipmaps = PixelFormatGpuUtils::getMaxMipmapCount( width, height );
-        numMipmaps = std::max<uint8>( numMipmaps, 5u ) - 4u;
+        //numMipmaps = std::max<uint8>( numMipmaps, 5u ) - 4u;
         return numMipmaps;
     }

Anyone else wants to join the discussion?

[TaaTT4]

Hi rujialiu,

If I remember correctly, in a PM you told me that letting mipmaps go until 1x1 allows to have a perfect match with substance player, right? Plus, what you're saying sounds correct: limiting mipmaps to 16x16 cause objects to be visually equal even if they use different roughness value (and that's would be incorrect).
We could open an issue on filament github and ask if it's a regression or there's a reason behind that choice.

[rujialiu]

If I remember correctly, in a PM you told me that letting mipmaps go until 1x1 allows to have a perfect match with substance player, right?

Unfortunately no... I never had time to compare this with substance player, I'm just comparing it with what I'm expecting from my mind
But I actually did tell you a different perfect match:
1. raw cubemap as skybox (no filtering) + PCC + pass ibl_Specular
2. filter the same cubemap with filament's cmgen in "latest released binary" and use it directly
The cmgen.exe in "latest released binary" I was using did NOT have the 16x16 limit, so the perfect matching should mean Ogre's ibl_specular implementation matched cmgen's.

However, dark_sylinc's using filament's "git version" (I think he compiled it by himself) which has the 16x16 limit. So it looks like there are two versions of filament, one with the limit, one without. But I never had time to find out whether/when/why the behavior changed.

Plus, what you're saying sounds correct: limiting mipmaps to 16x16 cause objects to be visually equal even if they use different roughness value (and that's would be incorrect).
We could open an issue on filament github and ask if it's a regression or there's a reason behind that choice.

Yes! But maybe it's better to confirm it first because several months already passed. Maybe the behavior changed again

[TaaTT4]

The cmgen.exe in "latest released binary" I was using did NOT have the 16x16 limit, so the perfect matching should mean Ogre's ibl_specular implementation matched cmgen's.

However, dark_sylinc's using filament's "git version" (I think he compiled it by himself) which has the 16x16 limit. So it looks like there are two versions of filament, one with the limit, one without. But I never had time to find out whether/when/why the behavior changed.

I took a brief look of cmgen and this seems the commit where they introduced the 16x16 limitation.

This is how they justify the change:
Filament will now allow incomplete mipmap chains for the IBL and
map the roughness to the available chain.

This allows to create roughness cubemaps with a minim size for
roughness==1, instead of always mapping it to 1x1.

cmgen will now use 16x16 cubemaps for roughness==1, which
improves the quality of rough objects significantly.

It's in play starting from version 1.4.4 and 16x16 is the default unless you use the --ibl-min-lod-size command option. What cmgen version are you using rujialiu?

[rujialiu]

It's in play starting from version 1.4.4 and 16x16 is the default unless you use the --ibl-min-lod-size command option. What cmgen version are you using rujialiu?

I'm using 1.4.3........ I believe that 1.4.4 was not released when I started using it, and when dark_sylinc cloned the repo, that code is already committed... Thanks!!!

Their comment makes sense and there must be something wrong elsewhere because according to their explanation, roughness mapping is still linear. For exmaple, if the mipmaps are of size 256, 128, 64, 32, 16, they can be mapped to roughness 0.0, 0.25, 0.5, 0.75, 1.0. But it looks like without my modification, we're mapping 256, 128, 64, 32, 16, 16, 16, 16, 16 to 0.0, 0.125, 0.25, 0.375, ..., 0.875, 1.0, so when roughness>=0.5 it looks the same.

[TaaTT4]

Their comment makes sense and there must be something wrong elsewhere because according to their explanation, roughness mapping is still linear. For exmaple, if the mipmaps are of size 256, 128, 64, 32, 16, they can be mapped to roughness 0.0, 0.25, 0.5, 0.75, 1.0. But it looks like without my modification, we're mapping 256, 128, 64, 32, 16, 16, 16, 16, 16 to 0.0, 0.125, 0.25, 0.375, ..., 0.875, 1.0, so when roughness>=0.5 it looks the same.

Probably it's because the pixel shader ignores that when ibl_specular is used the lowest mip is 16x16 and not 1x1. I guess it's something that Matias should be able to fix easily.
Ehi Matias, are you listening our prayers?

[dark_sylinc]

I don't know where the bug is (assuming there is one).

Everything has been coding under the assumption that if the cubemap goes from NxN to 16x16; then 16x16 = roughness 1.0

It's possible there's a very silly bug somewhere, either during the ibl_specular generation (which needs to be aware that mip 16x16 is roughnes 1.0 and distribute roughness evenly along the range of valid mips) or during sampling (in 800.PixelShader_ps.any and related shaders).

But I'm thinking of integrating RDV first (RemoteDebugVars), which is a simple but powerful tool I developed with a client and I've been given permission to open source it; which allows connecting an app to a GUI tool via TCP, in order to remotely control in realtime its debug variables (e.g. move a roughness slider in realtime); and would make our debugging job much easier

[TaaTT4]

I don't know where the bug is (assuming there is one).

Guess I know where it is
In ParallaxCorrectedCubemapAuto::setEnabled is used hlmsPbs->_notifyIblSpecMipmap( mRenderTarget->getNumMipmaps() ); to set the lowest mipmap, while it should be used mBindTexture->getNumMipmaps() (like it's done in HlmsPbs::resetIblSpecMipmap when numMipmaps is equal to 0). At the moment, considering a 512x512 reflection map, use mRenderTarget->getNumMipmaps() submits to the pixel shader a wrongly value of 10 (instead of 6) in pccVctMinDistance_invPccVctInvDistance_rightEyePixelStartX_envMapNumMipmaps.w.

OT: there's a similar bug in Sample_Tutorial_DynamicCubemap: the first time the sample runs, HlmsPbs::mMaxSpecIblMipmap remains equal to 1 because HlmsPbsDatablock::notifyTextureChanged isn't invoked before calling hlmsPbs->resetIblSpecMipmap( 0u ); in DynamicCubemapGameState::setupCompositor().

[dark_sylinc]

That's some goddamn great detective work!

Fix pushed. Thanks!

[rujialiu]

That's some goddamn great detective work!

Fix pushed. Thanks!

Thank you and TaaTT4!!!