battlegrounds, code review and suggestions

Originally posted by [b]theultramage[/b]
http://www.eathena.ws/board/index.php?autocom=bugtracker&showbug=2816

Here are some things I found in the recent changes which I think should be reconsidered. This message is meant primarily for the author, Zephyrus, but other people can also contribute their insight.

1. new monster state 'inmunity'.
   First, it's "immunity" (typo). Then, you added this new state. This is a fairly significant feature, and so should really be added only if there's no other way. Currently you only use it on 1 single spot, which is not good for the code structure. Also, there's no integration with the rest of the code, so for example emperium, which is also 'immune', still uses hardcoded monster id checks. This leads to chaos in the code.
   Also, you suddenly added a new script command "setmobdata", with the only function to set this flag. This is very very very wrong...
2. Multiple calls to status_get_emblem_id / status_get_guild_id were replaced with calls to 'wrapper' functions clif_bg_emblem_id / clif_bg_guild_id.
   Here I don't like the use of the word 'bg' in code that sends normal packets, even outside battlegrounds. Use of 'bg' is misleading here. Also, I don't get a good feeling from those wrapper functions at all. Isn't there some alternative to this?
3. new function pc_update_last_action()
   You rewrote lines "sd->idletime = last_tick;" into calls to "pc_update_last_action(sd);". This function updates the tick like before, and additionally prints "%s is no longer away..." if the player was in battlegrounds and was afk. This is the sole purpose. Is such a silly feature worth the damage this causes to the code?
4. hijacking the guild code for fake guild handling
   There are hooks in multiple places in the clif guild code to handle battlegrounds. like clif_parse_GuildRequestEmblem. Or the "You can't leave battleground guilds." message in guild leave code. Much of the new "clif_bg" packets are copypastes of guild packets, using the same id. I don't like this messy approach.
5. battleground logout and die event
   Handled explicitly in code. Couldn't this instead use the standard events?
6. a ton of new script functions
   Really necessary?
7. new 'battlegrounds' map flag applied to a lot of places
   Too bad it can't be done in the context of a normal GvG map, eh...
8. fake guilds and real guilds
   I saw that the mapserver keeps 2 fake guild structures and emblems, but you also require that the admin make 2 real guilds. Why have this duplicity? Isn't one enough?