AtlasTexChunk::generateCopy() bugged in 1.0.3
by Tom Spilman · in Torque Game Engine Advanced · 10/27/2007 (6:24 pm) · 3 replies
In AtlasTexChunk::generateCopy() you find the following:
In VC8 (it *may* be different in other compilers) GBitmap = GBitmap does not call the copy constructor! This doesn't take a copy of all the GBitmap members, but just assigns them to the new GBitmap. Later on this causes the AtlasTexChunk destructor to crash freeing already freed GBitmap members.
The fix is to add a proper GBitmap::copy() function and making the = operator protected to keep people from doing these things accidentally. I did the following in GBitmap.cpp.
If you want to be really thourogh you should block the = operator. Doing copies of potentially large objects like GBitmaps via an assignment can hide bugs. I tend to personally keep = operators for math classes and other small types like ColorF(). You wouldn't copy a Player via an assignment operator, so why GBitmap?
The fix is to make the operator private/protected. I did this by adding it to the end of GBitmap...
Now when you rebuild you'll get a few errors from from Atlas. First fix AtlasTexChunk::generateCopy()...
Next you'll find this in atlasGenerateTextureTOCFromTiles...
... actually no their not really "transferred"... both bitmaps point at the same copy of the internally allocated members like pBits! So what happens when the second GBitmap is deleted? That's right we have a crash. I fixed it like so...
That fixes up the crashes i was experiencing building a .atlas in a TGEA debug EXE.
atc->bitmap = new GBitmap[layerCount];
for(S32 layer=0; layer<layerCount; layer++)
{
if(bitmap[layer].sgAlreadyConverted)
bitmap[layer].convertToBGRx(true);
atc->bitmap[layer] = bitmap[layer]; // This isn't what you think it is!
}In VC8 (it *may* be different in other compilers) GBitmap = GBitmap does not call the copy constructor! This doesn't take a copy of all the GBitmap members, but just assigns them to the new GBitmap. Later on this causes the AtlasTexChunk destructor to crash freeing already freed GBitmap members.
The fix is to add a proper GBitmap::copy() function and making the = operator protected to keep people from doing these things accidentally. I did the following in GBitmap.cpp.
// Replace the copy constructor with this!
GBitmap::GBitmap(const GBitmap& rCopy)
: pBits(NULL),
pPalette(NULL)
{
copy( rCopy );
}
// Added this...
void GBitmap::copy(const GBitmap& rCopy)
{
AssertFatal(rCopy.pPalette == NULL, "Cant copy bitmaps with palettes");
internalFormat = rCopy.internalFormat;
byteSize = rCopy.byteSize;
SAFE_DELETE_ARRAY( pBits );
pBits = new U8[byteSize];
dMemcpy(pBits, rCopy.pBits, byteSize);
// preserveSize = rCopy.preserveSize;
width = rCopy.width;
height = rCopy.height;
bytesPerPixel = rCopy.bytesPerPixel;
numMipLevels = rCopy.numMipLevels;
dMemcpy(mipLevelOffsets, rCopy.mipLevelOffsets, sizeof(mipLevelOffsets));
SAFE_DELETE_ARRAY( pPalette );
sgAlreadyConverted = rCopy.sgAlreadyConverted;
}If you want to be really thourogh you should block the = operator. Doing copies of potentially large objects like GBitmaps via an assignment can hide bugs. I tend to personally keep = operators for math classes and other small types like ColorF(). You wouldn't copy a Player via an assignment operator, so why GBitmap?
The fix is to make the operator private/protected. I did this by adding it to the end of GBitmap...
private:
bool _writePNG(Stream& stream, const U32, const U32, const U32) const;
// Force usage of copy().
GBitmap& operator = ( const GBitmap& ) {}
static const U32 csFileVersion;
};Now when you rebuild you'll get a few errors from from Atlas. First fix AtlasTexChunk::generateCopy()...
atc->bitmap = new GBitmap[layerCount];
for(S32 layer=0; layer<layerCount; layer++)
{
if(bitmap[layer].sgAlreadyConverted)
bitmap[layer].convertToBGRx(true);
atc->bitmap[layer].copy( bitmap[layer] ); // Easier to understand now, huh?
}Next you'll find this in atlasGenerateTextureTOCFromTiles...
// Note that this copies the _pointers_ so we don't have to worry
// about leaking bmp's resources - they're transferred to atc->bitmap[0].
atc->bitmap[0] = *bmp;... actually no their not really "transferred"... both bitmaps point at the same copy of the internally allocated members like pBits! So what happens when the second GBitmap is deleted? That's right we have a crash. I fixed it like so...
// Although we're not going to use bmp further we still take a copy
// of it. This could be made more optimal with a swipeBits() function.
atc->bitmap[0].copy( *bmp );
delete bmp;That fixes up the crashes i was experiencing building a .atlas in a TGEA debug EXE.
About the author
Tom is a programmer and co-owner of Sickhead Games, LLC.
#2
10/28/2007 (4:01 am)
Wow - that's wacky. I wonder what C++ semantic is causing blah = bar not to use a copy constructor when it did under VC7?
#3
Copy constructors are only called when your creating a new object.
Assignment operators are only called when assigning a value between already constructed objects.
A copy constructor (both the default and overloaded) is never called to do an assignment between two allocated objects... on a standards compliant compiler.
I think the confusion comes from a combination of older copies of Visual C++ doing non-standard things and the similar behavior of the default copy constructor and the default assignment operator.
Regardless... i'd argue that we shouldn't be using assignment operators for these large objects that allocate members on the heap. Without looking at the class to see if the operator is implemented it's impossible to tell if A = B allocates new members or copies pointers.
10/28/2007 (9:28 am)
Well... it did seem strange to me at first, so i decided to look this up. It seems i need to go back to C++ school because the standard is clear and obvious...Copy constructors are only called when your creating a new object.
Assignment operators are only called when assigning a value between already constructed objects.
A copy constructor (both the default and overloaded) is never called to do an assignment between two allocated objects... on a standards compliant compiler.
MyClass a; // copy constructor MyClass b(a); // i think this is the confusion... this is a copy constructor. MyClass c = a; // here both classes are already allocated... you get an assignment operator. MyClass d; d = a;
I think the confusion comes from a combination of older copies of Visual C++ doing non-standard things and the similar behavior of the default copy constructor and the default assignment operator.
Regardless... i'd argue that we shouldn't be using assignment operators for these large objects that allocate members on the heap. Without looking at the class to see if the operator is implemented it's impossible to tell if A = B allocates new members or copies pointers.
Associate Tom Spilman
Sickhead Games
if(dds) { atc->dds = new DDSFile[layerCount]; for(S32 layer=0; layer<layerCount; layer++) atc->dds[layer] = dds[layer]; // Hello bug? }I believe this suffers from the same issue as GBitmap... DDSFile does not have a assignment operator. Someone should fix that... i'm done for the night. ;)