Many bugs found in tmwAthena server code
I was writing scripts and had so much trouble that I read the source code and made a script command guide.
During this process I found a number of BUGS in the tmwAthena server code.
// BUGS
BUG: builtin_getarraysize
Error message identifies it as function "builtin_copyarray".
Message should have "builtin_getarraysize".
BUG: builtin_call
Error message identifies it as function "callfunc".
Message should have "builtin_call".
BUG:
builtin_goto
builtin_callfunc
builtin_call
builtin_callsub
menu
builtin_else
builtin_elif
This executes abort when not a label. This is too severe as would
kill entire script engine.
Should execute something with more
graceful recovery, like "builtin_end".
BUG:
Game code should not have have any "assert" that are not conditional
on DEBUG.
These can halt entire server.
A more graceful recovery is always more practical.
Kill the script, do not kill the server.
Perhaps you have a way to disable all assert, but it makes me nervous to see them, one mistake.
BUG: builtin_countitem
PRINTF message has different format than any other error message.
Should be "builtin_countitem: wrong item ID (%i)".
BUG: builtin_npcareawarp
The vars lx,ly are initialized to -1, but are never set to any other
values. This defeats the intended usage as the last valid position found.
Replace this code:
Code: Select all
max = (y1 - y0 + 1) * (x1 - x0 + 1) * 3;
if (max > 1000)
max = 1000;
P<map_local> m = nd->bl_m;
if (cb) {
do
{
x = random_::in(x0, x1);
y = random_::in(y0, y1);
}
while (bool(map_getcell(m, x, y) & MapCell::UNWALKABLE)
&& (++j) < max);
if (j >= max)
{
if (lx >= 0)
{ // Since reference went wrong, the place which boiled before is used.
x = lx;
y = ly;
}
else
return; // Since reference of the place which boils first went wrong, it stops.
}
}
else
{
x = random_::in(x0, x1);
y = random_::in(y0, y1);
}
With this code:
Code: Select all
P<map_local> m = nd->bl_m;
if (cb)
{
int j = 0;
int max = (y1 - y0 + 1) * (x1 - x0 + 1);
if (max > 750)
max = 750;
do
{
x = random_::in(x0, x1);
y = random_::in(y0, y1);
if( ! bool(map_getcell(m, x, y) & MapCell::UNWALKABLE ) goto warp_the_npc;
} while ((++j) < max);
// Search went wrong, find any WALKABLE spot.
do
{
// Increment from last random position, searching for a WALKABLE.
x++;
if( x > x1 )
{
x = x0;
y++;
if( y > y1 ) y = y0;
}
if( ! bool(map_getcell(m, x, y) & MapCell::UNWALKABLE ) goto warp_the_npc;
} while( --j > 0 )
return; // WALKABLE place was not found, do not warp.
}
else
{
x = random_::in(x0, x1);
y = random_::in(y0, y1);
}
warp_the_npc:
BUG: builtin_getusers
FIXME
BUG: builtin_getusers
// Has disabled code due to SIGSEG.
Replace this code:
Code: Select all
val = 0;
switch (flag & 0x07)
{
case 0:
// FIXME: SIGSEGV error
//val = bl->bl_m->users;
PRINTF("Sorry, but getusers(0) is disabled. Please use getmapusers() instead.\n"_fmt);
break;
With this code:
Code: Select all
val = 0; // by default 0 users
switch (flag & 0x07)
{
case 0:
// Given protection against segfault
if( bl && bl->bl_m )
{
val = bl->bl_m->users;
}
break;
Existing scripts only have getusers(1),
so it could be improved if that was preserved.
So could use this improved code:
Code: Select all
dumb_ptr<block_list> bl = map_id2bl((flag == 0) ? st->oid : st->rid);
val = 0; // by default 0 users
switch (flag & 0x07)
{
case 0: // NPC map
case 2: // Player map
// Given protection against segfault
if( bl && bl->bl_m )
{
val = bl->bl_m->users;
}
break;
BUG: builtin_camera
The "rid" and "player" options are broken due to
an IF-stmt not blocking the final ELSE clause.
Existing code:
Code: Select all
dumb_ptr<block_list> bl;
short x = 0, y = 0;
bool rel = false;
if (auto *u = AARG(0).get_if<ScriptDataInt>())
bl = map_id2bl(wrap<BlockId>(u->numi));
if (auto *g = AARG(0).get_if<ScriptDataStr>())
{
if (g->str == "rid"_s || g->str == "player"_s)
bl = sd;
if (g->str == "relative"_s)
rel = true;
else if (g->str == "oid"_s || g->str == "npc"_s)
bl = map_id2bl(st->oid);
else
bl = npc_name2id(stringish<NpcName>(g->str));
}
if (HARG(1) && HARG(2))
{
x = conv_num(st, &AARG(1));
y = conv_num(st, &AARG(2));
}
if (rel)
clif_npc_action(sd, st->oid, 4, 0, x, y); // camera relative from current camera
else
clif_npc_action(sd, st->oid, 2, unwrap<BlockId>(bl->bl_id), x, y); // camera to actor
Replace with the following code:
Code: Select all
dumb_ptr<block_list> bl;
short x = 0, y = 0;
if (HARG(1) && HARG(2))
{
// default is 0,0
x = conv_num(st, &AARG(1));
y = conv_num(st, &AARG(2));
}
if (auto *u = AARG(0).get_if<ScriptDataInt>())
{
// interpret as map_id
bl = map_id2bl(wrap<BlockId>(u->numi));
}
else if (auto *g = AARG(0).get_if<ScriptDataStr>())
{
if (g->str == "relative"_s)
{
clif_npc_action(sd, st->oid, 4, 0, x, y); // camera relative from current camera
return;
}
else if (g->str == "rid"_s || g->str == "player"_s)
bl = sd; // player map
else if (g->str == "oid"_s || g->str == "npc"_s)
bl = map_id2bl(st->oid); // script NPC map
else
bl = npc_name2id(stringish<NpcName>(g->str)); // NPC name
}
clif_npc_action(sd, st->oid, 2, unwrap<BlockId>(bl->bl_id), x, y); // camera to actor
return;
I am sure you will want to look over these code suggestions.
However they are bugs and something will need to be done about them by somebody.
