Skip to content

Commit

Permalink
UPBGE: Fix issues on LibFree/LibLoad. (#824)
Browse files Browse the repository at this point in the history
The issue #805 reported a bug relative to LibLoad and LibFree. This bug is
reproduced by using two scenes which both use meshes without material (so using
defmaterial) and the first scene LibLoad and merge the second. What is happening
is that the new created scene will contain a RAS_MaterialBucket of defmaterial
which is also used by the host scene, but defmaterial is not owned by any file
as it's a global default material.
The technique used by LibFree was to tag all ID structs (data, e.g Mesh,
Material, Object) of a Main (the file loaded) and remove them from scenes and
users. In the situation of defmaterial, this struct is never tagged and so never
removed during LibFree. This resulted in a material always existing but all the
light objects used were removed and the function GPU_material_update_lamps
called in material preparation before rendering raise an illegal memory access
as it was iterating on freed lamps.

To solve this issue we have to find a way to remove all game engine data that
were created during the conversion of a libloaded scene. And the minor task is
avoiding end up with only a part of scene materials using all present lights.

The first point is solved by the introduction of a resource base class:
BL_Resource. The goal of this class is to store an identifier of the library
source of converted data. This identifier is opaque type Bl_Resource::Library
and the function BL_Resource::Belong(Library) returns true when the passed
library identifier match the one of the resource.
The classes inheriting from BL_Resource are:
- BL_ConvertObjectInfo
- KX_Mesh
- KX_BlenderMaterial
- BL_ActionData

All these resource types are registered in BL_SceneConverter where all
registering function set the library identifier from the one passed to
BL_SceneConverter constructor. From LinkBlendFile, ConvertMeshSpecial and
ConvertScene(KX_Scene *scene) (the functions creating a scene converter)
BL_SceneConverter is created using as library identifier the Main pointer of the
current file or the loaded file, this way we also ensure that all linked data of
a file will be owned by the same library. Other than that the loading changed by
the call of the function ReloadShaders to reload all shaders of a merged scene:
the new and old shaders, to benefit of all existing lights.

Concerning LibFree, the first modification is to delay the free to the end of
the frame instead doing it instantly. To achieve it, FreeBlendFile(const
std::string& path) stores the library path to free in m_freeQueue if the library
exists and the function ProcessScheduledLibraries process the merging of the
libloads and the free of libraries, this final function is called in
KX_KetsjiEngine::NextFrame at the end of each logic frame.
The actual library free takes place in FreeBlendFileData. It iterates over all
scenes and objects, if the object convert object info belongs to the free
library the object is removed else KX_GameObject::RemoveResources is called.
Finally the materials, meshes and actions of the library are removed from scene
data slots (m_sceneSlots). KX_GameObject::RemoveResources is removing all the
meshes that belong to the library or using materials from the library, and do
the same for actions currently played.
Previously BL_ActionManager was also impacted by LibFree as it was using
bAction, now the actuator is using a action name to remove this complexity when
freeing an action.

Some modifications were introduced to ease the libfree. The class BL_ActionData
is used to wrap bAction and link the BL_ScalarInterpolator of the action. All
the actions are converted and registered during scene conversion as the cost is
negligeable.
When a RAS_Texture is free (indirectly by material free while lib free) if the
renderer is non null, then the texture unregister itself from the renderer, this
mechanism of texture users is near to be obsolete.

Shader are now recompiled when merging and freeing a library to ensure that the
lights are properly used, the function BL_Covnerter::ReloadShaders is dedicated
for and call KX_BlenderMaterial::ReloadMaterial that can reload the shader or
create a new one if it wasn't initialized.

While reviewing and testing #805, two bugs were noticed. First the animations
were not merged in the targeted scene, second the textures could be initialized
in the conversion thread whithout opengl context.

The first report is fixed by in case of scenes merging creating a converter
which aim the conversion of animations of all scenes to merge, the function
BL_ConvertActions is called with this converter and convert all actions
from a library.
The second issue is resolved by moving the call to InitTextures in
KX_BlenderMaterial::ReloadShader which is the only place called synchronously
during material conversion.

Fix issue #805.
  • Loading branch information
panzergame committed Oct 1, 2018
1 parent e9b0780 commit b225cb5
Show file tree
Hide file tree
Showing 48 changed files with 873 additions and 641 deletions.
70 changes: 70 additions & 0 deletions source/gameengine/Common/CM_Map.h
@@ -0,0 +1,70 @@
/*
* ***** BEGIN GPL LICENSE BLOCK *****
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License
* as published by the Free Software Foundation; either version 2
* of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program; if not, write to the Free Software Foundation,
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*
* Contributor(s): Tristan Porteries.
*
* ***** END GPL LICENSE BLOCK *****
*/

/** \file CM_List.h
* \ingroup common
*/

#ifndef __CM_LIST_H__
#define __CM_LIST_H__

#include <map>
#include <unordered_map>

template <class Item, class Key, class ... Args>
inline const Item& CM_MapGetItemNoInsert(const std::map<Key, Item, Args ...>& map, const Key& key, const Item defaultItem = nullptr)
{
const typename std::map<Key, Item, Args ...>::iterator it = map.find(key);
if (it != map.end()) {
return it->second;
}
return defaultItem;
}

template <class Item, class Key, class ... Args>
inline const Item& CM_MapGetItemNoInsert(const std::unordered_map<Key, Item, Args ...>& map, const Key& key, const Item defaultItem = nullptr)
{
const typename std::map<Key, Item, Args ...>::iterator it = map.find(key);
if (it != map.end()) {
return it->second;
}
return defaultItem;
}

template <class Map, class Item>
inline bool CM_MapRemoveIfItemFound(Map& map, const Item& item)
{
bool found = false;
for (typename Map::iterator it = map.begin(); it != map.end();) {
if (it->second == item) {
it = map.erase(it);
found = true;
}
else {
++it;
}
}

return found;
}

#endif // __CM_LIST_H__
1 change: 1 addition & 0 deletions source/gameengine/Common/CMakeLists.txt
Expand Up @@ -48,6 +48,7 @@ set(SRC
CM_Clock.h
CM_Format.h
CM_List.h
CM_Map.h
CM_Message.h
CM_RefCount.h
CM_Template.h
Expand Down
26 changes: 12 additions & 14 deletions source/gameengine/Converter/BL_ActionActuator.cpp
Expand Up @@ -64,7 +64,7 @@ BL_ActionActuator::BL_ActionActuator(SCA_IObject *gameobj,
const std::string& framepropname,
float starttime,
float endtime,
struct bAction *action,
const std::string& actionName,
short playtype,
short blend_mode,
short blendin,
Expand All @@ -85,7 +85,7 @@ BL_ActionActuator::BL_ActionActuator(SCA_IObject *gameobj,
m_priority(priority),
m_layer(layer),
m_ipo_flags(ipo_flags),
m_action(action),
m_actionName(actionName),
m_propname(propname),
m_framepropname(framepropname)
{
Expand Down Expand Up @@ -121,7 +121,7 @@ bool BL_ActionActuator::Update(double curtime)
float end = m_endframe;

// If we don't have an action, we can't do anything
if (!m_action) {
if (m_actionName.empty()) {
return false;
}

Expand Down Expand Up @@ -174,7 +174,7 @@ bool BL_ActionActuator::Update(double curtime)
}

// If a different action is playing, we've been overruled and are no longer active
if (obj->GetCurrentAction(m_layer) != m_action && !obj->IsActionDone(m_layer)) {
if (obj->GetCurrentActionName(m_layer) != m_actionName && !obj->IsActionDone(m_layer)) {
m_flag &= ~ACT_FLAG_ACTIVE;
}

Expand Down Expand Up @@ -229,8 +229,8 @@ bool BL_ActionActuator::Update(double curtime)
}
else if ((m_flag & ACT_FLAG_ACTIVE) && negativeEvent) {
m_localtime = obj->GetActionFrame(m_layer);
bAction *curr_action = obj->GetCurrentAction(m_layer);
if (curr_action && curr_action != m_action) {
const std::string curr_action = obj->GetCurrentActionName(m_layer);
if (!curr_action.empty() && curr_action != m_actionName) {
// Someone changed the action on us, so we wont mess with it
// Hopefully there wont be too many problems with two actuators using
// the same action...
Expand Down Expand Up @@ -262,7 +262,7 @@ bool BL_ActionActuator::Update(double curtime)
// Convert into a play action and play back to the beginning
float temp = end;
end = start;
start = curr_action ? obj->GetActionFrame(m_layer) : temp;
start = curr_action.empty() ? temp : obj->GetActionFrame(m_layer);
Play(obj, start, end, BL_Action::ACT_MODE_PLAY);
m_flag |= ACT_FLAG_PLAY_END;
break;
Expand All @@ -287,7 +287,7 @@ void BL_ActionActuator::DecLink()
bool BL_ActionActuator::Play(KX_GameObject *obj, float start, float end, short mode)
{
const short blendmode = (m_blendmode == ACT_ACTION_ADD) ? BL_Action::ACT_BLEND_ADD : BL_Action::ACT_BLEND_BLEND;
return obj->PlayAction(m_action->id.name + 2, start, end, m_layer, m_priority, m_blendin, mode, m_layer_weight, m_ipo_flags, 1.0f, blendmode);
return obj->PlayAction(m_actionName, start, end, m_layer, m_priority, m_blendin, mode, m_layer_weight, m_ipo_flags, 1.0f, blendmode);
}

#ifdef WITH_PYTHON
Expand Down Expand Up @@ -341,7 +341,7 @@ PyAttributeDef BL_ActionActuator::Attributes[] = {
PyObject *BL_ActionActuator::pyattr_get_action(EXP_PyObjectPlus *self_v, const EXP_PYATTRIBUTE_DEF *attrdef)
{
BL_ActionActuator *self = static_cast<BL_ActionActuator *>(self_v);
return PyUnicode_FromString(self->GetAction() ? self->GetAction()->id.name + 2 : "");
return PyUnicode_FromStdString(self->m_actionName);
}

int BL_ActionActuator::pyattr_set_action(EXP_PyObjectPlus *self_v, const EXP_PYATTRIBUTE_DEF *attrdef, PyObject *value)
Expand All @@ -353,18 +353,16 @@ int BL_ActionActuator::pyattr_set_action(EXP_PyObjectPlus *self_v, const EXP_PYA
return PY_SET_ATTR_FAIL;
}

bAction *action = nullptr;
std::string val = _PyUnicode_AsString(value);

if (val != "") {
action = (bAction *)self->GetLogicManager()->GetActionByName(val);
if (!action) {
if (!val.empty()) {
if (!self->GetLogicManager()->GetActionByName(val)) {
PyErr_SetString(PyExc_ValueError, "actuator.action = val: Action Actuator, action not found!");
return PY_SET_ATTR_FAIL;
}
}

self->SetAction(action);
self->m_actionName = val;
return PY_SET_ATTR_SUCCESS;

}
Expand Down
7 changes: 2 additions & 5 deletions source/gameengine/Converter/BL_ActionActuator.h
Expand Up @@ -45,7 +45,7 @@ class BL_ActionActuator : public SCA_IActuator
const std::string& framepropname,
float starttime,
float endtime,
struct bAction *action,
const std::string& actionName,
short playtype,
short blend_mode,
short blendin,
Expand All @@ -63,9 +63,6 @@ class BL_ActionActuator : public SCA_IActuator
void SetLocalTime(float curtime);
void ResetStartTime(float curtime);

bAction* GetAction() { return m_action; }
void SetAction(bAction* act) { m_action= act; }

virtual void DecLink();

bool Play(KX_GameObject *obj, float start, float end, short mode);
Expand Down Expand Up @@ -114,7 +111,7 @@ class BL_ActionActuator : public SCA_IActuator
short m_priority;
short m_layer;
short m_ipo_flags;
struct bAction *m_action;
std::string m_actionName;
std::string m_propname;
std::string m_framepropname;
};
Expand Down
38 changes: 38 additions & 0 deletions source/gameengine/Converter/BL_ActionData.cpp
@@ -0,0 +1,38 @@
#include "BL_ActionData.h"

extern "C" {
# include "DNA_action_types.h"
# include "DNA_anim_types.h"
# include "BKE_fcurve.h"
}

BL_ActionData::BL_ActionData(bAction *action)
:m_action(action)
{
for (FCurve *fcu = (FCurve *)action->curves.first; fcu; fcu = fcu->next) {
if (fcu->rna_path) {
m_interpolators.emplace_back(fcu);
}
}
}

std::string BL_ActionData::GetName() const
{
return m_action->id.name + 2;
}

bAction *BL_ActionData::GetAction() const
{
return m_action;
}

BL_ScalarInterpolator *BL_ActionData::GetScalarInterpolator(const std::string& rna_path, int array_index)
{
for (BL_ScalarInterpolator &interp : m_interpolators) {
FCurve *fcu = interp.GetFCurve();
if (array_index == fcu->array_index && rna_path == fcu->rna_path) {
return &interp;
}
}
return nullptr;
}
31 changes: 31 additions & 0 deletions source/gameengine/Converter/BL_ActionData.h
@@ -0,0 +1,31 @@
#ifndef __BL_ACTION_DATA_H__
#define __BL_ACTION_DATA_H__

#include "BL_Resource.h"
#include "BL_ScalarInterpolator.h"

#include <string>

struct bAction;

/** Data related to a blender animation.
*/
class BL_ActionData : public BL_Resource
{
private:
/// The blender action.
bAction *m_action;
/// The interpolators for each curve (FCurve) of the action.
std::vector<BL_ScalarInterpolator> m_interpolators;

public:
BL_ActionData(bAction *action);
~BL_ActionData() = default;

std::string GetName() const;
bAction *GetAction() const;

BL_ScalarInterpolator *GetScalarInterpolator(const std::string& rna_path, int array_index);
};

#endif // __BL_ACTION_DATA_H__
30 changes: 21 additions & 9 deletions source/gameengine/Converter/BL_BlenderDataConversion.cpp
Expand Up @@ -112,6 +112,7 @@
#include "BL_ConvertProperties.h"
#include "BL_ConvertObjectInfo.h"
#include "BL_ArmatureObject.h"
#include "BL_ActionData.h"

#include "LA_SystemCommandLine.h"

Expand Down Expand Up @@ -524,7 +525,10 @@ KX_Mesh *BL_ConvertMesh(Mesh *me, Object *blenderobj, KX_Scene *scene, BL_SceneC

dm->release(dm);

// Needed for python scripting.
scene->GetLogicManager()->RegisterMeshName(meshobj->GetName(), meshobj);
converter.RegisterGameMesh(meshobj, me);

return meshobj;
}

Expand Down Expand Up @@ -729,6 +733,23 @@ RAS_Deformer *BL_ConvertDeformer(KX_GameObject *object, KX_Mesh *meshobj)
return deformer;
}

BL_ActionData *BL_ConvertAction(bAction *action, KX_Scene *scene, BL_SceneConverter& converter)
{
BL_ActionData *data = new BL_ActionData(action);
converter.RegisterActionData(data);
scene->GetLogicManager()->RegisterActionName(action->id.name + 2, data);

return data;
}

void BL_ConvertActions(KX_Scene *scene, Main *maggie, BL_SceneConverter& converter)
{
// Convert all actions and register.
for (bAction *act = (bAction *)maggie->action.first; act; act = (bAction *)act->id.next) {
BL_ConvertAction(act, scene, converter);
}
}

static void BL_CreateGraphicObjectNew(KX_GameObject *gameobj, KX_Scene *kxscene, bool isActive, PHY_IPhysicsEnvironment *phyEnv)
{
#ifdef WITH_BULLET
Expand Down Expand Up @@ -968,9 +989,6 @@ static KX_GameObject *BL_GameObjectFromBlenderObject(Object *ob, KX_Scene *kxsce
Mesh *mesh = static_cast<Mesh *>(ob->data);
KX_Mesh *meshobj = BL_ConvertMesh(mesh, ob, kxscene, converter);

// needed for python scripting
kxscene->GetLogicManager()->RegisterMeshName(meshobj->GetName(), meshobj);

if (ob->gameflag & OB_NAVMESH) {
gameobj = new KX_NavMeshObject(kxscene, KX_Scene::m_callbacks);
gameobj->AddMesh(meshobj);
Expand Down Expand Up @@ -1424,12 +1442,6 @@ void BL_ConvertBlenderObjects(struct Main *maggie,

EXP_ListValue<KX_GameObject> *logicbrick_conversionlist = new EXP_ListValue<KX_GameObject>();

// Convert actions to actionmap.
bAction *curAct;
for (curAct = (bAction *)maggie->action.first; curAct; curAct = (bAction *)curAct->id.next) {
logicmgr->RegisterActionName(curAct->id.name + 2, curAct);
}

BL_SetBlenderSceneBackground(blenderscene);

/* Let's support scene set.
Expand Down
6 changes: 6 additions & 0 deletions source/gameengine/Converter/BL_BlenderDataConversion.h
Expand Up @@ -41,10 +41,12 @@ class RAS_ICanvas;
class KX_KetsjiEngine;
class KX_Scene;
class BL_SceneConverter;
class BL_ActionData;
struct Mesh;
struct DerivedMesh;
struct Object;
struct Main;
struct bAction;

struct BL_MeshMaterial {
RAS_DisplayArray *array;
Expand All @@ -61,6 +63,10 @@ void BL_ConvertDerivedMeshToArray(DerivedMesh *dm, Mesh *me, const std::vector<B

RAS_Deformer *BL_ConvertDeformer(KX_GameObject *object, KX_Mesh *meshobj);

BL_ActionData *BL_ConvertAction(bAction *action, KX_Scene *scene, BL_SceneConverter& converter);
/// Convert all actions of a library and register them in a converter.
void BL_ConvertActions(KX_Scene *scene, Main *maggie, BL_SceneConverter& converter);

void BL_ConvertBlenderObjects(Main *maggie, KX_Scene *kxscene, KX_KetsjiEngine *ketsjiEngine,
RAS_Rasterizer *rendertools, RAS_ICanvas *canvas, BL_SceneConverter& sceneconverter,
bool alwaysUseExpandFraming, float camZoom, bool libloading);
Expand Down
4 changes: 3 additions & 1 deletion source/gameengine/Converter/BL_ConvertActuators.cpp
Expand Up @@ -222,13 +222,15 @@ void BL_ConvertActuators(const char *maggiename,
ipo_flags |= BL_Action::ACT_IPOFLAG_ADD;
}

const std::string actionName = (actact->act) ? actact->act->id.name + 2 : "";

BL_ActionActuator *tmpbaseact = new BL_ActionActuator(
gameobj,
propname,
propframe,
actact->sta,
actact->end,
actact->act,
actionName,
actact->type, // + 1, because Blender starts to count at zero,
actact->blend_mode,
actact->blendin,
Expand Down
6 changes: 6 additions & 0 deletions source/gameengine/Converter/BL_ConvertObjectInfo.cpp
@@ -0,0 +1,6 @@
#include "BL_ConvertObjectInfo.h"

BL_ConvertObjectInfo::BL_ConvertObjectInfo(Object *blendobj)
:m_blenderObject(blendobj)
{
}

0 comments on commit b225cb5

Please sign in to comment.