#853 Merged at fa1da5a
Repository
ogre
Branch
v2-2-WIP
Author
  1. Angus Laurie-Pile
Reviewers
Description
  • bugfix - Pbs HLMS texture piece had incorrect parameter names fo detail normal map sampler, causing shaders not to compile bugfix - Pbs HLMS had normalMapWeight defined after it was used, causing shaders not to compile Have moved the define to stuct pieces as this is where the actual varibles the define uses are defined themselves bugfix - Pbs HLMS using incorrect parameter name for detailed normal map array index
  • bugfix - Crash caused by assuming there was a normal map if there were detailed normal map Fix removes this assumption, but also does a check to make sure all normal maps use the same signed convention
  • bugfix - Hlms Json loaders were passing a nullptr to OGRE_HLMS_TEXTURE_BASE_CLASS::_setTexture, if not sampler was defeined causing an incorrect sampler index to be computer later in OGRE_HLMS_TEXTURE_BASE_CLASS::getIndexToDescriptorSampler (As no sampler was defined, was returning an index of 15)
  • bugfix - Crash caused by trying to generate mipmaps for compressed textures that did not have all their mipmaps. Check added to prevent Ogre generating mipmaps for compressed textures

Comments (6)

  1. Matias Goldberg

    Interesting bugs!

    Overall it's good.

    I have three problems though:

    1. The use of ´std::vector<TextureGpu*> datablockNormalMaps;´. I don't feel comfortable allocating memory here. It's a function that will run N times, with N being the number of objects created or higher; and that's a lot of micro allocations and deallocations, where are regular array living on the stack would work just fine. std::array would work fine here, but I don't think it's supported by VS2008, so I may just write our own since it's a trivial container, and replace that in the code.

    2. The following: ´PixelFormatGpuUtils::isCompressed( image.getPixelFormat() ) && image.getNumMipmaps() <= 1u;´ is not enough. There should be a routine called ´PixelFormatGpuUtils::supportsHwMipmap´ or similar. This document describes all formats that support generating mipmaps: https://docs.microsoft.com/en-us/windows/desktop/api/d3d11/nf-d3d11-id3d11devicecontext-generatemips It is much more restrictive than it seems. It's just that BC formats are loud because a RenderTarget with that format cannot be generated, whereas the other formats are silent. A rule of thumb is that GL would likely support the same formats.

    3. Simulate filters is not distinguishing between SW and HW mipmaps.

    I think I'll accept this PR since it doesn't introduce any regression except for point 3, and fix the issues afterwards.

    1. Angus Laurie-Pile author

      Thanks for reviewing quickly 🙂

      The use of ´std::vector<TextureGpu*> datablockNormalMaps;´. I don't feel comfortable allocating memory here. It's a function that will run N times, with N being the number of objects created or higher; and that's a lot of micro allocations and deallocations, where are regular array living on the stack would work just fine. std::array would work fine here, but I don't think it's supported by VS2008, so I may just write our own since it's a trivial container, and replace that in the code.

      I have never considered those functions to be particularly performance sensitive but thats a fair point. I wrote from a point of view of readability but can re-write and remove the heap allocations if you would like.

      The following: ´PixelFormatGpuUtils::isCompressed( image.getPixelFormat() ) && image.getNumMipmaps() <= 1u;´ is not enough. There should be a routine called ´PixelFormatGpuUtils::supportsHwMipmap´ or similar. This document describes all formats that support generating mipmaps: https://docs.microsoft.com/en-us/windows/desktop/api/d3d11/nf-d3d11-id3d11devicecontext-generatemips It is much more restrictive than it seems. It's just that BC formats are loud because a RenderTarget with that format cannot be generated, whereas the other formats are silent. A rule of thumb is that GL would likely support the same formats.

      Good point! I can add

      Simulate filters is not distinguishing between SW and HW mipmaps.

      This is something I have not yet fully understood but I guess there should be a PixelFormatGpuUtils::supportsSwMipmap as well as PixelFormatGpuUtils::supportsHwMipmap as mentioned above? Again I can sort this 🙂

  2. Matias Goldberg

    I have never considered those functions to be particularly performance sensitive but thats a fair point. I wrote from a point of view of readability but can re-write and remove the heap allocations if you would like.

    I'm writing a tiny std::array replacement that we can use. I like your approach because it's readable. That function is already cryptic enough as it is.

    This is something I have not yet fully understood but I guess there should be a PixelFormatGpuUtils::supportsSwMipmap as well as PixelFormatGpuUtils::supportsHwMipmap as mentioned above? Again I can sort this 🙂

    Yes, but I want to handle the supportsSwMipmap one so it's done in a way that is always up to date with whatever Image2::generateMipmaps does.

  3. Matias Goldberg

    Merged manually. I've changed the std::vector to StackVector, and finished the HW/SW mipmapping support issues. The original code was a bit rough on the edges. Thanks for helping me out realize that.