You know, I have thought about just using a single mine type for a while, and I actually started on a plug-in that only used shock waves. Somehow I managed to lose the code, and I didn't want to start all over again, so I gave up. I have been away from BZFlag as well.
I like that someone made changes to my code
. And you wrote the whole thing with only 250 lines of code. I always end up with a ton more code than I need. The reason is I get stuck on smaller problems, or sub-problems of my original problem. I'll spend a lot of time thinking about better ways doing things. But anyways, I also like the random messages.
Although I probably won't write any plug-ins, I have a few comments on your code. I mean them as constructive comments.
If you use a map instead of a switch statement, your code will clean up better.
Code: Select all
// Define this in your class.
std::map<int, std::string> wittyMsgMap;
// Put this in your constructor.
wittyMsgMap[0] = '"%s was killed by %s's mine. [%d]";
wittyMsgMap[1] = "%s was owned by %s's mine. [%d]";
...
wittyMsgMap[19] = "I ascertain that %s has been ruptured by a mine created from the heavens with the name dubbed %s. [%d]";
// Then send the message.
msg = wittyMsgMap[rand() % wittyMsgMap.size()];
bz_sendTextMessagef(BZ_SERVER, BZ_ALLUSERS, msg.c_str(), pr->callsign.c_str(), kr->callsign.c_str(), CountMines());
All of the functions in your class are declared as virtual. You might want to look up virtual functions. A virtual function is a function that can be re-implemented by a subclass. There are also pure virtual functions. Pure virtual functions must be redefined by every direct subclass. Using virtual functions here is harmless, but I don't think you intend on subclassing the uselessmine class. It would be better to write:
Code: Select all
virtual const char* Name (){return "Useless Mine";}
void Init (const char* /*config*/);
void Event(bz_EventData *eventData);
bool SlashCommand (int playerID,bz_ApiString command,bz_ApiString message,bz_APIStringList* params);
void Cleanup ();
int CountMines();
void SetMine(int playerID,float pos1,float pos2,float pos3);
void RemoveMine(int id);
void RemoveAllMines(int playerID);
There is a better way you could count the mines. Instead of looping through an array every time you want to count the number of mines, you could use a state variable. When someone adds a mine, increment by one, and when someone removes a mine, decrement by one.
What wasn't working with the following code? Using the flag grabbed event would be a better way of handling flag grabbed messages. Maybe instead of getting the flag type from the player, you should get the flag type from the event data? The grabbed flag type is stored in the bz_FlagGrabbedEventData_V1 class. Using the flag grabbed event, you could remove the minenotify array.
Code: Select all
case bz_eFlagGrabbedEvent: {
playerID=((bz_FlagGrabbedEventData_V1*)eventData)->playerID;
/*bz_BasePlayerRecord *pr = bz_getPlayerByIndex(playerID);
//bz_debugMessage(0,bz_getName(((bz_FlagGrabbedEventData_V1*)eventData)->flagID).c_str());
if (pr) {
if (!strcmp(pr->currentFlag.c_str(),"US")) {
bz_sendTextMessage(BZ_SERVER,playerID,"You grabbed a Useless flag! Type /mine at any time to set a useless mine!");
}
bz_freePlayerRecord(pr);
}*/
minenotify[playerID]=1;
}break;
I'm pretty sure calling delete on a null pointer does nothing, so it isn't necessary to check if a pointer actually points to something before deleting it.
Code: Select all
if (pr)
bz_freePlayerRecord(pr);
if (kr)
bz_freePlayerRecord(kr);
If anything, you should perform these checks when you allocate memory.
Code: Select all
bz_BasePlayerRecord *pr = bz_getPlayerByIndex(playerID);
if (!pr)
return;
bz_BasePlayerRecord *kr = bz_getPlayerByIndex(mineplayer[i]);
if (!kr)
{
bz_freePlayerRecord(pr);
return;
}
// If we get to this point, then everything allocated properly.
In my original code, I actually used template metaprogramming to unroll loops at compile time when computing the vector product. I honestly shouldn't have done that because it overly complicated things when it wasn't necessary. My excuse is I was learning about template metaprogramming at the time
. But anyways, I guess you would prefer a bounding box rather than a sphere? I would just use the Euclidean distance
http://en.wikipedia.org/wiki/Euclidean_distance.
Code: Select all
for (i=0;i<255;i++) {
if (mine[i] && mineplayer[i]!=playerID && (pr->team==eRogueTeam || pr->team!=mineteam[i])) {
if (pr->lastKnownState.pos[0]>minepos[i][0]-((bz_getBZDBDouble("_shockOutRadius")*0.75))
&& pr->lastKnownState.pos[0]<minepos[i][0]+((bz_getBZDBDouble("_shockOutRadius")*0.75))
&& pr->lastKnownState.pos[1]>minepos[i][1]-((bz_getBZDBDouble("_shockOutRadius")*0.75))
&& pr->lastKnownState.pos[1]<minepos[i][1]+((bz_getBZDBDouble("_shockOutRadius")*0.75))
&& pr->lastKnownState.pos[2]>minepos[i][2]-((bz_getBZDBDouble("_shockOutRadius")*0.75))
&& pr->lastKnownState.pos[2]<minepos[i][2]+((bz_getBZDBDouble("_shockOutRadius")*0.75))
&& safetytime[playerID]<=bz_getCurrentTime()
) {
bz_fireWorldWep("SW",2.0,BZ_SERVER,minepos[i],0,0,i+1000,0);
RemoveMine(i);
}
}
}
I think that is about it. My only other suggestion would be to use standard library containers instead of constant length arrays. You would need a random access container, since removing "mines" requires random access. That is, you need to be able to remove elements from the middle of the array.
