HLMS PBR BRDF

Discussion area about developing with Ogre2 branches (2.1, 2.2 and beyond)
rujialiu
Goblin
Posts: 294
Joined: Mon May 09, 2016 8:21 am
x 35

Re: HLMS PBR BRDF

Post by rujialiu »

dark_sylinc wrote:
Wed Apr 22, 2020 12:50 am
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?

User avatar
TaaTT4
OGRE Contributor
OGRE Contributor
Posts: 241
Joined: Wed Apr 23, 2014 3:49 pm
x 45

Re: HLMS PBR BRDF

Post by 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.
Senior game programmer at Vae Victis
Working on Racecraft

rujialiu
Goblin
Posts: 294
Joined: Mon May 09, 2016 8:21 am
x 35

Re: HLMS PBR BRDF

Post by rujialiu »

TaaTT4 wrote:
Sat Apr 25, 2020 11:06 am
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.
TaaTT4 wrote:
Sat Apr 25, 2020 11:06 am
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 8-)

User avatar
TaaTT4
OGRE Contributor
OGRE Contributor
Posts: 241
Joined: Wed Apr 23, 2014 3:49 pm
x 45

Re: HLMS PBR BRDF

Post by TaaTT4 »

rujialiu wrote:
Tue Apr 28, 2020 4:01 am
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?
Senior game programmer at Vae Victis
Working on Racecraft

rujialiu
Goblin
Posts: 294
Joined: Mon May 09, 2016 8:21 am
x 35

Re: HLMS PBR BRDF

Post by rujialiu »

TaaTT4 wrote:
Mon May 04, 2020 11:59 am
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.

User avatar
TaaTT4
OGRE Contributor
OGRE Contributor
Posts: 241
Joined: Wed Apr 23, 2014 3:49 pm
x 45

Re: HLMS PBR BRDF

Post by TaaTT4 »

rujialiu wrote:
Wed May 06, 2020 2:30 pm
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? :D
Senior game programmer at Vae Victis
Working on Racecraft

User avatar
dark_sylinc
OGRE Team Member
OGRE Team Member
Posts: 4501
Joined: Sat Jul 21, 2007 4:55 pm
Location: Buenos Aires, Argentina
x 936
Contact:

Re: HLMS PBR BRDF

Post by 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

User avatar
TaaTT4
OGRE Contributor
OGRE Contributor
Posts: 241
Joined: Wed Apr 23, 2014 3:49 pm
x 45

Re: HLMS PBR BRDF

Post by TaaTT4 »

dark_sylinc wrote:
Wed May 06, 2020 5:13 pm
I don't know where the bug is (assuming there is one).
Guess I know where it is :D
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().
Senior game programmer at Vae Victis
Working on Racecraft

User avatar
dark_sylinc
OGRE Team Member
OGRE Team Member
Posts: 4501
Joined: Sat Jul 21, 2007 4:55 pm
Location: Buenos Aires, Argentina
x 936
Contact:

Re: HLMS PBR BRDF

Post by dark_sylinc »

That's some goddamn great detective work!

Fix pushed. Thanks!

rujialiu
Goblin
Posts: 294
Joined: Mon May 09, 2016 8:21 am
x 35

Re: HLMS PBR BRDF

Post by rujialiu »

dark_sylinc wrote:
Tue May 19, 2020 6:02 pm
That's some goddamn great detective work!

Fix pushed. Thanks!
Thank you and TaaTT4!!!

Post Reply