From 9ce63bc7d3770e64934c6646b4496e1769757722 Mon Sep 17 00:00:00 2001 From: cutealien Date: Sun, 7 May 2023 14:51:09 +0000 Subject: [PATCH] Fix SMaterialLayer operator!= and optimize operator= SMaterialLayers are now identical when both have identity matrices. Before it didn't consider them identical when one layer had set the identity matrix explicitly and the other didn't. Generally didn't matter, just caused very rarely some extra state switches in the drivers. And just as rarely had a cheaper comparison. Just seems more correct this way. operator= no longer releases texture memory which was allocated at one point. Unless explicitly requested such memory is now always released later in the destructor. This can avoid quite a few memory allocations/released in the driver. Usually not a noticeable performance difference on most platforms. But it can help avoid memory fragmentation. We instead use an extra bool now to tell if the texture memory is used. So slight increase in SMaterialLayer and SMaterial size. But I did a quick performance test and this had no negative influence here, while it did improve speed in the case where it switched between material layers using/not using texture matrices a bit. git-svn-id: svn://svn.code.sf.net/p/irrlicht/code/trunk@6488 dfc29bdd-3216-0410-991c-e03cc46cb475 --- changes.txt | 4 +++ include/SMaterialLayer.h | 51 ++++++++++++++++++++-------------- tests/material.cpp | 32 +++++++++++++++++++++ tests/tests-last-passed-at.txt | 2 +- 4 files changed, 67 insertions(+), 22 deletions(-) diff --git a/changes.txt b/changes.txt index 10027554..66e641bf 100644 --- a/changes.txt +++ b/changes.txt @@ -1,6 +1,10 @@ -------------------------- Changes in 1.9 (not yet released) +- Bugfix: SMaterialLayer::operator!= no longer returns true when comparing a layer without texture matrix with one with a set identity texture matrix. Those are the same. +- SMaterialLayer no longer releases allocated texture memory before destructor unless explicitly requested. + This can avoid constantly allocation/releasing memory in the active driver material when setting materials. + Note that it does not copy texture memory when it's unused. This is still optimized. - Add IMeshSceneNode::setNodeRegistration to allow registering MeshSceneNodes to the SceneManager per buffer instead of per node - Add SMaterialLayer::hasSetTextureMatrix and SMaterialLayer::resetTextureMatrix - Add IShaderConstantSetCallBack::OnCreate to allow earlier access to IMaterialRendererServices diff --git a/include/SMaterialLayer.h b/include/SMaterialLayer.h index 48698cd5..c8006d01 100644 --- a/include/SMaterialLayer.h +++ b/include/SMaterialLayer.h @@ -51,7 +51,8 @@ namespace video public: //! Default constructor SMaterialLayer() : Texture(0), TextureWrapU(ETC_REPEAT), TextureWrapV(ETC_REPEAT), TextureWrapW(ETC_REPEAT), - BilinearFilter(true), TrilinearFilter(false), AnisotropicFilter(0), LODBias(0), TextureMatrix(0) + BilinearFilter(true), TrilinearFilter(false), AnisotropicFilter(0), LODBias(0), + TextureMatrix(0), TextureMatrixUsed(false) { } @@ -84,26 +85,22 @@ namespace video return *this; Texture = other.Texture; - if (TextureMatrix) + if (other.TextureMatrixUsed) { - if (other.TextureMatrix) - *TextureMatrix = *other.TextureMatrix; - else + if (TextureMatrix) { - MatrixAllocator.destruct(TextureMatrix); - MatrixAllocator.deallocate(TextureMatrix); - TextureMatrix = 0; + *TextureMatrix = *other.TextureMatrix; } - } - else - { - if (other.TextureMatrix) + else { TextureMatrix = MatrixAllocator.allocate(1); MatrixAllocator.construct(TextureMatrix,*other.TextureMatrix); } - else - TextureMatrix = 0; + TextureMatrixUsed = true; + } + else + { + TextureMatrixUsed = false; } TextureWrapU = other.TextureWrapU; TextureWrapV = other.TextureWrapV; @@ -124,6 +121,12 @@ namespace video { TextureMatrix = MatrixAllocator.allocate(1); MatrixAllocator.construct(TextureMatrix,core::IdentityMatrix); + TextureMatrixUsed = true; + } + else if ( !TextureMatrixUsed ) + { + *TextureMatrix = core::IdentityMatrix; + TextureMatrixUsed = true; } return *TextureMatrix; } @@ -132,7 +135,7 @@ namespace video /** \return Texture matrix of this layer. */ const core::matrix4& getTextureMatrix() const { - if (TextureMatrix) + if (TextureMatrixUsed) return *TextureMatrix; else return core::IdentityMatrix; @@ -151,25 +154,27 @@ namespace video } else *TextureMatrix = mat; + TextureMatrixUsed = true; } //! Check if we have set a custom texture matrix //! Note that otherwise we get an IdentityMatrix as default inline bool hasSetTextureMatrix() const { - return TextureMatrix != 0; + return TextureMatrixUsed; } //! Reset texture matrix to identity matrix - //! Releases memory, which is expensive, but ver rarely useful for optimizations - void resetTextureMatrix() + /** \param releaseMemory Releases also texture memory. Otherwise done in destructor */ + void resetTextureMatrix(bool releaseMemory=true) { - if ( TextureMatrix ) + if ( TextureMatrix && releaseMemory) { MatrixAllocator.destruct(TextureMatrix); MatrixAllocator.deallocate(TextureMatrix); TextureMatrix = 0; } + TextureMatrixUsed = false; } //! Inequality operator @@ -189,8 +194,11 @@ namespace video if (different) return true; else - different |= (TextureMatrix != b.TextureMatrix) && - (!TextureMatrix || !b.TextureMatrix || (*TextureMatrix != *(b.TextureMatrix))); + { + different = (TextureMatrixUsed && b.TextureMatrixUsed && (*TextureMatrix != *b.TextureMatrix)) + || (TextureMatrixUsed && !b.TextureMatrixUsed && (*TextureMatrix != core::IdentityMatrix)) + || (!TextureMatrixUsed && b.TextureMatrixUsed && (core::IdentityMatrix != *b.TextureMatrix)); + } return different; } @@ -241,6 +249,7 @@ namespace video /** Do not access this element directly as the internal resource management has to cope with Null pointers etc. */ core::matrix4* TextureMatrix; + bool TextureMatrixUsed; // TextureMatrix memory stays until destructor even when unused to avoid unnecessary allocation/de-allocations }; } // end namespace video diff --git a/tests/material.cpp b/tests/material.cpp index 2a6dc1ae..d5b7e3bf 100644 --- a/tests/material.cpp +++ b/tests/material.cpp @@ -71,9 +71,41 @@ static bool polygonOffset(video::E_DRIVER_TYPE type) return result; } +static bool testSMaterial() +{ + irr::video::SMaterial a; + irr::video::SMaterial b; + + // Same by default? + if ( !(a == b) ) + return false; + if ( a != b ) + return false; + + // getting (creating) one texture matrix shouldn't change things + b.TextureLayer[0].getTextureMatrix(); + if ( a != b ) + return false; + + // no longer same now + b.TextureLayer[0].getTextureMatrix().setTextureScale(5.f, 0.5f); + if ( a == b ) + return false; + + return true; +} + bool material() { bool result = true; + TestWithAllDrivers(polygonOffset); + + if ( !testSMaterial() ) + { + logTestString("testSMaterial failed\n\n"); + result = false; + } + return result; } diff --git a/tests/tests-last-passed-at.txt b/tests/tests-last-passed-at.txt index f7f59afa..a9989971 100644 --- a/tests/tests-last-passed-at.txt +++ b/tests/tests-last-passed-at.txt @@ -1,4 +1,4 @@ Tests finished. 72 tests of 72 passed. Compiled as DEBUG -Test suite pass at GMT Fri May 05 18:39:44 2023 +Test suite pass at GMT Sun May 07 14:26:39 2023