Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Issue #225: Fix for memleak when loading terrain. #150

Merged
merged 4 commits into from

2 participants

@lgromanowski

Hi,
here is fix for memleak when loading terrain data.

best regards,
Lukasz

@zinnschlag
Owner

Need to think about it more. Your fix is at least incomplete, since copying and assignments will cause problems. But the whole land record code looks incorrect in the first place.

@lgromanowski

You're right, I will prepare another patch today.

@lgromanowski lgromanowski Issue #225: Correction to commit ae98904.
Changed pointer to LandData struct to simple member variable.
0e5c90d
@lgromanowski

Hi,
is patch 0e5c90d good, or should be changed?

@zinnschlag
Owner

The problem with this approach is that it increases the memory consumption a lot. If my calculations are right easily in the order of magnitude of 100MB. And most of this memory will probably never be used during a playing session.
Actually, the land data should not be part of this record at all, because you are not supposed to modify the ESM store after the loading is complete (it is only possible because some other code breaks constness rules). But that is something we really need think through better and this is not the time for it. As a quick fix, I suggest to return to the pointer approach and disable copy constructor and assignment operator (declare them privately and don't implement them). This will break the code once we start optimising the ESM store containers, but at least it won't do so silently and produces an error message during the build process isntead.

lgromanowski added some commits
@lgromanowski lgromanowski Revert "Issue #225: Correction to commit ae98904."
This reverts commit 0e5c90d3e783cd471197637ae281494de4dd0b08.
cd2789b
@lgromanowski lgromanowski Issue #225: Land struct is not copyable.
Disabled copy constructor and assignment operator in Land structure.
ea8e5cb
@lgromanowski

Copy constructor and assignment operator are now disabled for Land structure.

@zinnschlag zinnschlag merged commit ea8e5cb into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 1, 2012
  1. @lgromanowski
Commits on Apr 2, 2012
  1. @lgromanowski

    Issue #225: Correction to commit ae98904.

    lgromanowski authored
    Changed pointer to LandData struct to simple member variable.
Commits on Apr 4, 2012
  1. @lgromanowski

    Revert "Issue #225: Correction to commit ae98904."

    lgromanowski authored
    This reverts commit 0e5c90d3e783cd471197637ae281494de4dd0b08.
  2. @lgromanowski

    Issue #225: Land struct is not copyable.

    lgromanowski authored
    Disabled copy constructor and assignment operator in Land structure.
This page is out of date. Refresh to see the latest.
View
11 apps/openmw/mwrender/terrain.cpp
@@ -98,7 +98,10 @@ namespace MWRender
ESM::Land* land = mEnvironment.mWorld->getStore().lands.search(cellX, cellY);
if ( land != NULL )
{
- land->loadData();
+ if (!land->dataLoaded)
+ {
+ land->loadData();
+ }
}
//split the cell terrain into four segments
@@ -420,7 +423,11 @@ namespace MWRender
ESM::Land* land = mEnvironment.mWorld->getStore().lands.search(cellX, cellY);
if ( land != NULL )
{
- land->loadData();
+ if (!land->dataLoaded)
+ {
+ land->loadData();
+ }
+
return land->landData
->textures[y * ESM::Land::LAND_TEXTURE_SIZE + x];
}
View
18 components/esm/loadland.cpp
@@ -2,6 +2,24 @@
namespace ESM
{
+
+Land::Land()
+ : flags(0)
+ , X(0)
+ , Y(0)
+ , mEsm(NULL)
+ , hasData(false)
+ , dataLoaded(false)
+ , landData(NULL)
+{
+}
+
+Land::~Land()
+{
+ delete landData;
+}
+
+
void Land::load(ESMReader &esm)
{
mEsm = &esm;
View
8 components/esm/loadland.hpp
@@ -11,6 +11,9 @@ namespace ESM
struct Land
{
+ Land();
+ ~Land();
+
int flags; // Only first four bits seem to be used, don't know what
// they mean.
int X, Y; // Map coordinates.
@@ -77,6 +80,11 @@ struct Land
* Frees memory allocated for land data
*/
void unloadData();
+
+ private:
+ Land(const Land& land);
+ Land& operator=(const Land& land);
};
+
}
#endif
Something went wrong with that request. Please try again.