Yesterday I had about 4 hours of free time so I figured it was time to dig in to the mechanics of crafting.
So I loaded up my servers, fired up the client and logged in with a noobie dealer, bought a few craft skills and just to make sure everything was working properly, I returned to the character select screen and relogged her.
CRASH Goes the server......
WTF I think. It worked just a couple of minutes ago. And to top it off, the debugger failed, saying "no debug symbols saved" or some such bollocks....
Checked the database and all the character info is good and in the right place. the only difference between the save data no and before is that I have a few hundred more skill points and 5 shiny new skills at level 1.
So I recompiled the server, being careful to make sure that it was set the compiler to /Zi in the properties C++ section (it was already but this is c++ we are talking about)
Logged in again.
Same error but at least this time I could debug it.
This is where it threw a wobbly fit on me. VisibilityList() in PlayerFunctions.cpp
- VisiblePlayers.clear();
- VisibleDrops.clear();
- VisibleMonsters.clear();
- VisibleNPCs.clear();
- VisiblePlayers = newVisiblePlayers;
- VisibleDrops = newVisibleDrops;
- VisibleMonsters = newVisibleMonsters;
- VisibleNPCs = newVisibleNPCs;
- newVisiblePlayers.clear();
- newVisibleDrops.clear();
- newVisibleMonsters.clear();
- newVisibleNPCs.clear();
It crashed on the highlighted line with some garbled crap about a corrupted heap.
Basically it's a memory management problem
I've been running into this kind of thing a lot lately.
A couple of weeks ago I was getting random client crashes immediately after login which I managed to trace back to corrupted skills data being sent to the client.
Skills were loading in from the database perfectly well and then they were getting messed up during the loading of inventory items. Specifically the first 3 skills were being completely deleted when the code loaded the item 'stats' value. There was no code that could possibly have been writing to the skills directly so I concluded that the memory location in which the skills were held was being written to by the item loading code.
But HOW?
In Plyer.h the inventory was declared
CItem items[MAX_INVENTORY]; prior to the declaration of the skills
SKILLS cskills[MAX_ALL_SKILL]; and then later the skills were loaded first, followed immediately by the inventory items
I tried everything i could think of.
I added specific constructors to the CItems and cskills structs (yes they are structs rather than classes but that shouldn't make any difference). It didn't help.
Eventually I sidestepped the underlying issue by moving the skills declaration to BEFORE the inventory declaration. It worked fine after that but there is still an underlying problem that I haven't been able to identify so I'm not really happy with this kind of band-aid fix.
Anyway, back to the issue that I have now.
The code in the VisibilityList function is all kinds of UUUGGHHHH. I hate it but as yet I don't see any better way to write it (and that sucks)
I hate that it makes temporary vectors and then scans through the PlayerList, MonsterList, NPCList and the DropsList one by one and compares every single member of those lists against the existing list of the same type (again by scanning it item by item) and then checking the distance to said item against MINVISUALRANGE before building the temporary vectors from the ground.... on every cycle!
before finally wiping the old lists and copying the new ones into them.
That is a horrible mis-use of processor time IMO, but HOW to do it better is the question?
And more to the point WHY is it suddenly starting to crash now when this same code has worked flawlessly for years?
I'm going to have to re-code the whole frickin server at this rate with memory management as the primary concern.... What a horrible thought...
Also WHY is the NPCList made up of a vector of CNPC* objects rather than a vector of shorts containing the clientids of the NPCs in visual range. That would take up so much less memory and it would save having to do a cast for every single one in the map on every cycle to compare it to the one in the list
And why use vectors at all if we are only adding items to the end of the list and never erasing one from the middle?
Vectors require a block of contiguous memory so they grab HUGE chunks of memory to reserve for filling up later.
A List or even a dynamic array would both work in smaller chunks of memory, just grabbing a chunk big enough for the next entry and saving a pointer to the new location automatically. (an automatically managed link list for those who come from a C background)
Today I have a few hours ahead of me where (as of right now) nothing here is broken and needs to be fixed yesterday, no coding (for work) needs to be done and no students, faculty, staff or professors need to be taught how to use a mass spectrometer, remote manipulator arm or laser ablation system. Basically I have a completely free day, pending any disasters that might occur later.
So I'm gonna figure a way to completely rewrite all the visibility process so that they work in a much more effective manner (and hopefully don't crash any more)
Please feel free to chime in with any comments regarding memory management, vectors, arrays, lists or classes. I'm a little rusty in these areas so I'm on a research and development mission.