[models] free on params causes double free

[chriscamacho]

I was looking specifically at the PBR example in examples/models

I notice that GL (at least Nvidia's implementation) both allocates and free's material params. So I uncommented the the freeing of it in UnloadModel to further investigate. I loaded and unloaded an additional model (happened to be a multi mesh normally textured GLTF) and it didn't cause issues I also loaded a OBJ file with a MTL containing a texture

however now the sample was running, this highlighted a few other issues

the pbr sample copies the default material and so doesn't get some of its resources released as a consequence so I added to LoadMaterialPBR

    Material mat; // = LoadMaterialDefault();   // Initialize material to default
    #define MAX_MATERIAL_MAPS               12  //rlgl define
    mat.maps = (MaterialMap *)RL_CALLOC(MAX_MATERIAL_MAPS, sizeof(MaterialMap));

a CreateMaterial function might be needed...

I tried removing the free in UnloadMaterial but this causes issues, in the end I had to restore the RL_FREE in UnloadMaterial while still not freeing it in UnloadModel

in order to stop the sample from leaking, I then had to change the following in its deallocation section

   // UnloadMaterial(model.materials[0]); // Unload material: shader and textures
    UnloadShader(model.materials[0].shader);
    UnloadTexture(model.materials[0].maps[MATERIAL_MAP_ALBEDO].texture);
    UnloadTexture(model.materials[0].maps[MATERIAL_MAP_NORMAL].texture);
    UnloadTexture(model.materials[0].maps[MATERIAL_MAP_METALNESS].texture);
    UnloadTexture(model.materials[0].maps[MATERIAL_MAP_ROUGHNESS].texture);
    UnloadTexture(model.materials[0].maps[MATERIAL_MAP_OCCLUSION].texture);
    UnloadTexture(model.materials[0].maps[MATERIAL_MAP_IRRADIANCE].texture);
    UnloadTexture(model.materials[0].maps[MATERIAL_MAP_PREFILTER].texture);
    UnloadTexture(model.materials[0].maps[MATERIAL_MAP_BRDG].texture);

From memory I think that free(0) fails silently just returning, which is why its been fine till now...

I note that the sample is hard coded and can only be used in Dresden, and that albedo, metalness, and roughness can be only set for all sub models at once.

It is not uncommon to have these properties have different values per sub mesh, an avatar might have woolly gloves and shiny metal helmet for example

These are all available from a gltf file per material when exported from blender for example and while this is for those working on the GLTF loader, a thought does need to be spared on what happens when they do load multiple PBR materials and UnloadModel is called

I'm fine with lots of duplicate materials if you do want Unload model to do the material freeing (although there could be an UnloadModelKeepMats)

I'm really not sure what's the best way to handle this (just getting the pbr sample to work properly without making other stuff explode was bad enough!) the whole issue needs reviewing, ideally with anyone who happens to be working on the GLTF format

I feel strongly that a material system that can follow the path of Blender -> GLTF -> Raylib, would be a great asset not just for PBR but even quite simple multi material meshes (just being able to effect simple parameters like roughness etc so as to effect the lighting model, without the need of complex environment maps, would be very useful)

This would require a lighting shader that's a half way house between the simple lighting shader and the PBR shader.

[chriscamacho]

I did some light hacking on rlights.h to reset the counter, so it could add 4 lights (set up exactly the same) to the simpleLighting shader

I'm using the pbr shader example just with a new instance of the model using a different shader

other than making the model a little dimmer, the pbr shader is having little or no effect, (I tried with a range of settings) it does not behave in anyway like lighting!

notice on the obj with the albedo texture in the diffuse map and the 4 lights (they are R G B and magenta ) that the lights have a significant effect, yet do not effect the prb shaded one at all

I also tried changing the Desden cube map to a test texture that has coloured squares (see fog shader example) and the change has absolutely no effect

I did have a look at the shaders themselves and by rights they should be having more effect than they are... Colour me puzzled (just not with the pbr shader :wink: )

[raysan5]

@chriscamacho actually, the PBR shader is not working properly... it was reported a while ago...

[chriscamacho]

I've got ComputeMaterialProperty working for what its worth...

the light dot product seems wrong (ie zero or very close to it!)

[chriscamacho]

I'm getting somewhere but I do need a working MeshTangents, can anyone help?

[chriscamacho]

at the least can you initialise all params pointers to -1 and not free them it they are -1 ! UnloadModel and UnloadMesh

[chriscamacho]

I've though about this, and as no where in RL does params get allocated, so no where in RL should it be free'd ! Its a basic principle, that I follow even structurally within my own apps... whatever "layer" of the infrastructure allocates, its that layer that deallocates

[chriscamacho]

I dumped the generated textures to png's - the brdf looks good cube map and irradiance are all pure black prelit is half alpha and bottom half pure black

could be why the skybox example isn't working too....

[raysan5]

@chriscamacho yeah, the issue is probably there. You can use RenderDoc tool to capture one frame and analyze the generated textures/cubemaps and all the OpenGL state, it's the tool I usually use to debug those graphics issues.

[chriscamacho]

I'm gonna call it - there's just too much broken on the pbr example, would be simpler to start from scratch...

Can I suggest removing it, so no one else wastes their time on it

I'd also convert dresden to a png and remove the hdr version

any thoughts on the params pointer?

[raysan5]

@chriscamacho I prefer to leave the example, I'll try to review it at some point.

About material.params, I reviewed it to avoid alloc/free requirement.

[chriscamacho]

is 4 enough?

I'm not clear in my mind why the graphics driver was freeing this ? (at least when I was looking at the PBR example)

[raysan5]

Actually, I considered just removing material.params, I can't think of any use case at the moment. In any case, I'm just leaving it as 4 and users can change that value if really required.

[chriscamacho]

I still don't understand what the GL driver was doing allocating and deallocating to it ??? only ever come across it when I was attempting to fix the PBR sample...