Page 1 of 1

[2.2]PBS material with detailed normals but no base normal crashes

Posted: Fri Jan 25, 2019 4:47 pm
by al2950
As title says, Ogre 2.2 crashes in OgreHlmsPbs.cpp line 702

Code: Select all

        if( usesNormalMap )
        {
            TextureGpu *norma0lMapTex = datablock->getTexture( PBSM_NORMAL );
This is because 'useNormalMap' is set to true above because material has detailed normals. Ogre 2.1 did not have a problem. The question is though, do you need to have a base normal map if you want to use detailed normal maps, or should it support detailed normal maps on its own?

Re: [2.2]PBS material with detailed normals but no base normal crashes

Posted: Fri Jan 25, 2019 10:44 pm
by dark_sylinc
This is a C++ bug. Thanks. We should use the first normal map ptr we can use. Should be easy to fix.

In the future we would actually do this per normal map, i.e. each normal map may be in a different format. Right now we assume they're all using the same convention.
al2950 wrote: Fri Jan 25, 2019 4:47 pm The question is though, do you need to have a base normal map if you want to use detailed normal maps, or should it support detailed normal maps on its own?
Detailed normals maps without normal maps is supported by the math. See the shader code:

Code: Select all

/// If there is no normal map, the first iteration must
/// initialize pixelData.normal instead of try to merge with it.
@property( normal_map_tex )
    @piece( detail_nm_op_sum )+=@end
    @piece( detail_nm_op_mul )*=@end
@else
    @piece( detail_nm_op_sum )=@end
    @piece( detail_nm_op_mul )=@end
@end
Theoretically speaking, it is very slightly slower (vs using the only one detail normal map as a normal map), but that may fall into the microoptimization category.

Cheers
Matias

Re: [2.2]PBS material with detailed normals but no base normal crashes

Posted: Sun Jan 27, 2019 11:34 am
by al2950
Thanks

Would you like me to create a PR which fixes this, but also checks all normal maps use the same convention?

Re: [2.2]PBS material with detailed normals but no base normal crashes

Posted: Sun Jan 27, 2019 4:33 pm
by dark_sylinc
That would be most welcome! I have my hands full at the moment

Re: [2.2]PBS material with detailed normals but no base normal crashes

Posted: Tue Jan 29, 2019 2:09 pm
by al2950
Ok, so it turns out that detailed normal maps where broken in a variety of ways :D. I have fixed everything apart from one remaining issue that meant I had to study all of the new Texture code, which is a good thing for me.

Currently calculateHashForPreCreate checks the pixel format for normal maps to check for signed convention used, however calculateHashForPreCreate is called before' datablock->loadAllTextures()'

Code: Select all

    void HlmsPbs::calculateHashFor( Renderable *renderable, uint32 &outHash, uint32 &outCasterHash )
    {
        assert( dynamic_cast<HlmsPbsDatablock*>( renderable->getDatablock() ) );
        HlmsPbsDatablock *datablock = static_cast<HlmsPbsDatablock*>( renderable->getDatablock() );
        if( datablock->getDirtyFlags() & (DirtyTextures|DirtySamplers) )
        {
            //Delay hash generation for later, when we have the final (or temporary) descriptor sets.
            outHash = 0;
            outCasterHash = 0;
        }
        else
        {
            Hlms::calculateHashFor( renderable, outHash, outCasterHash );
        }

        datablock->loadAllTextures();
    }
This means the texture most likely has not been loaded and so is currently filled with a dummy texture with 0 size and an unknown pixel format (PFG_UNKNOWN)

The question is could I just update the function above to load texture before calling ' Hlms::calculateHashFor( renderable, outHash, outCasterHash );' or does this require a more heavy duty fix?

**EDIT** Answer no :(, if image meta data has not already been processed that texture info will not be available yet and will be loaded in a different thread... Help needed!

Re: [2.2]PBS material with detailed normals but no base normal crashes

Posted: Tue Jan 29, 2019 5:26 pm
by dark_sylinc
Looks left.... looks right.... whispers **it's by design**.

But obviously there can be bugs.

The Hlms code in 2.2 needs to be able to deal with the fact that the texture may not be loaded at all.
This is more obvious if you run the Test_TextureResidency (make sure OGRE_BUILD_TEST in CMake is set) where you can load the cubes, press F2 to set the textures to OnStorage mode, and they will show white.

If texture->isMetadataReady() returns true, you can trust the texture->getPixelFormat() return value.
Otherwise you can assume whatever default/fallback you want or pretend that texture does not exist (i.e. no normal maps) because the texture will return white while it's used without being fully loaded (and white isn't valid normal data).
Pretending it doesn't exist may clash with other code, so the safest route is to take the bite with few rendering glitches and just use the texture with fallback defaults.

Once the texture finishes loading, OGRE_HLMS_TEXTURE_BASE_CLASS::notifyTextureChanged will be called, the texture descriptors will be recreated, updateDescriptorSets will be called, needsRecalculateHash should become true*, and thus flushRenderables will be called.

This last call will force HlmsPbs::calculateHashFor to be called again.

(*) If needsRecalculateHash didn't become true, that means the old texture descriptor and the new texture descriptor are exactly the same; which could not happen if the texture previously had invalid metadata and now it is fully loaded.

TL;DR: If the texture returns format PFG_UNKNOWN, just assume a default and don't worry, HlmsPbs::calculateHashFor will soon be called again with the correct value. If that doesn't happen, then we're in a presence of a bug (assuming the texture isn't being kept unloaded on purpose).
That's why the texture metadata cache is so important, because without it, you may end up compiling two (or more) shaders instead of just one.

Re: [2.2]PBS material with detailed normals but no base normal crashes

Posted: Tue Jan 29, 2019 5:33 pm
by al2950
Ah ok, got it. Had not quite realized HlmsPbs::calculateHashFor would be called again, that makes much more sense :lol:. I will update my fix and create a PR... probably tomorrow

Re: [2.2]PBS material with detailed normals but no base normal crashes

Posted: Wed Jan 30, 2019 1:38 pm
by al2950
Done please see PR here.

The only thing to note is the use of OGRE_HLMS_TEXTURE_BASE_CLASS::_setTexture. We were passing a nullptr for the sampler, with valid texture which I have fixed. However the function has a default value of 0 for the sampler, suggesting its ok to pass 0. This is not the case, at least not the case if you are passing a valid texture ptr but not a valid sampler ptr. Suggest removing the default value or/and adding asserts to prevent misuse?