2474: Fix FreeImage texture loader bugs, add test of image contents #2475
Conversation
The main one was not calling FreeImage_ConvertTo24Bits() on 32-bit input images, but the std::memcpy() was also wrong. Fixes #2474
ghost
assigned 
| mipImage = image; | ||
| } | ||
|
|
||
| const auto bytesPerPixel = FreeImage_GetLine(mipImage) / FreeImage_GetWidth(mipImage); |
There was a problem hiding this comment.
What is going on here? Please document, or even better, extract a function?
There was a problem hiding this comment.
The bytesPerPixel calculation is from the FreeImage documentation, I'll add a comment and reference the part of the docs I'm following here.
There was a problem hiding this comment.
or do you mean these for loops below in general? Yeah, I can extract this into a function. It's copying out the contents of the FIBITMAP as a sequence of bytes ordered RGBA, with no padding between the rows. (Internally in the FBITMAP they're stored in an order that is given by the FI_RGBA_* macros, it might not be RGBA, and there is padding between the rows so you can't just memcpy the whole image).
There was a problem hiding this comment.
Ah, ok. But also I'm wondering why we create mipmaps here when we already have code in Texture that will instruct OpenGL to generate them. Is it not better to leave it to GL?
There was a problem hiding this comment.
Yeah, we could just leave it to OpenGL to create the mipmaps, that would remove the outer loop. This code is also slower because it scales each mipmap from the full size image, instead of scaling it from the next larger mipmap which is what you should do, afaik.
Is it possible to change that without some refactoring to all of the texture loaders? Right now it looks like you pass in the number of mip levels you want to the constructor FreeImageTextureReader::FreeImageTextureReader. Maybe we should leave that for later if it requires refactoring all the loaders?
There was a problem hiding this comment.
We could use that but from my reading of docs/bugs, it doesn't change the byte order, so you still have to check the FI_RGBA_* macros.
https://sourceforge.net/p/freeimage/bugs/172/
Looking at FreeImage.h and FI_RGBA_RED etc., the byte order is either BGRA or RGBA so we could just check for the order and map it to the correct OpenGL enum.
There was a problem hiding this comment.
FreeImageTextureReader is constructed in TextureLoader::createTextureReader with a default of 4 mip levels. I think that's fine?
There was a problem hiding this comment.
Mapping the image format to the correct GL enum seems like the way to go. Less code means less bugs, and also faster texture loading.
There was a problem hiding this comment.
Agreed, updating it now. Also I see how to skip generating the mips in FreeImageTextureReader.
There was a problem hiding this comment.
Oh, right, there's more code to generate mips in there. I think that can just be removed. We should leave it to OpenGL to generate them, it'll probably be faster and more correct that way.
Fixes #2474