[GEN][ZH] Fix drawable assignment in StealthUpdate::changeVisualDisguise()

[Mauller]

This fix was originally discovered by @helmutbuhler and split out of the original PR it was contained in.

This fix corrects the behaviour where when the drawable was not being set for a new visual disguise on a stealth unit.
The call to get the new drawable was not being handled correctly and was not updating the draw variable.
This would have resulted in a memory leak, possible use after free and unexpected behaviour with stealthing units.

• Relates [ZH] Minor memory corruption and leak fixes #716

[feliwir]

Looks good to me. We should add @helmutbuhler as Co-Author when Squash-merging

[Mauller]

Rebased with main and pushed so ready for testing

[sorcerer86pt]

Created a replay with gla where i disguised some bomb trucks and then replayed it on this pr exes

VC6 replay ok

[Mauller]

Cleaned up comment, should be good to go.

[xezon]

I removed the Bug label because there is no evidence that this was a runtime problem in the retail game.

Additionally, in a debug session with VS2022 we observed that calling

TheGameClient->destroyDrawable( draw );

TheThingFactory->newDrawable( tTemplate );

amazingly did not pose an issue because the new pointer returned by newDrawable simply took the exact same memory location as the prior draw pointer. This likely is the explanation why this would have never crashed with VS6 builds.

Other than that, we also did not manage to enter the !m_disguiseAsTemplate code path. Disguising to regular world objects will give a valid template.

[Mauller]

I removed the Bug label because there is no evidence that this was a runtime problem in the retail game.

Additionally, in a debug session with VS2022 we observed that calling

TheGameClient->destroyDrawable( draw );

TheThingFactory->newDrawable( tTemplate );

amazingly did not pose an issue because the new pointer returned by newDrawable simply took the exact same memory location as the prior draw pointer. This likely is the explanation why this would have never crashed with VS6 builds.

Other than that, we also did not manage to enter the !m_disguiseAsTemplate code path. Disguising to regular world objects will give a valid template.

I just got this to trigger and it throws a memory exception.

It occurs when the bomb truck is returning to its original visual, either by being discovered or exploding.

>	generalszh.exe!Drawable::getDrawModules() Line 4000	C++
 	generalszh.exe!Drawable::setIndicatorColor(int color) Line 4107	C++
 	generalszh.exe!Drawable::friend_bindToObject(Object * obj) Line 4170	C++
 	generalszh.exe!GameLogic::bindObjectAndDrawable(Object * obj, Drawable * draw) Line 4169	C++
 	generalszh.exe!StealthUpdate::changeVisualDisguise() Line 1062	C++
 	generalszh.exe!StealthUpdate::update() Line 675	C++
 	generalszh.exe!GameLogic::update() Line 3759	C++
 	generalszh.exe!SubsystemInterface::UPDATE() Line 78	C++
 	generalszh.exe!GameEngine::update() Line 775	C++
 	generalszh.exe!Win32GameEngine::update() Line 90	C++
 	generalszh.exe!GameEngine::execute() Line 836	C++
 	generalszh.exe!GameMain(int argc, char * * argv) Line 44	C++
 	generalszh.exe!WinMain(HINSTANCE__ * hInstance, HINSTANCE__ * hPrevInstance, char * lpCmdLine, int nCmdShow) Line 1033	C++

Exception occurs when it's trying to access the module data from the drawable.

[xezon]

Did you test that with the Null Memory or with regular Game Memory implementation?

Drawable uses the Memory Pool and by the look at MemoryPoolBlob it will reassign the last free'd drawable to the new allocated drawable via the following:

Deallocating is pushing that memory blob to the free list as its first element.

void MemoryPoolBlob::freeSingleBlock(MemoryPoolSingleBlock *block)
{	
	DEBUG_ASSERTCRASH(block->getOwningBlob() == this, ("block does not belong to this blob"));

	block->setNextFreeBlock(m_firstFreeBlock);
	m_firstFreeBlock = block;
	--m_usedBlocksInBlob;
...

Allocating is using that memory blob from the first element in the free list.

MemoryPoolSingleBlock *MemoryPoolBlob::allocateSingleBlock(DECLARE_LITERALSTRING_ARG1)
{
	DEBUG_ASSERTCRASH(m_firstFreeBlock, ("no free blocks available in MemoryPoolBlob"));
	
	MemoryPoolSingleBlock *block = m_firstFreeBlock;
	m_firstFreeBlock = block->getNextFreeBlock();
	++m_usedBlocksInBlob;
...

[Mauller]

Did you test that with the Null Memory or with regular Game Memory implementation?

The null memory implementation, so as you mentioned this likely only works because an object still exists at the location in the memory manager.

Visually the truck fully returns to looking like the truck before exploding when i have it set the draw variable as per the fix.

[xezon]

Yes, I tested and it certainly does not crash with Game Memory because of that MemoryPoolBlob behaviour. So no retail bug, but certainly a problem that is good to fix.

[Mauller]

Yes, I tested and it certainly does not crash with Game Memory because of that MemoryPoolBlob behaviour. So no retail bug, but certainly a problem that is good to fix.

Just testing retail compared to this and it looks like this fix does create a visual difference.
In retail the bombtruck never fully returns to looking like the truck while still disguised when detonating. Only the carcus left behind looks like a bomb truck.

With this fix the truck fully returns to looking like the truck just before it explodes.

[xezon]

In retail the bombtruck never fully returns to looking like the truck while still disguised when detonating.

I was unable to confirm this observation. The Bomb Truck will undisguise normally before this change.

[Mauller]

In retail the bombtruck never fully returns to looking like the truck while still disguised when detonating.

I was unable to confirm this observation. The Bomb Truck will undisguise normally before this change.

I think it was just timing in the end, i had the bomb truck right next to an enemy building before having it attack.
in retail it never seemed to fully reveal itself compared to when checking the SH version.