Fixed models using other model's textures bug

[CMDR-JohnAlex]

Hello! First of all I love your tutorials SO MUCH! Thank you for them!

I noticed that when you load a model that does not have a diffuse, specular, normal or height map it will use the one from the previously rendered model. What I have here was my simple fix to the issue. This should also be able to close issue #317 .

[JG-Adams]

It is a good practice. Although, I feel it may decrease performance because you're having to iterate through each textures and use an expensive set and bind function.
What I did to fix this type of issue is actually go to the Model file and implement a default image for every texture type that don't exist. As for normal you want to use a purple image.
Make sure it not having copies.
This way it's impossible to have invalid images.
Also, you might want to have glActiveTexture(GL_TEXTURE0); at the end.

[CMDR-JohnAlex]

Hello @JG-Adams. Now that I look at my code again it does look like there would be a performance decrease when using my workaround. The more textures loaded, the more the expensive set and bind functions are used... 🤦

Thank you for letting me know your workaround to this issue but I have one question. How did you implement the default image solution?
Right now I have some code checking if there are no textures if (mat->GetTextureCount(type) == 0) and if that is true then load the default texture for the material texture.id = textureFromFile(materialDefault, this->directory);. Did you do the same? Because this would require a default image for diffuse, specular, normals, etc next to every model. Unless of course the directory path is changed. I'm just curious how you did it.

Thank you!

[CMDR-JohnAlex]

I just updated my pull request to show my current solution. Probably not tutorial grade code so I don't expect it to be merged.
But I will leave this pr here unless someone closes it so others with the same issue can find a solution.

[JG-Adams]

I just updated my pull request to show my current solution. Probably not tutorial grade code so I don't expect it to be merged. But I will leave this pr here unless someone closes it so others with the same issue can find a solution.

It look almost identical to mine actually! :)
The way I was getting my default image is this:
texture.id = 1; // The default no-tex id!
if (typeName=="texture_normal")
texture.id = 2; // The default no-normal id!
This is dependent on the fact that the first two texture to ever exist is the default image acquired from file.
However, it would be desirable for it to operate solely on its own.
So, looking into this I figures you could use static member for an image to exist only as single shared object.
And static function in the constructor to build it only once no matter how many models you have.

Almost got it.

[JG-Adams]

Static function doesn't work the way I thought it would. But it's still fine.
This is a little sophisticated, but it conserve memory space and it work well for me!

Put this in private:
static unsigned int notex, nonorm;
And this outside of class to define it.
unsigned int Model::notex=0, Model::nonorm=0;

Put this in the constructor:if (notex==0)
{
uint8_t data[3] = {255,255,255};
glGenTextures(1, &notex);
glBindTexture(GL_TEXTURE_2D, notex);
glTexImage2D(GL_TEXTURE_2D, 0, GL_RGB, 1, 1, 0, GL_RGB, GL_UNSIGNED_BYTE, data);
glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE);
glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);
glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST);
glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_NEAREST);
}
if (nonorm==0)
{
uint8_t data[3] = {128,128,255};
glGenTextures(1, &nonorm);
glBindTexture(GL_TEXTURE_2D, nonorm);
glTexImage2D(GL_TEXTURE_2D, 0, GL_RGB, 1, 1, 0, GL_RGB, GL_UNSIGNED_BYTE, data);
glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE);
glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);
glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST);
glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_NEAREST);
}

In function TextureFromFile():
Put return textureID; inside of if (data) {} at the end. So, it return successful new fileimage.
In place of where that was, at the end, have return notex; (To use the default image by default)

Your new code where you do this:
if (!skip)
{ // if the empty texture hasn't been loaded already, load it
Texture texture;

            // just create an empty texture
            unsigned int textureID;   <--- replace this
            glGenTextures(1, &textureID);   <--- replace this
            texture.id = textureID;   <--- replace this

            texture.type = typeName;
            texture.path = path.c_str();
            textures.push_back(texture);
            textures_loaded.push_back(texture); // store it as texture loaded for entire model, to ensure we won't unnecessary load duplicate textures.
        }

Where I put <--- replace this
Replace with:
texture.id = notex;
if (typeName=="texture_normal")
texture.id++; // Use the next default image which is normalmap!

Then, below, in the existing code like the one above but has: texture.id = TextureFromFile(str.C_Str(), this->directory, gamma);
Place this after it.
if (texture.id==notex)
if (typeName=="texture_normal")
texture.id++;

And that's it!
Now you have the option to put in your own "No Tex!" image! :)

[JG-Adams]

Check this for reference. #323

[CMDR-JohnAlex]

The code looks great but I believe there is a problem with it. When I load sponza and look at the plants, you can see that the plant is very bright and sometimes there are parts of quads that didn't get discarded in the fragment shader?

Here is what it should look like for reference:

But the great thing I love about your solution is the option for a default texture! I have this sphere model that doesn't have a texture and when I load it in the fix I currently have, you don't see it... But with your fix it's possible! :)

[CMDR-JohnAlex]

From my understanding, those artefacts happen here:

if (notex == 0)
    {
        uint8_t data[3] = { 255,255,255 }; // <-- This
        glGenTextures(1, &notex);
        glBindTexture(GL_TEXTURE_2D, notex);
        glTexImage2D(GL_TEXTURE_2D, 0, GL_RGB, 1, 1, 0, GL_RGB, GL_UNSIGNED_BYTE, data);
        glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE);
        glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);
        glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST);
        glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_NEAREST);
    }

We have no alpha channel/no transparency so those fragments won't get discarded and therefor we see those strange artefacts?

I just tested by changing data to an array of 4 and I changed GL_RGB to GL_RGBA but I still get the strange artefacts, so I guess I am wrong.

[CMDR-JohnAlex]

Ok, so after a while of messing around I fixed the sponza plant issue and I'm just adding my changes to github right now.

Let me know if this works for you too and thank you for your help! 😃

[JG-Adams]

Yay! Teamwork!
I forgot all about the transparency! lol
I like it! I think we made a great improvement! :)
Although one thing about setting specular to 0 is that it render specular non-existence unless you provide a specular map.
Modern computer make it something you'd always put in. but sometime you might want just a simple model.
Actually, using shaders make that much less of an issue! lol!

Hmm, thinking about it though. This might cause confusion. Like, "why isn't there any specular?" if they didn't know it was set to 0.
So, you could have it as you want it. But leave it at 255 here, that would be good!

[CMDR-JohnAlex]

Yes! Teamwork! I didn't know I was good at working with other devs.
I too think this is a great improvement, thank you!

I added specular as 255 on github again as the, "why isn't there any specular?" scenario is possible.

[CMDR-JohnAlex]

I already wrote this in the commit message but here it is:

After slapping some std::cout statements everywhere I found that once the program loads a default diffuse, specular or normal texture it would use that again for the next missing texture!
So for example if a model doesn't have a specular or normal, it will check if the specular map is loaded, and because it is not it will use the default specular map for the missing specular map and the texture.path will be NO_TEXTURE. When we check if the normal map is already loaded it will see there is a texture already loaded with the name NO_TEXTURE so it will just use that texture thinking its the specular map it wanted, when in reality that is a specular map, not a normal map!
Same goes if a model doesn't have any textures, it will use the default texture for diffuse, specular and normal.

[JG-Adams]

Oh! woops! good find!

[JG-Adams]

This can function in the private section. Unless you have external texture you want to use.
There's a way for that too!

[CMDR-JohnAlex]

The issue with this is the textureFromFile() function is outside of the model class so when it tries to return Model::defaultDiffuse it can't because that is a private member.

[JG-Adams]

Having a default specular and others may not be necessary and can lead to same confusion.
This I think can be handled through the use of shaders instead.
Because it's really simple. Everything's white except Normal which need to face the proper direction.
You'll have things like heightmap, emission, shininess, opacity, displacement, lightmap, reflection, ect.
All are good as white, not black or invisible for a default.
It would be way impractical to use all of that for everything. So it's up to the designers to implement feature that make certain things get used out of necessity.

[CMDR-JohnAlex]

All are good as white, not black or invisible for a default.

Ok, I'll return the code to using a white specular for everything else.

Edit: Sorry it was taking me a while to do things, I'm a little busy on my end.