Previously we don't merge material cached list, it create dangling pointer and memory leak.
Now we merge material cache list during the scene merge, and remove material in this list during the library free.
Details
Details
Diff Detail
Diff Detail
- Repository
- rB Blender
- Branch
- ge_fix_libload
Event Timeline
Comment Actions
You should search for manifests that are fixed with this patch. If you find one then change the Title to BGE: Fix: Txxxx, Txxxx, this will auto close the manifests.
https://developer.blender.org/search/query/ryDPkVnNvrk2/#R
I don't looked much in this code area. But there is an extra for loops to delete the associated materials. I think this extra loop is not necessary and can be removed. Also this would speed up the search process for the next material.
I made a test the patch fix the crash, so it looks good for me.
But I think it is better that Moguri will look on this, because he is the LibLoad module owner.
| source/gameengine/Converter/KX_BlenderSceneConverter.cpp | ||
|---|---|---|
| 977–981 | short flag = 0; | |
| 1542 | This comment does not describe anything. The comment should describe in which case the method should be used.Same for the prototype. | |
| 1547 | Can be deleted if third for loop will be deleted. | |
| 1554 | materials->erase(matit->first); | |
| 1557 | This extra for loop is unnecessary. | |
| 1578 | Same as above in RemoveCachedMaterial. | |
| source/gameengine/Converter/KX_BlenderSceneConverter.h | ||
| 101 | Use Blender style KX_Scene *scene | |
| source/gameengine/Converter/KX_BlenderSceneConverter.cpp | ||
|---|---|---|
| 1545 | This code could be a lot more readable with a typedef for std::map<Material *, BL_Material *>. It looks like this new typedef could also clean up the MaterialCache typedef in the header file. | |
| 1550 | I don't see much use for assigning this to a temporary variable, we might as well just do if (matit->second == materialToRemove) | |
| 1559 | Comments for RemoveCachedMaterial() apply here too. | |
| source/gameengine/Converter/KX_BlenderSceneConverter.cpp | ||
|---|---|---|
| 1542–1568 | Duplicate code always makes me nervous. Can we use templating here to reduce this to one function? | |
| source/gameengine/Converter/KX_BlenderSceneConverter.h | ||
| 59–60 | I'm not sure why we needed to rename MaterialCache and PolyMaterialCache. I would prefer the typedefs for map<material *, BL_Material *> and map<Material *, RAS_IPolyMaterial *> be something different and keep the original typedefs the same. Something like MaterialCacheEntry, MaterialCacheValue, or even something like BlenderMaterialMap (and BlenderPolyMaterialMap) might be better. | |
| 74–75 | If you don't rename the typedefs, then this code does not need to be changed. The fewer changes a patch makes, the better. | |
Comment Actions
If I remember correctly, the odd thing that made me stop this was that once you clear the caches, you still end up having a stale KX_Scene* pointer in the keys of both maps ("other" in LinkBlendFile was the source of this). That had no nasty side effects because that key was never referenced again but for each libload a new stale key is added - which is a leak in itself, even if very small.
Comment Actions
PS. I noticed that by printing on the console the amount of keys in the maps and running LibLoad/LibFree in sequence. You should see that the number always increases.
| source/gameengine/Converter/KX_BlenderSceneConverter.cpp | ||
|---|---|---|
| 1542–1568 | So, in this case typedef are useless. | |
Comment Actions
I am starting to wonder if this patch is even correct. We remove the scene (and all of it's materials) from the material caches around lines 1052~1058. I think the question that needs to be answered is: why do we still have dangling pointers to materials? Are these materials being associated with the wrong scene in the material cache?
Comment Actions
That's the way to go. Merging the caches is logically sound and it get rids of the pointer problems ("other" included).
| source/gameengine/Converter/KX_BlenderSceneConverter.cpp | ||
|---|---|---|
| 1545 | The code would be more idiomatic but not more readable - typedef hides the value type from the use site and you end up having no idea what the heck is in that typedef'd thing until the IDE tells you. Unfortunately i'm too old to start a C++ revolution. | |
| 1550 | It was an excess of clarity. After spending a few days with the eminently obscure bge routines my immune system reacted making me write the more blatantly crystal clear code I could. It might be a little too much :D. | |
Comment Actions
This patch is looking nice and small now. :)
Have you run this through something like Valgrind's memcheck?
| source/gameengine/Converter/KX_BlenderSceneConverter.cpp | ||
|---|---|---|
| 1456 | you could avoid 2x lookups here (m_mat_cache[from]), not sure exactly how good C++ optimizers are but dont think they de-duplicate lookups. | |
| source/gameengine/Converter/KX_BlenderSceneConverter.cpp | ||
|---|---|---|
| 1455 | This is really a minor nit-pick at this stage, but we should use similar naming conventions as the rest of this code, which would make matCachedFromIt -> mat_cached_from_it. Same for polyMatCachedFromIt. If you wanted shorter names, you could probably do mat_cache_it, poly_mat_cache_it. | |
Comment Actions
Looks good to me. Make sure to have a good commit message as the current summary wont work well.