From 8ebd901dfa7f77e1808c17960c824e633c3b7b8d Mon Sep 17 00:00:00 2001 From: Mark Vejvoda Date: Wed, 2 Jun 2010 08:03:56 +0000 Subject: [PATCH] Fixed the following nasty bugs: - memory corruption when mouse click happens because there are more than 3 possible values for mouse button click - Recursive crash when outputting string representation of unitRef. --- source/glest_game/type_instances/command.cpp | 16 +++++++- source/glest_game/type_instances/faction.cpp | 38 ++++++++++++++++--- source/glest_game/type_instances/unit.cpp | 29 ++++++++++---- source/glest_game/type_instances/unit.h | 3 ++ .../shared_lib/include/platform/sdl/window.h | 6 +-- .../sources/platform/sdl/window.cpp | 20 ++++++++-- source/shared_lib/sources/util/util.cpp | 8 ++-- 7 files changed, 96 insertions(+), 24 deletions(-) diff --git a/source/glest_game/type_instances/command.cpp b/source/glest_game/type_instances/command.cpp index 1601fa1d..a0033c78 100644 --- a/source/glest_game/type_instances/command.cpp +++ b/source/glest_game/type_instances/command.cpp @@ -70,16 +70,28 @@ void Command::setUnit(Unit *unit){ } std::string Command::toString() const { + //SystemFlags::OutputDebug(SystemFlags::debugSystem,"In [%s::%s Line: %d]\n",__FILE__,__FUNCTION__, __LINE__); + std::string result = ""; result = "commandType id = " + intToStr(commandType->getId()) + ", desc = " + commandType->toString() + ", pos = " + pos.getString() + ", facing = " + intToStr(facing.asInt()); - if(unitRef.getUnit() != NULL) { - result += ", unitRef.getUnit() = " + unitRef.getUnit()->toString(); + //SystemFlags::OutputDebug(SystemFlags::debugSystem,"In [%s::%s Line: %d]\n",__FILE__,__FUNCTION__, __LINE__); + + //if(unitRef.getUnit() != NULL) { + if(unitRef.getUnitId() >= 0) { + //SystemFlags::OutputDebug(SystemFlags::debugSystem,"In [%s::%s Line: %d]\n",__FILE__,__FUNCTION__, __LINE__); + + result += ", unitRef.getUnit() id = " + unitRef.getUnitId(); } + //SystemFlags::OutputDebug(SystemFlags::debugSystem,"In [%s::%s Line: %d]\n",__FILE__,__FUNCTION__, __LINE__); + if(unitType != NULL) { result += ", unitTypeId = " + intToStr(unitType->getId()) + ", unitTypeDesc = " + unitType->getReqDesc(); } + + //SystemFlags::OutputDebug(SystemFlags::debugSystem,"In [%s::%s Line: %d]\n",__FILE__,__FUNCTION__, __LINE__); + return result; } diff --git a/source/glest_game/type_instances/faction.cpp b/source/glest_game/type_instances/faction.cpp index b12185f6..1668fa20 100644 --- a/source/glest_game/type_instances/faction.cpp +++ b/source/glest_game/type_instances/faction.cpp @@ -403,28 +403,56 @@ void Faction::setResourceBalance(const ResourceType *rt, int balance){ } Unit *Faction::findUnit(int id){ + //SystemFlags::OutputDebug(SystemFlags::debugSystem,"In [%s::%s Line: %d] id = %d\n",__FILE__,__FUNCTION__, __LINE__,id); + UnitMap::iterator it= unitMap.find(id); + //SystemFlags::OutputDebug(SystemFlags::debugSystem,"In [%s::%s Line: %d]\n",__FILE__,__FUNCTION__, __LINE__); + if(it==unitMap.end()){ + //SystemFlags::OutputDebug(SystemFlags::debugSystem,"In [%s::%s Line: %d]\n",__FILE__,__FUNCTION__, __LINE__); return NULL; } + //SystemFlags::OutputDebug(SystemFlags::debugSystem,"In [%s::%s Line: %d] it->second = %p\n",__FILE__,__FUNCTION__, __LINE__,it->second); + //SystemFlags::OutputDebug(SystemFlags::debugSystem,"In [%s::%s Line: %d] it->second->id = %d\n",__FILE__,__FUNCTION__, __LINE__,it->second->getId()); + //SystemFlags::OutputDebug(SystemFlags::debugSystem,"In [%s::%s Line: %d] it->second = %s\n",__FILE__,__FUNCTION__, __LINE__,it->second->toString().c_str()); return it->second; } void Faction::addUnit(Unit *unit){ + //SystemFlags::OutputDebug(SystemFlags::debugSystem,"In [%s::%s Line: %d] id = %d\n",__FILE__,__FUNCTION__, __LINE__,unit->getId()); + + //SystemFlags::OutputDebug(SystemFlags::debugSystem,"In [%s::%s Line: %d] it->second = %p\n",__FILE__,__FUNCTION__, __LINE__,unit); + //SystemFlags::OutputDebug(SystemFlags::debugSystem,"In [%s::%s Line: %d] it->second->id = %d\n",__FILE__,__FUNCTION__, __LINE__,unit->getId()); + //SystemFlags::OutputDebug(SystemFlags::debugSystem,"In [%s::%s Line: %d] it->second->str = %s\n",__FILE__,__FUNCTION__, __LINE__,unit->toString().c_str()); + units.push_back(unit); - unitMap.insert(make_pair(unit->getId(), unit)); + //unitMap.insert(make_pair(unit->getId(), unit)); + unitMap[unit->getId()] = unit; + + //SystemFlags::OutputDebug(SystemFlags::debugSystem,"In [%s::%s Line: %d] id = %d\n",__FILE__,__FUNCTION__, __LINE__,unit->getId()); } void Faction::removeUnit(Unit *unit){ - for(int i=0; igetId()); + + assert(units.size()==unitMap.size()); + + int unitId = unit->getId(); + for(int i=0; igetId() == unitId) { units.erase(units.begin()+i); - unitMap.erase(unit->getId()); - assert(units.size()==unitMap.size()); + unitMap.erase(unitId); + assert(units.size() == unitMap.size()); + + //SystemFlags::OutputDebug(SystemFlags::debugSystem,"In [%s::%s Line: %d] id = %d\n",__FILE__,__FUNCTION__, __LINE__,unitId); + return; } } + //SystemFlags::OutputDebug(SystemFlags::debugSystem,"In [%s::%s Line: %d] id = %d\n",__FILE__,__FUNCTION__, __LINE__,unitId); + + throw runtime_error("Could not remove unit from faction!"); assert(false); } diff --git a/source/glest_game/type_instances/unit.cpp b/source/glest_game/type_instances/unit.cpp index 617230b3..0c9fcf42 100644 --- a/source/glest_game/type_instances/unit.cpp +++ b/source/glest_game/type_instances/unit.cpp @@ -105,9 +105,14 @@ void UnitReference::operator=(const Unit *unit){ } Unit *UnitReference::getUnit() const{ + //SystemFlags::OutputDebug(SystemFlags::debugSystem,"In [%s::%s Line: %d]\n",__FILE__,__FUNCTION__, __LINE__); + if(faction!=NULL){ + //SystemFlags::OutputDebug(SystemFlags::debugSystem,"In [%s::%s Line: %d]\n",__FILE__,__FUNCTION__, __LINE__); + return faction->findUnit(id); } + //SystemFlags::OutputDebug(SystemFlags::debugSystem,"In [%s::%s Line: %d]\n",__FILE__,__FUNCTION__, __LINE__); return NULL; } @@ -138,6 +143,7 @@ Unit::Unit(int id, const Vec2i &pos, const UnitType *type, Faction *faction, Map this->type=type; this->faction=faction; this->map= map; + this->targetRef = NULL; level= NULL; loadType= NULL; @@ -193,28 +199,33 @@ Unit::Unit(int id, const Vec2i &pos, const UnitType *type, Faction *faction, Map } Unit::~Unit(){ + SystemFlags::OutputDebug(SystemFlags::debugSystem,"In [%s::%s Line: %d] delete unitid = %d\n",__FILE__,__FUNCTION__,__LINE__,id); + //Just to be sure, should already be removed if (livingUnits.erase(id)) { - SystemFlags::OutputDebug(SystemFlags::debugSystem,"In [%s::%s] START\n",__FILE__,__FUNCTION__); + SystemFlags::OutputDebug(SystemFlags::debugSystem,"In [%s::%s Line: %d]\n",__FILE__,__FUNCTION__,__LINE__); //Only an error if not called at end } //Just to be sure, should already be removed if (livingUnitsp.erase(this)) { - SystemFlags::OutputDebug(SystemFlags::debugSystem,"In [%s::%s] START\n",__FILE__,__FUNCTION__); + SystemFlags::OutputDebug(SystemFlags::debugSystem,"In [%s::%s Line: %d]\n",__FILE__,__FUNCTION__,__LINE__); } + SystemFlags::OutputDebug(SystemFlags::debugSystem,"In [%s::%s Line: %d]\n",__FILE__,__FUNCTION__,__LINE__); //remove commands while(!commands.empty()){ delete commands.back(); commands.pop_back(); } + SystemFlags::OutputDebug(SystemFlags::debugSystem,"In [%s::%s Line: %d]\n",__FILE__,__FUNCTION__,__LINE__); // fade(and by this remove) all unit particle systems while(!unitParticleSystems.empty()){ unitParticleSystems.back()->fade(); unitParticleSystems.pop_back(); } stopDamageParticles(); + SystemFlags::OutputDebug(SystemFlags::debugSystem,"In [%s::%s Line: %d]\n",__FILE__,__FUNCTION__,__LINE__); } void Unit::setModelFacing(CardinalDir value) { @@ -507,14 +518,15 @@ unsigned int Unit::getCommandSize() const{ //give one command (clear, and push back) CommandResult Unit::giveCommand(Command *command, bool tryQueue) { - - SystemFlags::OutputDebug(SystemFlags::debugSystem,"In [%s::%s Line: %d] Unit id = %d name = %s, Command [%s] tryQueue = %d\n", - __FILE__,__FUNCTION__, __LINE__,this->id,(this->type != NULL ? this->type->getName().c_str() : "null"), (command != NULL ? command->toString().c_str() : "null"),tryQueue); + SystemFlags::OutputDebug(SystemFlags::debugSystem,"In [%s::%s Line: %d]\n",__FILE__,__FUNCTION__, __LINE__); assert(command != NULL); assert(command->getCommandType() != NULL); + SystemFlags::OutputDebug(SystemFlags::debugSystem,"In [%s::%s Line: %d] Unit id = %d name = %s, Command [%s] tryQueue = %d\n", + __FILE__,__FUNCTION__, __LINE__,this->id,(this->type != NULL ? this->type->getName().c_str() : "null"), (command != NULL ? command->toString().c_str() : "null"),tryQueue); + //SystemFlags::OutputDebug(SystemFlags::debugSystem,"In [%s::%s Line: %d]\n",__FILE__,__FUNCTION__,__LINE__); const int command_priority = command->getCommandType()->getPriority(); @@ -1302,8 +1314,11 @@ std::string Unit::toString() const { //SystemFlags::OutputDebug(SystemFlags::debugSystem,"In [%s::%s Line: %d]\n",__FILE__,__FUNCTION__,__LINE__); - if(this->targetRef.getUnit() != NULL) { - result += " targetRef = " + this->targetRef.getUnit()->toString(); + // WARNING!!! Don't access the Unit pointer in this->targetRef in this method or it causes + // a stack overflow + if(this->targetRef.getUnitId() >= 0) { + //result += " targetRef = " + this->targetRef.getUnit()->toString(); + result += " targetRef = " + intToStr(this->targetRef.getUnitId()) + " - factionIndex = " + intToStr(this->targetRef.getUnitFaction()->getIndex()); } //SystemFlags::OutputDebug(SystemFlags::debugSystem,"In [%s::%s Line: %d]\n",__FILE__,__FUNCTION__,__LINE__); diff --git a/source/glest_game/type_instances/unit.h b/source/glest_game/type_instances/unit.h index c5da8833..ea3b6a93 100644 --- a/source/glest_game/type_instances/unit.h +++ b/source/glest_game/type_instances/unit.h @@ -90,6 +90,9 @@ public: void operator=(const Unit *unit); Unit *getUnit() const; + + int getUnitId() const { return id; } + Faction *getUnitFaction() const { return faction; } }; // ===================================================== diff --git a/source/shared_lib/include/platform/sdl/window.h b/source/shared_lib/include/platform/sdl/window.h index e023060c..2be57cf9 100644 --- a/source/shared_lib/include/platform/sdl/window.h +++ b/source/shared_lib/include/platform/sdl/window.h @@ -104,9 +104,9 @@ enum WindowStyle{ class Window { private: - Uint32 lastMouseDown[3]; - int lastMouseX[3]; - int lastMouseY[3]; + Uint32 lastMouseDown[mbCount]; + int lastMouseX[mbCount]; + int lastMouseY[mbCount]; static unsigned int lastMouseEvent; /** for use in mouse hover calculations */ static MouseState mouseState; diff --git a/source/shared_lib/sources/platform/sdl/window.cpp b/source/shared_lib/sources/platform/sdl/window.cpp index 54a1e173..bee1860c 100644 --- a/source/shared_lib/sources/platform/sdl/window.cpp +++ b/source/shared_lib/sources/platform/sdl/window.cpp @@ -53,7 +53,9 @@ bool Window::isActive = false; // ========== PUBLIC ========== Window::Window() { - memset(lastMouseDown, 0, sizeof(lastMouseDown)); + for(int idx = 0; idx < mbCount; idx++) { + lastMouseDown[idx] = 0; + } assert(global_window == 0); global_window = this; @@ -70,12 +72,17 @@ Window::~Window() { } bool Window::handleEvent() { + //SystemFlags::OutputDebug(SystemFlags::debugSystem,"In [%s::%s Line: %d]\n",__FILE__,__FUNCTION__,__LINE__); + SDL_Event event; SDL_GetMouseState(&oldX,&oldY); + //SystemFlags::OutputDebug(SystemFlags::debugSystem,"In [%s::%s Line: %d]\n",__FILE__,__FUNCTION__,__LINE__); + while(SDL_PollEvent(&event)) { try { //printf("START [%d]\n",event.type); + //SystemFlags::OutputDebug(SystemFlags::debugSystem,"In [%s::%s Line: %d]\n",__FILE__,__FUNCTION__,__LINE__); switch(event.type) { case SDL_MOUSEBUTTONDOWN: @@ -88,6 +95,8 @@ bool Window::handleEvent() { break; } + //SystemFlags::OutputDebug(SystemFlags::debugSystem,"In [%s::%s Line: %d]\n",__FILE__,__FUNCTION__,__LINE__); + switch(event.type) { case SDL_QUIT: //printf("In [%s::%s] Line :%d\n",__FILE__,__FUNCTION__,__LINE__); @@ -224,9 +233,11 @@ bool Window::handleEvent() { SystemFlags::OutputDebug(SystemFlags::debugSystem,"In [%s::%s %d] (b) Couldn't process event: [UNKNOWN ERROR]\n",__FILE__,__FUNCTION__,__LINE__); } + //SystemFlags::OutputDebug(SystemFlags::debugSystem,"In [%s::%s Line: %d]\n",__FILE__,__FUNCTION__,__LINE__); } //printf("END [%d]\n",event.type); + //SystemFlags::OutputDebug(SystemFlags::debugSystem,"In [%s::%s Line: %d]\n",__FILE__,__FUNCTION__,__LINE__); return true; } @@ -441,15 +452,18 @@ void Window::handleMouseDown(SDL_Event event) { return; } - Uint32 ticks = SDL_GetTicks(); int n = (int) button; + + assert(n >= 0 && n < mbCount); + if(ticks - lastMouseDown[n] < DOUBLECLICKTIME && abs(lastMouseX[n] - event.button.x) < DOUBLECLICKDELTA && abs(lastMouseY[n] - event.button.y) < DOUBLECLICKDELTA) { eventMouseDown(event.button.x, event.button.y, button); eventMouseDoubleClick(event.button.x, event.button.y, button); - } else { + } + else { eventMouseDown(event.button.x, event.button.y, button); } lastMouseDown[n] = ticks; diff --git a/source/shared_lib/sources/util/util.cpp b/source/shared_lib/sources/util/util.cpp index 15ef4760..39c770e2 100644 --- a/source/shared_lib/sources/util/util.cpp +++ b/source/shared_lib/sources/util/util.cpp @@ -265,12 +265,12 @@ void SystemFlags::handleDebug(DebugType type, const char *fmt, ...) { printf("Opening logfile [%s] type = %d, currentDebugLog.fileStreamOwner = %d\n",debugLog.c_str(),type, currentDebugLog.fileStreamOwner); - currentDebugLog.mutex->p(); + MutexSafeWrapper safeMutex(currentDebugLog.mutex); (*currentDebugLog.fileStream) << "Starting Mega-Glest logging for type: " << type << "\n"; (*currentDebugLog.fileStream).flush(); - currentDebugLog.mutex->v(); + safeMutex.ReleaseLock(); } //printf("Logfile is open [%s]\n",SystemFlags::debugLogFile); @@ -279,12 +279,12 @@ void SystemFlags::handleDebug(DebugType type, const char *fmt, ...) { assert(currentDebugLog.fileStream != NULL); - currentDebugLog.mutex->p(); + MutexSafeWrapper safeMutex(currentDebugLog.mutex); (*currentDebugLog.fileStream) << "[" << szBuf2 << "] " << szBuf; (*currentDebugLog.fileStream).flush(); - currentDebugLog.mutex->v(); + safeMutex.ReleaseLock(); } // output to console else {