ADE_FUNC(getColor, l_Team, NULL, "Gets the IFF color of the specified Team", "number, number, number", "rgb color for the specified team or nil if invalid") {
int idx;
int r,g,b;
if(!ade_get_args(L, "o", l_Team.Get(&idx)))
return ADE_RETURN_NIL;
if(idx < 0 || idx >= Num_iffs)
return ADE_RETURN_NIL;
color* col = iff_get_color_by_team(idx, 0, 0);
r = col->red;
g = col->green;
b = col->blue;
return ade_set_args(L, "iii", r, g, b);
}
Arg, this should really be implemented with a color object. Not that it exists at this point but it would be easy to change the existing functions to have one. Returning a nil is also absolutely unacceptable. The Lua interpreter has a history of irreversibly crashing when it encounters a nil that somebody's treating as something else.
This too:
ADE_FUNC(getFOVs, l_Subsystem, NULL, "Returns current turrets FOVs", "number, number, number", "Standard FOV, maximum barrel elevation, turret base fov.")
{
ship_subsys_h *sso;
float fov, fov_e, fov_y;
if(!ade_get_args(L, "o", l_Subsystem.GetPtr(&sso)))
return ADE_RETURN_NIL;
if(!sso->IsValid())
return ADE_RETURN_NIL;
model_subsystem *tp = sso->ss->system_info;
fov = tp->turret_fov;
fov_e = tp->turret_max_fov;
fov_y = tp->turret_y_fov;
return ade_set_args(L, "fff", fov, fov_e, fov_y);
}
These are three very different things and should have been implemented as virtvars, if possible. Otherwise you end up implementing a set function as well. So you don't really save anything (two functions implemented instead of three) but clarity enormously suffers at this point. The description doesn't really understand what these things are and if you did fully explain, it would make the description enormous and not easy to read when skimming through the scripting reference.
This three:
ADE_FUNC(accessButtonInfo, l_Control_Info, "number, number, number, number", "Access the four bitfields containing the button info", "number, number, number,number", "Four bitfields")
{
int i;
int bi_status[4];
for(i=0;i<4;i++)
bi_status[i] = 0;
if(!ade_get_args(L, "|iiii", &bi_status[0], &bi_status[1], &bi_status[2], &bi_status[3]))
return ADE_RETURN_NIL;
if(ADE_SETTING_VAR) {
for(i=0;i<4;i++)
Player->lua_bi.status[i] = bi_status[i];
}
for(i=0;i<4;i++)
bi_status[i] = Player->lua_bi.status[i];
return ade_set_args(L, "iiii", bi_status[0], bi_status[1], bi_status[2], bi_status[3]);
}
What bitfields? Once again there is not enough description in the description to be of use to somebody who is not already familiar with what this function is supposed to be doing. Whoever implemented this should have taken a step back and asked themselves whether a completely random modder could understand what this function is for. It looks like it was implemented by somebody with knowledge of the codebase. So this probably has some correspondence to some internal code thing, but is not organized so that somebody outside the looking glass can understand what it's for.
In addition, it's not forward-compatible either. I don't know what the bitfields are for but if they correspond to buttons, what if somebody adds another button to the mouse? Also, I added 'enumerations' specifically to get away from bitfields and make scripting easier to use. I don't think I ever fully implemented them to replace bitfields, but the idea was that you could get very far away from some internal code-dependent thing.
So change to a virtvar array and use enumerations for the bitfields. And improve the name and description. It might even be appropriate to make the separate bits to be separate variables and add/or add an object for them.
Just noticed that this function also sets the four bitfields. Definitely change to a virtvar. This is the whole point that they were implemented.
Four:
ADE_FUNC(pollAllButtons, l_Control_Info, NULL, "Access the four bitfields containing the button info", "number, number, number,number", "Four bitfields")
{
int i;
int bi_status[4];
if(!(lua_game_control & LGC_B_POLL_ALL))
return ADE_RETURN_NIL;
for(i=0;i<4;i++)
bi_status[i] = Player->lua_bi_full.status[i];
return ade_set_args(L, "iiii", bi_status[0], bi_status[1], bi_status[2], bi_status[3]);
}
Every single comment above for this function.
And worst of all:
ADE_FUNC(getScreenCoords, l_Vector, NULL, "Gets screen cordinates of a world vector", "number,number", "X (number), Y (number), or false if off-screen")
{
vec3d v3;
if(!ade_get_args(L, "o", l_Vector.Get(&v3)))
return ADE_RETURN_NIL;
vertex vtx;
bool do_g3 = G3_count < 1;
if(do_g3)
g3_start_frame(1);
g3_rotate_vertex(&vtx,&v3);
g3_project_vertex(&vtx);
if(do_g3)
g3_end_frame();
if(vtx.flags & PF_OVERFLOW)
return ADE_RETURN_FALSE;
return ade_set_args(L, "ff", vtx.sx, vtx.sy);
}
I think this is a function I coded, and I feel very much at fault for this. First of all, this absolutely breaks my guarantee that I tried to implement the last time I made a huge revision to the scripting system. That guarantee is that you should always be able to treat a function as if it succeeded and not crash. This function should return a point object that is [0,0] in the case of failure but can be checked with a member function.
Not only that but this is probably the first function that had multiple return values, and that is something I wished to avoid as much as possible, even though Lua lets you do this.
I picked these functions out because I saw the multiple return values, saw that this had been converted to SVN, and went "uh-oh". There is nothing wrong with the coding of these functions and I'm sure that they efficiently solved the problem that whoever implemented them was trying to solve. It's just that they seem to be written from the perspective of simply solving that problem, without much consideration for how it's going to affect (i) forwards compatibility (ii) non-insider readability (iii) stability (iv) existing convention.
That last one needs a bit of justification (since I set the convention I'm effectively saying 'because I said so' and I don't feel that I need to hide behind convention, even though that is generally good programming practice). I'm thinking chiefly of virtvars here, and using those whenever a get/set function pair (or an 'access' function) could be used instead. The reasoning was that it makes the syntax simpler and you could do things like
mn.Ships['Alpha 1'].Shields[SHIELD_FRONT] = 50
As opposed to
mn.getShipByName('Alpha 1').accessShields(SHIELD_FRONT, 50)
This is less immeidately obvious as to what you're doing to someone not already familiar with scripting. But the former is familiar to anybody who's ever worked with any procedural programming language before.
But you might argue it would be more intuitive to have get/set:
mn.getShipByName('Alpha 1').setShields(SHIELD_FRONT, 50)
This is obvious what the intention is. But if you don't implement a 'get' function, it's not immeidately obvious that set also returns the value of the shields. In fact it would be counter-intuitive to have this:
shieldStrength = mn.getShipByName('Alpha 1').setShields(SHIELD_FRONT)
But having a separate 'get' function would force the coder to implement two different functions, and risk things going out of sync.
I apologize for the ginormous message and if you've actually read this far and are still paying attention, I appreciate it. My hope is that some of this explication will draw attention to some of the hidden properties of scripting that aren't immediately obvious but are deliberately there. These are things that I do believe make a scripter's life far more easier in the long-term even if it isn't immediately obvious when only a few functions break the norm.