diff --git a/android/filament-android/src/main/cpp/RenderableManager.cpp b/android/filament-android/src/main/cpp/RenderableManager.cpp index 4f38e46f82d..d9b56e02fb6 100644 --- a/android/filament-android/src/main/cpp/RenderableManager.cpp +++ b/android/filament-android/src/main/cpp/RenderableManager.cpp @@ -459,6 +459,13 @@ Java_com_google_android_filament_RenderableManager_nSetMaterialInstanceAt(JNIEnv materialInstance); } +extern "C" JNIEXPORT void JNICALL +Java_com_google_android_filament_RenderableManager_nClearMaterialInstanceAt(JNIEnv*, jclass, + jlong nativeRenderableManager, jint i, jint primitiveIndex) { + RenderableManager *rm = (RenderableManager *) nativeRenderableManager; + rm->clearMaterialInstanceAt((RenderableManager::Instance) i, (size_t) primitiveIndex); +} + extern "C" JNIEXPORT jlong JNICALL Java_com_google_android_filament_RenderableManager_nGetMaterialInstanceAt(JNIEnv*, jclass, jlong nativeRenderableManager, jint i, jint primitiveIndex) { diff --git a/android/filament-android/src/main/java/com/google/android/filament/RenderableManager.java b/android/filament-android/src/main/java/com/google/android/filament/RenderableManager.java index 0da9a0d6e79..81053197c55 100644 --- a/android/filament-android/src/main/java/com/google/android/filament/RenderableManager.java +++ b/android/filament-android/src/main/java/com/google/android/filament/RenderableManager.java @@ -869,6 +869,13 @@ public void setMaterialInstanceAt(@EntityInstance int i, @IntRange(from = 0) int nSetMaterialInstanceAt(mNativeObject, i, primitiveIndex, materialInstance.getNativeObject()); } + /** + * Clears the material instance for the given primitive. + */ + public void clearMaterialInstanceAt(@EntityInstance int i, @IntRange(from = 0) int primitiveIndex) { + nClearMaterialInstanceAt(mNativeObject, i, primitiveIndex); + } + /** * Creates a MaterialInstance Java wrapper object for a particular material instance. */ @@ -1012,6 +1019,7 @@ public long getNativeObject() { private static native void nGetAxisAlignedBoundingBox(long nativeRenderableManager, int i, float[] center, float[] halfExtent); private static native int nGetPrimitiveCount(long nativeRenderableManager, int i); private static native void nSetMaterialInstanceAt(long nativeRenderableManager, int i, int primitiveIndex, long nativeMaterialInstance); + private static native void nClearMaterialInstanceAt(long nativeRenderableManager, int i, int primitiveIndex); private static native long nGetMaterialInstanceAt(long nativeRenderableManager, int i, int primitiveIndex); private static native void nSetGeometryAt(long nativeRenderableManager, int i, int primitiveIndex, int primitiveType, long nativeVertexBuffer, long nativeIndexBuffer, int offset, int count); private static native void nSetBlendOrderAt(long nativeRenderableManager, int i, int primitiveIndex, int blendOrder); diff --git a/filament/include/filament/RenderableManager.h b/filament/include/filament/RenderableManager.h index 1ca238352b9..82d4ace2624 100644 --- a/filament/include/filament/RenderableManager.h +++ b/filament/include/filament/RenderableManager.h @@ -798,6 +798,13 @@ class UTILS_PUBLIC RenderableManager : public FilamentAPI { void setMaterialInstanceAt(Instance instance, size_t primitiveIndex, MaterialInstance const* UTILS_NONNULL materialInstance); + /** + * Clear the MaterialInstance for the given primitive. + * @param instance Renderable's instance + * @param primitiveIndex Primitive index + */ + void clearMaterialInstanceAt(Instance instance, size_t primitiveIndex); + /** * Retrieves the material instance that is bound to the given primitive. */ diff --git a/filament/src/RenderPass.cpp b/filament/src/RenderPass.cpp index 5b9d94613ea..485081fdd56 100644 --- a/filament/src/RenderPass.cpp +++ b/filament/src/RenderPass.cpp @@ -704,11 +704,22 @@ RenderPass::Command* RenderPass::generateCommandsImpl(RenderPass::CommandTypeFla */ for (auto const& primitive: primitives) { FMaterialInstance const* const mi = primitive.getMaterialInstance(); - FMaterial const* const ma = mi->getMaterial(); + if (UTILS_UNLIKELY(!mi)) { + // This can happen because RenderPrimitives can be initialized with + // a null MaterialInstance. In this case, skip the primitive. + if constexpr (isColorPass) { + curr->key = uint64_t(Pass::SENTINEL); + ++curr; + } + curr->key = uint64_t(Pass::SENTINEL); + ++curr; + continue; + } // TODO: we should disable the SKN variant if this primitive doesn't have either // skinning or morphing. + FMaterial const* const ma = mi->getMaterial(); cmd.info.mi = mi; cmd.info.rph = primitive.getHwHandle(); cmd.info.vbih = primitive.getVertexBufferInfoHandle(); diff --git a/filament/src/RenderableManager.cpp b/filament/src/RenderableManager.cpp index 87ea0d0e462..e301ab0ff3a 100644 --- a/filament/src/RenderableManager.cpp +++ b/filament/src/RenderableManager.cpp @@ -113,6 +113,10 @@ void RenderableManager::setMaterialInstanceAt(Instance instance, downcast(this)->setMaterialInstanceAt(instance, 0, primitiveIndex, downcast(materialInstance)); } +void RenderableManager::clearMaterialInstanceAt(Instance instance, size_t primitiveIndex) { + downcast(this)->clearMaterialInstanceAt(instance, 0, primitiveIndex); +} + MaterialInstance* RenderableManager::getMaterialInstanceAt( Instance instance, size_t primitiveIndex) const noexcept { return downcast(this)->getMaterialInstanceAt(instance, 0, primitiveIndex); diff --git a/filament/src/components/RenderableManager.cpp b/filament/src/components/RenderableManager.cpp index 8dfd56b449c..48ff360b694 100644 --- a/filament/src/components/RenderableManager.cpp +++ b/filament/src/components/RenderableManager.cpp @@ -792,10 +792,10 @@ void FRenderableManager::destroyComponentPrimitives( void FRenderableManager::setMaterialInstanceAt(Instance instance, uint8_t level, size_t primitiveIndex, FMaterialInstance const* mi) { + assert_invariant(mi); if (instance) { Slice& primitives = getRenderPrimitives(instance, level); - if (primitiveIndex < primitives.size()) { - assert_invariant(mi); + if (primitiveIndex < primitives.size() && mi) { FMaterial const* material = mi->getMaterial(); // we want a feature level violation to be a hard error (exception if enabled, or crash) @@ -819,6 +819,16 @@ void FRenderableManager::setMaterialInstanceAt(Instance instance, uint8_t level, } } +void FRenderableManager::clearMaterialInstanceAt(Instance instance, uint8_t level, + size_t primitiveIndex) { + if (instance) { + Slice& primitives = getRenderPrimitives(instance, level); + if (primitiveIndex < primitives.size()) { + primitives[primitiveIndex].setMaterialInstance(nullptr); + } + } +} + MaterialInstance* FRenderableManager::getMaterialInstanceAt( Instance instance, uint8_t level, size_t primitiveIndex) const noexcept { if (instance) { diff --git a/filament/src/components/RenderableManager.h b/filament/src/components/RenderableManager.h index d774996f275..521dcb260c1 100644 --- a/filament/src/components/RenderableManager.h +++ b/filament/src/components/RenderableManager.h @@ -204,6 +204,7 @@ class FRenderableManager : public RenderableManager { size_t getPrimitiveCount(Instance instance, uint8_t level) const noexcept; void setMaterialInstanceAt(Instance instance, uint8_t level, size_t primitiveIndex, FMaterialInstance const* materialInstance); + void clearMaterialInstanceAt(Instance instance, uint8_t level, size_t primitiveIndex); MaterialInstance* getMaterialInstanceAt(Instance instance, uint8_t level, size_t primitiveIndex) const noexcept; void setGeometryAt(Instance instance, uint8_t level, size_t primitiveIndex, PrimitiveType type, FVertexBuffer* vertices, FIndexBuffer* indices, diff --git a/filament/src/details/Engine.cpp b/filament/src/details/Engine.cpp index 1b20b06ac9a..ebf21e2b36e 100644 --- a/filament/src/details/Engine.cpp +++ b/filament/src/details/Engine.cpp @@ -1154,19 +1154,9 @@ UTILS_NOINLINE bool FEngine::destroy(const FMaterial* ptr) { if (ptr == nullptr) return true; bool const success = terminateAndDestroy(ptr, mMaterials); - if (success) { + if (UTILS_LIKELY(success)) { auto pos = mMaterialInstances.find(ptr); - if (pos != mMaterialInstances.cend()) { - // ensure we've destroyed all instances before destroying the material - if (!ASSERT_PRECONDITION_NON_FATAL(pos->second.empty(), - "destroying material \"%s\" but %u instances still alive", - ptr->getName().c_str(), (*pos).second.size())) { - // We return true here, because the material has been destroyed. However, this - // leaks this material's ResourceList until the engine - // is shut down. Note that it's still possible for the MaterialInstances to be - // destroyed manually. - return true; - } + if (UTILS_LIKELY(pos != mMaterialInstances.cend())) { mMaterialInstances.erase(pos); } } @@ -1176,6 +1166,39 @@ bool FEngine::destroy(const FMaterial* ptr) { UTILS_NOINLINE bool FEngine::destroy(const FMaterialInstance* ptr) { if (ptr == nullptr) return true; + + // Check that the material instance we're destroying is not in use in the RenderableManager + // To do this, we currently need to inspect all render primitives in the RenderableManager + EntityManager const& em = mEntityManager; + FRenderableManager const& rcm = mRenderableManager; + Entity const* entities = rcm.getEntities(); + size_t const count = rcm.getComponentCount(); + for (size_t i = 0; i < count; i++) { + Entity const entity = entities[i]; + if (em.isAlive(entity)) { + RenderableManager::Instance const ri = rcm.getInstance(entity); + size_t const primitiveCount = rcm.getPrimitiveCount(ri, 0); + for (size_t j = 0; j < primitiveCount; j++) { + auto const* const mi = rcm.getMaterialInstanceAt(ri, 0, j); + if (features.engine.debug.assert_material_instance_in_use) { + FILAMENT_CHECK_PRECONDITION(mi != ptr) + << "destroying MaterialInstance \"" + << mi->getName() << "\" which is still in use by Renderable (entity=" + << entity.getId() << ", instance=" + << ri.asValue() << ", index=" << j << ")"; + } else { + if (UTILS_UNLIKELY(mi == ptr)) { + slog.e << "destroying MaterialInstance \"" + << mi->getName() << "\" which is still in use by Renderable (entity=" + << entity.getId() << ", instance=" + << ri.asValue() << ", index=" << j << ")" + << io::endl; + } + } + } + } + } + if (ptr->isDefaultInstance()) return false; auto pos = mMaterialInstances.find(ptr->getMaterial()); assert_invariant(pos != mMaterialInstances.cend()); diff --git a/filament/src/details/Engine.h b/filament/src/details/Engine.h index 7788c41e596..df76f23333c 100644 --- a/filament/src/details/Engine.h +++ b/filament/src/details/Engine.h @@ -501,6 +501,10 @@ class FEngine : public Engine { return FEngine::getActiveFeatureLevel() >= neededFeatureLevel; } + auto const& getMaterialInstanceResourceList() const noexcept { + return mMaterialInstances; + } + #if defined(__EMSCRIPTEN__) void resetBackendState() noexcept; #endif @@ -683,6 +687,13 @@ class FEngine : public Engine { struct { bool use_shadow_atlas = false; } shadows; + struct { +#ifndef NDEBUG + bool assert_material_instance_in_use = true; +#else + bool assert_material_instance_in_use = false; +#endif + } debug; } engine; struct { struct { @@ -705,7 +716,10 @@ class FEngine : public Engine { &features.backend.opengl.assert_native_window_is_valid, true }, { "engine.shadows.use_shadow_atlas", "Uses an array of atlases to store shadow maps.", - &features.engine.shadows.use_shadow_atlas, true } + &features.engine.shadows.use_shadow_atlas, true }, + { "features.engine.debug.assert_material_instance_in_use", + "Assert when a MaterialInstance is destroyed while it is in use by RenderableManager.", + &features.engine.debug.assert_material_instance_in_use, false } }}; utils::Slice getFeatureFlags() const noexcept { diff --git a/filament/src/details/Material.cpp b/filament/src/details/Material.cpp index be8ca04be2e..efa8775b9e6 100644 --- a/filament/src/details/Material.cpp +++ b/filament/src/details/Material.cpp @@ -373,6 +373,22 @@ void FMaterial::invalidate(Variant::type_t variantMask, Variant::type_t variantV void FMaterial::terminate(FEngine& engine) { + if (mDefaultMaterialInstance) { + mDefaultMaterialInstance->setDefaultInstance(false); + engine.destroy(mDefaultMaterialInstance); + mDefaultMaterialInstance = nullptr; + } + + // ensure we've destroyed all instances before destroying the material + auto const& materialInstanceResourceList = engine.getMaterialInstanceResourceList(); + auto pos = materialInstanceResourceList.find(this); + if (UTILS_LIKELY(pos != materialInstanceResourceList.cend())) { + FILAMENT_CHECK_PRECONDITION(pos->second.empty()) + << "destroying material \"" << this->getName().c_str_safe() << "\" but " + << pos->second.size() << " instances still alive."; + } + + #if FILAMENT_ENABLE_MATDBG // Unregister the material with matdbg. matdbg::DebugServer* server = engine.debug.server; @@ -383,16 +399,9 @@ void FMaterial::terminate(FEngine& engine) { destroyPrograms(engine); - if (mDefaultMaterialInstance) { - mDefaultMaterialInstance->setDefaultInstance(false); - engine.destroy(mDefaultMaterialInstance); - mDefaultMaterialInstance = nullptr; - } - - mPerViewDescriptorSetLayout.terminate( - engine.getDescriptorSetLayoutFactory(), engine.getDriverApi()); - mDescriptorSetLayout.terminate( - engine.getDescriptorSetLayoutFactory(), engine.getDriverApi()); + DriverApi& driver = engine.getDriverApi(); + mPerViewDescriptorSetLayout.terminate(engine.getDescriptorSetLayoutFactory(), driver); + mDescriptorSetLayout.terminate(engine.getDescriptorSetLayoutFactory(), driver); } void FMaterial::compile(CompilerPriorityQueue priority, diff --git a/filament/src/details/Skybox.cpp b/filament/src/details/Skybox.cpp index 6526c5a2970..5e2e78ef702 100644 --- a/filament/src/details/Skybox.cpp +++ b/filament/src/details/Skybox.cpp @@ -153,13 +153,13 @@ FMaterial const* FSkybox::createMaterial(FEngine& engine) { void FSkybox::terminate(FEngine& engine) noexcept { // use Engine::destroy because FEngine::destroy is inlined Engine& e = engine; - e.destroy(mSkyboxMaterialInstance); e.destroy(mSkybox); + e.destroy(mSkyboxMaterialInstance); engine.getEntityManager().destroy(mSkybox); - mSkyboxMaterialInstance = nullptr; mSkybox = {}; + mSkyboxMaterialInstance = nullptr; } void FSkybox::setLayerMask(uint8_t select, uint8_t values) noexcept { diff --git a/libs/filamentapp/src/Cube.cpp b/libs/filamentapp/src/Cube.cpp index a8027b6953b..4283aa88b04 100644 --- a/libs/filamentapp/src/Cube.cpp +++ b/libs/filamentapp/src/Cube.cpp @@ -140,12 +140,15 @@ void Cube::mapAabb(filament::Engine& engine, filament::Box const& box) { Cube::~Cube() { mEngine.destroy(mVertexBuffer); mEngine.destroy(mIndexBuffer); - mEngine.destroy(mMaterialInstanceSolid); - mEngine.destroy(mMaterialInstanceWireFrame); + // We don't own the material, only instances mEngine.destroy(mSolidRenderable); mEngine.destroy(mWireFrameRenderable); + // material instances must be destroyed after the renderables + mEngine.destroy(mMaterialInstanceSolid); + mEngine.destroy(mMaterialInstanceWireFrame); + utils::EntityManager& em = utils::EntityManager::get(); em.destroy(mSolidRenderable); em.destroy(mWireFrameRenderable); diff --git a/libs/gltfio/src/FilamentAsset.cpp b/libs/gltfio/src/FilamentAsset.cpp index 161ae031955..4940766e1df 100644 --- a/libs/gltfio/src/FilamentAsset.cpp +++ b/libs/gltfio/src/FilamentAsset.cpp @@ -37,12 +37,8 @@ FFilamentAsset::~FFilamentAsset() { // Free transient load-time data if they haven't been freed yet. releaseSourceData(); - // Destroy all instance objects. Instance entities / components are - // destroyed later in this method because they are owned by the asset - // (except for the root of the instance). for (FFilamentInstance* instance : mInstances) { mEntityManager->destroy(instance->mRoot); - delete instance; } delete mWireframe; @@ -76,6 +72,12 @@ FFilamentAsset::~FFilamentAsset() { } } + // FilamentInstances need to be destroyed after the renderables have been destroyed + // so that there are no dangling MaterialInstance around + for (FFilamentInstance* instance : mInstances) { + delete instance; + } + for (auto vb : mVertexBuffers) { mEngine->destroy(vb); } diff --git a/libs/iblprefilter/src/IBLPrefilterContext.cpp b/libs/iblprefilter/src/IBLPrefilterContext.cpp index 9e889f36a76..0ac10e50711 100644 --- a/libs/iblprefilter/src/IBLPrefilterContext.cpp +++ b/libs/iblprefilter/src/IBLPrefilterContext.cpp @@ -254,8 +254,8 @@ Texture* IBLPrefilterContext::EquirectangularToCubemap::operator()( const uint32_t dim = outCube->getWidth(); RenderableManager& rcm = engine.getRenderableManager(); - rcm.setMaterialInstanceAt( - rcm.getInstance(mContext.mFullScreenQuadEntity), 0, mi); + auto const ci = rcm.getInstance(mContext.mFullScreenQuadEntity); + rcm.setMaterialInstanceAt(ci, 0, mi); TextureSampler environmentSampler; environmentSampler.setMagFilter(SamplerMagFilter::LINEAR); @@ -289,6 +289,7 @@ Texture* IBLPrefilterContext::EquirectangularToCubemap::operator()( engine.destroy(rt); } + rcm.clearMaterialInstanceAt(ci, 0); engine.destroy(mi); return outCube; @@ -328,8 +329,8 @@ IBLPrefilterContext::IrradianceFilter::IrradianceFilter(IBLPrefilterContext& con mi->setParameter("sampleCount", float(mSampleCount)); RenderableManager& rcm = engine.getRenderableManager(); - rcm.setMaterialInstanceAt( - rcm.getInstance(mContext.mFullScreenQuadEntity), 0, mi); + auto const ci = rcm.getInstance(mContext.mFullScreenQuadEntity); + rcm.setMaterialInstanceAt(ci, 0, mi); RenderTarget* const rt = RenderTarget::Builder() .texture(RenderTarget::AttachmentPoint::COLOR0, mKernelTexture) @@ -340,6 +341,8 @@ IBLPrefilterContext::IrradianceFilter::IrradianceFilter(IBLPrefilterContext& con renderer->renderStandaloneView(view); + rcm.clearMaterialInstanceAt(ci, 0); + engine.destroy(rt); engine.destroy(mi); } @@ -410,8 +413,8 @@ filament::Texture* IBLPrefilterContext::IrradianceFilter::operator()( MaterialInstance* const mi = mContext.mIrradianceIntegrationMaterial->createInstance(); RenderableManager& rcm = engine.getRenderableManager(); - rcm.setMaterialInstanceAt( - rcm.getInstance(mContext.mFullScreenQuadEntity), 0, mi); + auto const ci = rcm.getInstance(mContext.mFullScreenQuadEntity); + rcm.setMaterialInstanceAt(ci, 0, mi); const uint32_t sampleCount = mSampleCount; const float linear = options.hdrLinear; @@ -455,6 +458,8 @@ filament::Texture* IBLPrefilterContext::IrradianceFilter::operator()( engine.destroy(rt); } + rcm.clearMaterialInstanceAt(ci, 0); + engine.destroy(mi); return outIrradianceTexture; @@ -533,8 +538,8 @@ IBLPrefilterContext::SpecularFilter::SpecularFilter(IBLPrefilterContext& context mi->setParameter("roughness", roughnessArray, 16); RenderableManager& rcm = engine.getRenderableManager(); - rcm.setMaterialInstanceAt( - rcm.getInstance(mContext.mFullScreenQuadEntity), 0, mi); + auto const ci = rcm.getInstance(mContext.mFullScreenQuadEntity); + rcm.setMaterialInstanceAt(ci, 0, mi); RenderTarget* const rt = RenderTarget::Builder() .texture(RenderTarget::AttachmentPoint::COLOR0, mKernelTexture) @@ -545,6 +550,8 @@ IBLPrefilterContext::SpecularFilter::SpecularFilter(IBLPrefilterContext& context renderer->renderStandaloneView(view); + rcm.clearMaterialInstanceAt(ci, 0); + engine.destroy(rt); engine.destroy(mi); } @@ -643,8 +650,8 @@ Texture* IBLPrefilterContext::SpecularFilter::operator()( MaterialInstance* const mi = mContext.mIntegrationMaterial->createInstance(); RenderableManager& rcm = engine.getRenderableManager(); - rcm.setMaterialInstanceAt( - rcm.getInstance(mContext.mFullScreenQuadEntity), 0, mi); + auto const ci = rcm.getInstance(mContext.mFullScreenQuadEntity); + rcm.setMaterialInstanceAt(ci, 0, mi); const uint32_t sampleCount = mSampleCount; const float linear = options.hdrLinear; @@ -707,6 +714,8 @@ Texture* IBLPrefilterContext::SpecularFilter::operator()( dim >>= 1; } + rcm.clearMaterialInstanceAt(ci, 0); + engine.destroy(mi); return outReflectionsTexture; diff --git a/web/filament-js/jsbindings.cpp b/web/filament-js/jsbindings.cpp index 24677902bff..296337f5c57 100644 --- a/web/filament-js/jsbindings.cpp +++ b/web/filament-js/jsbindings.cpp @@ -1100,6 +1100,7 @@ class_("RenderableManager") .function("getPrimitiveCount", &RenderableManager::getPrimitiveCount) .function("setMaterialInstanceAt", &RenderableManager::setMaterialInstanceAt, allow_raw_pointers()) + .function("clearMaterialInstanceAt", &RenderableManager::clearMaterialInstanceAt) .function("getMaterialInstanceAt", &RenderableManager::getMaterialInstanceAt, allow_raw_pointers())