Thoughts on Fixing the Prereq AA issue

[seveianrex]

Unfortunately, I think there is a major issue with adding AAs to that table that causes the prereq_skill fields to get messed up. I didn't understand what was going on at first when I added some AAs, but apparently, the prereq_skill number is actually the line number in the table in order. So, if you add one to the table, it throws off all prereqs that are higher than the one at the line you added. This seems like a really horrible way for this table to work. I am going to see if there is a better way to do it. Maybe we can get it changed so that making a change there doesn't mess everything else up. It would certainly make adding non-existing AAs MUCH easier.

So, basically, you probably don't want to update that table yet until the code for prereqs is adjusted/fixed. Unless you know of a good way to have it increment all prereqs higher than that line. Like, if the AA you add is on line number 100, anything over 99 in the prereq field would need to be incremented by 1. I am trying to figure that out to correct an issue with the AAs I added. Until then, it might be best to just have those lines for the new AAs I added removed from the table. Hopefully I can find a good answer soon! Still trying to figure out what to do about it until then...


In regards to this problem. I have been thinking..... I believe I understand how it has to work, but correct me if I'm wrong:

The client interprets the AA Index which is why we store that value in the DB. We pull the value for each AA from the DB and send it to the client in the process ZoneDatabase:GetAASkillVars() (aa.cpp).

My question is this- would it not be a lot less of a headache to store the SKILL ID of the AA in the database and determine the so-called "index" at the time of sending it to the client? I.e:


for (int i = 0; i < MAX_AAs; i++) {
if (targetAA == AAs[i]) return i;
}


something like that??

[AndMetal]

The client interprets the AA Index which is why we store that value in the DB. We pull the value for each AA from the DB and send it to the client in the process ZoneDatabase:GetAASkillVars() (aa.cpp).

That's what it looks like, since all we do with it is put it into the OP_SendAATable packet & send that off to the client. As a result, the client gets it in the same order we're getting it from the DB
RunQuery(query, MakeAnyLenString(&query, "SELECT cost, max_level, hotkey_sid, hotkey_sid2, "
"title_sid, desc_sid, type, prereq_skill, prereq_minpoints, spell_type, spell_refresh, "
"classes, berserker,spellid,class_type,name,cost_inc"
" FROM altadv_vars WHERE skill_id=%i", skill_id), errbuf, &result)
which is sorted by the index number since we don't have a SORT BY clause.

I'm wondering if we can do all of this with a modified DB query (using some sort of subquery probably) that translates a prereq_skill as a skill_id to the index #?

After working over this for a bit (with some inspiration (http://forums.mysql.com/read.php?10,36490,36511)), here's the query I came up with:

SET @row = 0;
SELECT
a.cost,
a.max_level,
a.hotkey_sid,
a.hotkey_sid2,
a.title_sid,
a.desc_sid,
a.type,
COALESCE(
(
SELECT
prereq_index_num
FROM
(
SELECT
@row := @row + 1 AS prereq_index_num
FROM
altadv_vars
) AS prereq_conv
WHERE
prereq_skill = prereq_index_num
),
0) AS prereq_skill_index,
a.prereq_minpoints,
a.spell_type,
a.spell_refresh,
a.classes,
a.berserker,
a.spellid,
a.class_type,
a.name,
a.cost_inc
FROM
altadv_vars a
;
which you actually have to execute as this for it to work properly:
SET @row = 0;
SELECT a.cost, a.max_level, a.hotkey_sid, a.hotkey_sid2, a.title_sid, a.desc_sid, a.type, COALESCE((SELECT prereq_index_num FROM (SELECT @row := @row + 1 AS prereq_index_num FROM altadv_vars) AS prereq_conv WHERE prereq_skill = prereq_index_num), 0) AS prereq_skill_index, a.prereq_minpoints, a.spell_type, a.spell_refresh, a.classes, a.berserker, a.spellid, a.class_type, a.name, a.cost_inc FROM altadv_vars a;

The only problem I've run into with the query is being able to manipulate the derived column, prereq_skill_index, in the where clause:

mysql> SELECT a.cost, a.max_level, a.hotkey_sid, a.hotkey_sid2, a.title_sid, a.desc_sid, a.type, COALESCE((SELECT prereq_index_num FROM (SELECT @row := @row + 1 AS prereq_index_num FROM altadv_vars) AS prereq_conv WHERE prereq_skill = prereq_index_num), 0) AS prereq_skill_index, a.prereq_minpoints, a.spell_type, a.spell_refresh, a.classes, a.berserker, a.spellid, a.class_type, a.name, a.cost_inc FROM altadv_vars a WHERE prereq_index_num > 0;
ERROR 1054 (42S22): Unknown column 'prereq_index_num' in 'where clause'

However, at this point, we don't really need to do anything with it, and even if we did, we should be able to utilize the still existing prereq_skill column in the altadv_vars table.

We could definitely iterate through the possible AA IDs, but I think this would be a lot cleaner & less resource intensive (relatively, since we're only talking about a few hundred to a few thousand loops, depending on how quickly we find our match).

So, if we decide to go this route, we can do this:
in zone/AA.cpp, around line 1241 in ZoneDatabase::GetAASkillVars(), change

if (RunQuery(query, MakeAnyLenString(&query, "SET @row = 0"), errbuf, &result)) { //initialize "row" variable in database for next query
query = 0; //reset for next query
if (RunQuery(query, MakeAnyLenString(&query,
"SELECT cost, "
"max_level, "
"hotkey_sid, "
"hotkey_sid2, "
"title_sid, "
"desc_sid, "
"type, "
"COALESCE(" //so we can return 0 if it's null
"(" //this is our derived table that has the row # that we can SELECT from, because the client is stupid
"SELECT "
"prereq_index_num "
"FROM "
"("
"SELECT "
"@row := @row + 1 AS prereq_index_num "
"FROM "
"altadv_vars"
") AS prereq_conv "
"WHERE "
"prereq_skill = prereq_index_num"
"), "
"0) AS prereq_skill_index, "
"prereq_minpoints, "
"spell_type, "
"spell_refresh, "
"classes, "
"berserker, "
"spellid, "
"class_type, "
"name, "
"cost_inc "
" FROM altadv_vars WHERE skill_id=%i", skill_id), errbuf, &result)) {
safe_delete_array(query);
if (mysql_num_rows(result) == 1) {
int total_abilities = GetTotalAALevels(skill_id);
int totalsize = total_abilities * sizeof(AA_Ability) + sizeof(SendAA_Struct);

buffer = new uchar[totalsize];
memset(buffer,0,totalsize);
sendaa = (SendAA_Struct*)buffer;

row = mysql_fetch_row(result);

//ATOI IS NOT UNISGNED LONG-SAFE!!!

sendaa->cost = atoul(row[0]);
sendaa->cost2 = sendaa->cost;
sendaa->max_level = atoul(row[1]);
sendaa->hotkey_sid = atoul(row[2]);
sendaa->id = skill_id;
sendaa->hotkey_sid2 = atoul(row[3]);
sendaa->title_sid = atoul(row[4]);
sendaa->desc_sid = atoul(row[5]);
sendaa->type = atoul(row[6]);
sendaa->prereq_skill = atoul(row[7]);
sendaa->prereq_minpoints = atoul(row[8]);
sendaa->spell_type = atoul(row[9]);
sendaa->spell_refresh = atoul(row[10]);
sendaa->classes = atoul(row[11]);
sendaa->berserker = atoul(row[12]);
sendaa->last_id = 0xFFFFFFFF;
sendaa->current_level=1;
sendaa->spellid = atoul(row[13]);
sendaa->class_type = atoul(row[14]);
strcpy(sendaa->name,row[15]);

sendaa->total_abilities=total_abilities;
if(sendaa->max_level > 1)
sendaa->next_id=skill_id+1;
else
sendaa->next_id=0xFFFFFFFF;

sendaa->cost_inc = atoi(row[16]);
}
mysql_free_result(result);
} else {
LogFile->write(EQEMuLog::Error, "Error in GetAASkillVars '%s': %s", query, errbuf);
safe_delete_array(query);
}
} else {
LogFile->write(EQEMuLog::Error, "Error in GetAASkillVars '%s': %s", query, errbuf);
safe_delete_array(query);
}


On a side note, I think this may also be a good opportunity to merge the classes & berserker columns in the database, since they can be OR'd together, and separate them out in the same function.

Any feedback?

[trevius]

If this works as intended, it looks like a great solution to me! And I agree about the berserker field too.

Then, we could just have a single SQL update in the SVN that comes at the same time this code goes in that will drop the altadv_vars table and create a new modified one that includes all current AAs with the new prereq_skill, classes, and berserker fields information. And then any time a new AA is created, the associated SQL could be added to the /utils/sql/svn directory. This would keep everyone up to date on code and AAs, and the new way of setting prereq_skills would stop the risk of messing up AAs when adding new AAs to the table.

The only other possible solution I can think of for the Indexing and prereq_skill field issues would be to fill in the skill_id for every possible AA in Titanium. I think this solution might not be too bad. We would just need to add the skill ID for each one (maybe a few hundred or so) and then leave the other fields blank and set the class to 0 for all of the blank ones so they don't show up for players. Then, we could even add 1 more column to the table that would increment by 1 for each row and match up with the indexing of the skill_id field. So, if someone wanted to add a new AA, all they would have to do is fill in the blank fields for the appropriate skill_id, and nothing else would need to be changed. And, if anyone felt like doing the work, they could even fill in names and other info for the blank ones. As long as class is left at 0 until the AA is actually implemented, we shouldn't have to worry about it. I would definitely consider doing the SQL work if it sounds like a good solution.

[KLS]

To be honest I really don't see the point of all this. It doesn't fix any sort of real problem it just makes you lazy bastards happy. =p

[trevius]

Well, considering that adding AAs to the source actually requires SQL in order to use them, then IMO, the SQL does need to be added to the /util/sql/svn directory when an update is made. All other changes to the source that require SQL changes get posted that way. Sure, we could probably get the table from PEQ, but this is the only scenario where a change in the source requires SQL and isn't being included in the download.

I personally don't keep up-to-date with the PEQ database and don't really ever plan to. My server as many others are custom and the PEQ database is just used for grounds to start on and maybe allow extra content for players on the server. So, having to get the PEQ DB just to get new AAs seems a bit like an extra unneeded chore to me.

I don't think it is as much about being lazy as it is about being consistent and having low risk updates. I am sure you don't think that using indexing for that table the way it is now is the best way to do it. All it would take is 1 little SQL mistake and some AAs could be broken. And given the number of AAs, it could be a while before it was even noticed to be corrected. I think it would be nice to have an option to let anyone update AAs purely through the SVN and not have to use an alternate site or upload or post or whatever to provide PEQ with the SQL for it. Then wait for it to be added to PEQ for it to be usable by all.

It can be done by using your query here:
SET @idindex = 0;
SELECT skill_id, name, (@idindex:=@idindex+1) FROM altadv_vars ORDER BY skill_id ASC;

And once you know what to set the prereq_skill to, you can use this SQL to up all prereq_skills that are higher by 1:
UPDATE `altadv_vars` SET `prereq_skill`=`prereq_skill`+1 Where `prereq_skill`>=86;

Then, once you are done preparing the table for the entry, you just make the entry:
INSERT INTO altadv_vars (skill_id, name, cost, max_level, hotkey_sid, hotkey_sid2, title_sid, desc_sid, type, spellid, prereq_skill, prereq_minpoints, spell_type, spell_refresh, classes, berserker, class_type, cost_inc) VALUES
(209, 'Death Peace', 5, 1, 13738, 13739, 13736, 13737, 7, 4294967295, 0, 0, 0, 5, 2080, 0, 65, 0);

And that should let people get them added fairly easily via SVN I think. But the thing that concerns me about that is if someone makes a mistake, it can throw off the whole table. Or, if someone runs the same SQL more than 1 time, it will mess up the whole table. Sure, it probably wouldn't happen too often and is reversible, but if we can remove the risk, why not?

And what is so bad about being lazy?! :P I like to think of it as more efficient. Who wants to do things the hard way? If we are going to continue to get a steady flow of AA additions, then IMO it would be well worth it to make it as painless and safe as possible. :)

[AndMetal]

It doesn't fix any sort of real problem it just makes you lazy bastards happy. =p

And what is so bad about being lazy?! :P I like to think of it as more efficient. Who wants to do things the hard way?

I thought that's what coding was all about... :D

[trevius]

Finally broke down and downloaded to PEQ DB and found out I was missing about 60 AAs! That is both awesome and sad at the same time lol.

[AndMetal]

After working with the query a little bit, I figured out I was comparing index to index rather than skill_id to skill_id, so nothing was actually happening. I just had to change the prereq_skill_index part:

COALESCE((SELECT prereq_index_num FROM (SELECT skill_id, @row := @row + 1 AS prereq_index_num FROM altadv_vars) AS p WHERE p.skill_id = a.prereq_skill), 0) AS prereq_skill_index

So, the code is working as I expected now:

Index: Z:/svn/EQEmuServer/zone/AA.cpp
================================================== =================
--- Z:/svn/EQEmuServer/zone/AA.cpp (revision 158)
+++ Z:/svn/EQEmuServer/zone/AA.cpp (working copy)
@@ -1233,58 +1233,96 @@
SendAA_Struct* ZoneDatabase::GetAASkillVars(int32 skill_id)
{
char errbuf[MYSQL_ERRMSG_SIZE];
- char *query = 0;
- MYSQL_RES *result;
- MYSQL_ROW row;
+ char *query = 0;
SendAA_Struct* sendaa = NULL;
uchar* buffer;
- if (RunQuery(query, MakeAnyLenString(&query, "SELECT cost, max_level, hotkey_sid, hotkey_sid2, "
- "title_sid, desc_sid, type, prereq_skill, prereq_minpoints, spell_type, spell_refresh, "
- "classes, berserker,spellid,class_type,name,cost_inc"
- " FROM altadv_vars WHERE skill_id=%i", skill_id), errbuf, &result)) {
- safe_delete_array(query);
- if (mysql_num_rows(result) == 1) {
- int total_abilities = GetTotalAALevels(skill_id);
- int totalsize = total_abilities * sizeof(AA_Ability) + sizeof(SendAA_Struct);
-
- buffer = new uchar[totalsize];
- memset(buffer,0,totalsize);
- sendaa = (SendAA_Struct*)buffer;
-
- row = mysql_fetch_row(result);
-
- //ATOI IS NOT UNISGNED LONG-SAFE!!!
-
- sendaa->cost = atoul(row[0]);
- sendaa->cost2 = sendaa->cost;
- sendaa->max_level = atoul(row[1]);
- sendaa->hotkey_sid = atoul(row[2]);
- sendaa->id = skill_id;
- sendaa->hotkey_sid2 = atoul(row[3]);
- sendaa->title_sid = atoul(row[4]);
- sendaa->desc_sid = atoul(row[5]);
- sendaa->type = atoul(row[6]);
- sendaa->prereq_skill = atoul(row[7]);
- sendaa->prereq_minpoints = atoul(row[8]);
- sendaa->spell_type = atoul(row[9]);
- sendaa->spell_refresh = atoul(row[10]);
- sendaa->classes = atoul(row[11]);
- sendaa->berserker = atoul(row[12]);
- sendaa->last_id = 0xFFFFFFFF;
- sendaa->current_level=1;
- sendaa->spellid = atoul(row[13]);
- sendaa->class_type = atoul(row[14]);
- strcpy(sendaa->name,row[15]);
-
- sendaa->total_abilities=total_abilities;
- if(sendaa->max_level > 1)
- sendaa->next_id=skill_id+1;
- else
- sendaa->next_id=0xFFFFFFFF;
-
- sendaa->cost_inc = atoi(row[16]);
+ if (RunQuery(query, MakeAnyLenString(&query, "SET @row = 0"), errbuf)) { //initialize "row" variable in database for next query
+ query = 0; //reset for next query
+ MYSQL_RES *result; //we don't really need these unless we get to this point, so why bother?
+ MYSQL_ROW row;
+
+ if (RunQuery(query, MakeAnyLenString(&query,
+ "SELECT "
+ "a.cost, "
+ "a.max_level, "
+ "a.hotkey_sid, "
+ "a.hotkey_sid2, "
+ "a.title_sid, "
+ "a.desc_sid, "
+ "a.type, "
+ "COALESCE(" //so we can return 0 if it's null
+ "(" //this is our derived table that has the row # that we can SELECT from, because the client is stupid
+ "SELECT "
+ "p.prereq_index_num "
+ "FROM "
+ "("
+ "SELECT "
+ "a2.skill_id, "
+ "@row := @row + 1 AS prereq_index_num "
+ "FROM "
+ "altadv_vars a2"
+ ") AS p "
+ "WHERE "
+ "p.skill_id = a.prereq_skill"
+ "), "
+ "0) AS prereq_skill_index, "
+ "a.prereq_minpoints, "
+ "a.spell_type, "
+ "a.spell_refresh, "
+ "a.classes, "
+ "a.berserker, "
+ "a.spellid, "
+ "a.class_type, "
+ "a.name, "
+ "a.cost_inc "
+ " FROM altadv_vars a WHERE skill_id=%i", skill_id), errbuf, &result)) {
+ safe_delete_array(query);
+ if (mysql_num_rows(result) == 1) {
+ int total_abilities = GetTotalAALevels(skill_id);
+ int totalsize = total_abilities * sizeof(AA_Ability) + sizeof(SendAA_Struct);
+
+ buffer = new uchar[totalsize];
+ memset(buffer,0,totalsize);
+ sendaa = (SendAA_Struct*)buffer;
+
+ row = mysql_fetch_row(result);
+
+ //ATOI IS NOT UNISGNED LONG-SAFE!!!
+
+ sendaa->cost = atoul(row[0]);
+ sendaa->cost2 = sendaa->cost;
+ sendaa->max_level = atoul(row[1]);
+ sendaa->hotkey_sid = atoul(row[2]);
+ sendaa->id = skill_id;
+ sendaa->hotkey_sid2 = atoul(row[3]);
+ sendaa->title_sid = atoul(row[4]);
+ sendaa->desc_sid = atoul(row[5]);
+ sendaa->type = atoul(row[6]);
+ sendaa->prereq_skill = atoul(row[7]);
+ sendaa->prereq_minpoints = atoul(row[8]);
+ sendaa->spell_type = atoul(row[9]);
+ sendaa->spell_refresh = atoul(row[10]);
+ sendaa->classes = atoul(row[11]);
+ sendaa->berserker = atoul(row[12]);
+ sendaa->last_id = 0xFFFFFFFF;
+ sendaa->current_level=1;
+ sendaa->spellid = atoul(row[13]);
+ sendaa->class_type = atoul(row[14]);
+ strcpy(sendaa->name,row[15]);
+
+ sendaa->total_abilities=total_abilities;
+ if(sendaa->max_level > 1)
+ sendaa->next_id=skill_id+1;
+ else
+ sendaa->next_id=0xFFFFFFFF;
+
+ sendaa->cost_inc = atoi(row[16]);
+ }
+ mysql_free_result(result);
+ } else {
+ LogFile->write(EQEMuLog::Error, "Error in GetAASkillVars '%s': %s", query, errbuf);
+ safe_delete_array(query);
}
- mysql_free_result(result);
} else {
LogFile->write(EQEMuLog::Error, "Error in GetAASkillVars '%s': %s", query, errbuf);
safe_delete_array(query);


Next step, creating a query to migrate from the indexes to the skill IDs so we don't have to do it by hand.

[cavedude]

Finally broke down and downloaded to PEQ DB and found out I was missing about 60 AAs! That is both awesome and sad at the same time lol.

A bit of warning, many of them may not work. I have fixed most of them since, but there is still some tweaking to be done. I figure I'll get everything as clean as possible before letting other cooks into the kitchen. PEQ CVS will be updated as well as the code's SVN when that happens.

[AndMetal]

I figure I'll get everything as clean as possible before letting other cooks into the kitchen. PEQ CVS will be updated as well as the code's SVN when that happens.

I don't think I follow...

[cavedude]

I just meant I am going to get the AA tables as correct as possible before releasing them so other people can work with them. It's silly to have 4 or 5 different people working with AAs and not even working off a single DB.

[AndMetal]

Next step, creating a query to migrate from the indexes to the skill IDs so we don't have to do it by hand.

SET @row = 0;
UPDATE altadv_vars a SET prereq_skill = COALESCE((SELECT p.skill_id FROM (SELECT a2.skill_id, @row := @row + 1 AS prereq_index_num FROM altadv_vars a2) AS p WHERE p.prereq_index_num = a.prereq_skill), 0) WHERE prereq_skill < 1000000;

I also had to make a slight change to the query so that it doesn't fubar archetype & class requirements (4294967295 & 4294967294):
COALESCE((SELECT prereq_index_num FROM (SELECT skill_id, @row := @row + 1 AS prereq_index_num FROM altadv_vars) AS p WHERE p.skill_id = a.prereq_skill), a.prereq_skill) AS prereq_skill_index

Because of the database changes needed, I'm going to hold off on committing this for now until everyone feels comfortable with making the switch.

[trevius]

I am going to test this out on my server the next time I update to the SVN (probably this weekend). Figured it was worth asking the status of the possibility of getting this committed if it continues to test well. Does anyone have issues with this being committed as long as it works as intended? I really think it will make adding new SQL for AAs considerably easier.

Cavedude is right that we should all be working off the same table when creating or fixing AAs. But, with this code, we could be working from completely different tables and it shouldn't make any difference.

[AndMetal]

Because of the database changes needed, I'm going to hold off on committing this for now until everyone feels comfortable with making the switch.

Has anyone had a chance to work with this yet? If so, I think we might be able to start adding more missing AAs.

[trevius]

I would love to see this get implemented. I think cavedude should have the final word, since the PEQ database is the most important thing to have adjusted for this change. Once we have a good table to work from that has these modifications in it, it should make adding AAs quite a bit easier. Then, new AA SQL updates could be added to the /sql/svn directory when doing SVN commits just like any other SQL changes.

I think this just got forgotten about after the new AA additions on the SVN died down. I forget if I tested this or not yet. I would test it, but I don't really have much extra free time lately with all of the other stuff I have going :P

[cavedude]

Yeah, I've forgotten about this thread as well. Let me give the code a try, and work out a query to migrate the current AAs to the new system. If all goes well, I don't see a reason not to commit it.

[AndMetal]

Let me give the code a try, and work out a query to migrate the current AAs to the new system.

This is the final migration query, which worked for me when I tested it out a while ago:

SET @row = 0;
UPDATE altadv_vars a SET prereq_skill = COALESCE((SELECT p.skill_id FROM (SELECT a2.skill_id, @row := @row + 1 AS prereq_index_num FROM altadv_vars a2) AS p WHERE p.prereq_index_num = a.prereq_skill), 0) WHERE prereq_skill < 1000000;


To clarify, it worked great for me when I worked on it a few months ago, but because of the need to make 1 migration going forward (and everything could get VERY confusing if you run it multiple times), I didn't want to force everyone into it right away.

I think the only thing I didn't do was combine the classes & berserker columns in the db, then separate them in the code (easy enough to do).

[trevius]

I don't think we need to combine the classes and berserker columns. They are sent to the client that way, so it doesn't hurt having 1 extra field to fill out. It wouldn't hurt to do it, but there isn't really much point to do so. Even on EQLive, it is still sent that same way for some reason. They are probably to lazy to do it :P

EDIT: Woah, I am developer now, lol! Be afraid!

[cavedude]

EDIT: Woah, I am developer now, lol! Be afraid!

Oh crap, I meant to put you in the noob group. My finger musta slipped. Oh well.

[cavedude]

This works perfectly. I don't have a problem with the code and query being committed, unless anybody else has any objections. It even allowed me to correct a couple of Necro AAs that have been nagging at the back of my mind.