Crash "NotCoreThead" about regoth-bs HOT 7 OPEN

I assume this was introduced when I added multithreaded bulk-caching in commit 468360f.

If you comment out the line here, does it still crash?

It might worked for me be because I'm using the "low-fidelity" render-pipeline of bsf...

from regoth-bs.

EDIT: it is 74b53b0.
EDIT2: Looks like BsMaterial might have some dependency on running on the Core Thread
EDIT3: Yeah, I dont know. I guess for some CoreObject deconstructor is called, but I dont see where that happens for us.
On another note, I have a problem in the previous commits where I only see some black texture :D
No, that does not appear to be it.
I will try to find out which commit causes this, probably easier to go from there.

from regoth-bs.

By @mworchel in gitter:

void VisualSkeletalAnimation::destroyClonedMaterials()
{
  for (bs::HMaterial m : mClonedMaterials)
  {
    m->destroy();
  }
  mClonedMaterials.clear();
}

The m->destroy(); is the cause for the crash
Don't know why but it seems that the material core object is not deleted on the core thread. I would just skip the explicit destroy call and just clear the list of cloned materials. bs::HMaterial does reference counting anyway and if the material is not referenced anymore, it should be deleted...I hope

from regoth-bs.

Hi, thanks for the pointer, I've been trying to make this work on Friday.

To me, the VisualSkeletalAnimation::destroyClonedMaterials() method seems a bit weird using a member field. It seems to work for me with a bit of refactoring:

  void VisualSkeletalAnimation::cloneMaterials()
  {
    // Access the original materials for cloning
    const auto& materials = mMesh->getMaterials();

    bs::Vector<bs::HMaterial> clonedMaterials(materials.size());
    for (bs::UINT32 i = 0; i < (bs::UINT32)clonedMaterials.size(); i++)
    {
      clonedMaterials[i] = materials[i]->clone();
    }

    mSubRenderable->setMaterials(clonedMaterials);
  }

NOTE: I don't know the code or the libraries it's using very well, but I don't see why materials need to be destroyed, since this should be done by HRenderable, and I also don't see why they need to be memorized in VisualSkeletalAnimation

from regoth-bs.

I'm not sure whether resources are destroyed when they're not used anymore. If the visual changes meshes, materials would have to be cloned again. If we don't get the clones from the previous mesh deleted, we would have made us a memory leak.

If the cloned materials are really destroyed because of ref-counting, then destroying them manually is certainly wrong.

from regoth-bs.

HMaterial is a typedef of ResourceHandle<Material> which is in turn a typedef for TResourceHandle<Material, WeakHandle=false>. This class derives from TResourceHandleBase<false> and calls TResourceHandleBase<false>::releaseRef() in the destructor which is defined as

void releaseRef()
{
	if (mData)
	{
		std::uint32_t refCount = mData->mRefCount.fetch_sub(1, std::memory_order_release);

		if (refCount == 1)
		{
			std::atomic_thread_fence(std::memory_order_acquire);
			destroy();
		}
	}
};

So essentially it also calls destroy only that it has this atomic_thread_fence call which I'm not aware of the details. From the looks, this means no memory leaks should be introduced by PR #114 .

from regoth-bs.

Thanks Markus, makes sense, and the destructor probably gets called in this method:

template<bool Core>
void TRenderable<Core>::setMaterials(const Vector<MaterialType>& materials)
{
	UINT32 numMaterials = (UINT32)mMaterials.size();
	UINT32 min = std::min(numMaterials, (UINT32)materials.size());

	for (UINT32 i = 0; i < min; i++)
		mMaterials[i] = materials[i];

	for (UINT32 i = min; i < numMaterials; i++)
		mMaterials[i] = nullptr;

	_markDependenciesDirty();
	_markResourcesDirty();
	_markCoreDirty();
}

More precisely:

for (UINT32 i = min; i < numMaterials; i++)
	mMaterials[i] = nullptr;

from regoth-bs.