Add support for custom polls via plug-ins #61

allejo · 2017-01-16T04:33:08Z

This pull request introduces the ability to create custom poll types to the API. A custom poll type is defined as adding your own action to /poll <action>. I've added a sample plug-in showing how to create a primitive /poll mute option.

New API Events + Data Objects

bz_eAllowPollEvent -> bz_AllowPollEventData_V1
bz_ePollStartEvent -> bz_PollStartEventData_V1
bz_ePollVoteEvent -> bz_PollVoteEventData_V1
bz_ePollVetoEvent -> bz_PollVetoEventData_V1
bz_ePollEndEvent -> bz_PollEndEventData_V1

New API Functions

bool bz_registerCustomPollOption (const char* option, const char* parameters, bz_CustomPollOptionHandler *handler);
bool bz_removeCustomPollOption (const char* option);

bz_CustomPollOptionHandler Usage

bool PollOpen (bz_BasePlayerRecord *player, const char* action, const char* parameters)

This function is called before every custom poll begins. When this function returns true, the server will proceed and begin the poll. If the plugin returns false, then the poll will not begin. This function is intended for allowing plug-in developers to check conditions or permissions before the poll is begun. Any messages or errors that occur should be sent to the player from this function, otherwise it will appear as if the poll failed to start.

player - the player record of the player who would like to initiate a poll
action - the action is that being polled, e.g. 'mute'
value - the value or target of the action, e.g. 'talkative_player'

void PollClose (const char* action, const char* parameters, bool success)

This function is called after every custom poll has ended. This is where the business logic of a successful poll should be defined.

action - the action is that being polled, e.g. 'mute'
value - the value or target of the action, e.g. 'talkative_player'
success - whether or not the poll succeeded

I'd appreciate feedback and help testing too ❤️

jwmelto · 2017-01-17T00:41:47Z

Be sure to change the comments (in the header files, at a minimum) to reflect the change in API. For example, in VotingArbiter.h you changed "std::string getPollRequestor()" to "int getPollOrganizer" but the comments still say "return a string representing the player who requested the poll".

jwmelto · 2017-01-17T00:44:13Z

this is a nit, but "const int" as a function parameter is silly. There is no valid reason to declare a pass-by-value parameter "const" since it's impossible to change the underlying value. It is an artifact of a global replace of "std::string&" to "int" in the signatures.

jwmelto · 2017-01-17T00:48:56Z

This is one of my pet peeves, but if you are modifying a ctor, make every effort to initialize members in the member initializer list, and not in the function body.

jwmelto · 2017-01-17T00:52:51Z

include/bzfsAPI.h

+  bz_ePollStartEvent,
+  bz_ePollVoteEvent,
+  bz_ePollVetoEvent,
+  bz_ePollEndEvent,


Doesn't this change require incrementing the API version? Previously compiled plugins are likely to be incompatible.

allejo · 2017-01-17T01:53:28Z

@jwmelto I've made some commits (that I'll squash when this is ready to merge).

Regarding your comment about the API version, I've bumped BZ_API_VERSION but I didn't know that's what that number was for. Maybe @JeffM2501 could confirm?

As for initializing members, would I do all of the members or only the members that would have default values?

I've also fixed the comment in VotingArbiter and changed it to just an int instead of a const int

jwmelto · 2017-01-17T03:00:01Z

sorry for the spam comments. I finally figured out how to group them in a review... but to member initialization: default initialization may be elided, but may be explicit for pedantic and documentation purposes.
http://www.cplusplus.com/forum/articles/17820/

allejo · 2017-01-17T03:53:57Z

So taking bz_PollStartEventData_V1 as an example, would I just initialize all those strings to empty strings and BZ_SERVER for the playerID?

VotingArbiter will keep track of player IDs now and fetch the player callsigns from that ID when a poll is created

jwmelto

Seems like we should seriously consider refactoring all of the server functions into built-in plugins. This is reasonable and no glaring errors, but the maintenance would be cleaner if there were just "plugins"

jwmelto · 2017-01-17T01:10:15Z

include/bzfsAPI.h

+  bz_ApiString pollTarget;
+
+  // This value will be set to the victim's IP if it's a 'kick' 'kill' or 'ban'
+  bz_ApiString pollValue;


overloaded values are a poor API. Wouldn't it be better to have a polymorphic "poll details" structure where each type of poll carries its own details?

pollValue since then has been removed since it's only available on certain occasions and having overload values are a pain, as you said. Besides, the information is only an IP so that can be fetched/stored by the plug-in

jwmelto · 2017-01-17T05:15:22Z

include/VotingArbiter.h

+struct PollOption {
+  std::string pollParameters;
+  bz_CustomPollOptionHandler* pollHandler;
+};


This structure needs a ctor to initialize the pollHandler member to 0 or else the map below will have incoherent semantics.

jwmelto · 2017-01-17T05:16:44Z

include/VotingArbiter.h

+  std::string pollParameters;
+  bz_CustomPollOptionHandler* pollHandler;
+};
+extern std::map<std::string, PollOption> customPollOptions;


I haven't gotten the the body yet, but I suspect that exposing the global is neither necessary or prudent. It will interfere with any future goals of writing threaded code.

Without making this extern, I wouldn't be able to access the map in other files as is. Do you have a better approach in mind for this?

jwmelto · 2017-01-17T05:45:50Z

src/bzfs/commands.cxx

-  if (vote == 0) {
-    if ((cast = arbiter->voteNo(callsign)) == true) {
-      /* player voted no */
+  if (vote == 0 || vote == 1) {


minor point, but this kind of value checking is a maintenance issue. The logic is that "vote" is initialized to -1, and it only gets a different value if a valid "yes" or "no" answer is provided. So this test is more completely "vote != -1" or "vote > -1".

The problem with this logic is seen on line 2668. What is really needed is a "tri-state" boolean that carries the information of whether it has been initialized, and whether it is true or false. Ideally, it would have an "operator bool()" method so it could be tested here and assigned natively on line 2668. But that's a nuance that doesn't need fixing here.

jwmelto · 2017-01-17T05:58:50Z

src/bzfs/commands.cxx

+    bz_AllowPollEventData_V1 allowPollData;
+    allowPollData.playerID = t;
+
+    worldEventManager.callEvents(bz_eAllowPollEvent, &allowPollData);


shouldn't the "allow" event handling include the type of poll? Might a plugin want to allow a player to do some kind of poll and not another?

jwmelto · 2017-01-17T06:08:52Z

src/game/VotingArbiter.cxx


  /* poof */
  _suffraged.clear();

  return true;
 }

-bool VotingArbiter::poll(const std::string &player, const std::string &playerRequesting, std::string action, std::string playerIP)
+bool VotingArbiter::poll(const std::string &target, const int playerRequestingID, std::string action, std::string playerIP)


"const int" is superfluous. "int" is sufficient.

macsforme · 2017-01-17T07:38:45Z

I've made some commits (that I'll squash when this is ready to merge).

Kind of a side note, but I get the impression that you're planning on making a habit of squashing your pull requests when you merge them in. It would be one thing if all development was done in pull requests, and our main repository's history was basically a list of merges. However, since it's not, I personally think that commit history from within pull requests is just as useful as that of commits we push to the main repository, so I would encourage you to keep them intact.

allejo · 2017-01-17T17:26:46Z

include/bzfsAPI.h

@@ -45,7 +45,7 @@

 class bz_Plugin;

-#define BZ_API_VERSION	26
+#define BZ_API_VERSION	27


after confirmation, no version bump is needed. it's only necessary when existing constructors, objects, or functions are modified

allejo · 2017-01-17T18:19:24Z

plugins/customPollOptionSample/customPollOptionSample.cpp

@@ -0,0 +1,80 @@
+// CustomZoneSample.cpp : Defines the entry point for the DLL application.


fix the copy paste

allejo · 2017-01-17T18:29:38Z

plugins/customPollOptionSample/customPollOptionSample.cpp

+  Flush();
+
+  // Remove the poll option when this plugin is loaded or else what other plugin would handle it?
+  bz_removeCustomMapObject("mute");


bz_removeCustomPollOption*

allejo · 2017-01-17T18:35:02Z

include/bzfsAPI.h

+
+  // Should return false to prevent the poll from starting
+  virtual bool PollOpen  (int playerID, bz_ApiString action, bz_ApiString value) = 0;
+  virtual void PollClose (bz_ApiString action, bz_ApiString value, bool success) = 0;


since these values won't change/shouldn't be changed, making them const char* would work

bz_eAllowPollEvent -> bz_AllowPollEventData_V1 Called before a poll is started to give plug-in developers access to prevent a poll from starting bz_ePollStartEvent -> bz_PollStartEventData_V1 Called whenever a new poll is started bz_ePollVoteEvent -> bz_PollVoteEventData_V1 Called whenever a vote is cast. Grants access to plug-ins to disallow votes from being cast bz_ePollVetoEvent -> bz_PollVetoEventData_V1 Called whenever an admin /veto's a poll or bz_pollVeto() is called. If bz_pollVeto() is called, the playerID will be set to BZ_SERVER bz_ePollEndEvent -> bz_PollEndEventData_V1 Called whenever a poll is closed without being vetoed

- Created a bz_CustomPollOptionHandler class with two virtual functions: PollOpen() Called before a custom poll is started and allows plug-in developers to have any necessary checks and disallow the poll, for example. PollClose() Called at the end of a custom poll that has ended without being vetoed. This is were plug-in developers can implement the logic for what the poll should do. - Add two API functions bz_registerCustomPollOption() bz_removeCustomPollOption() - Custom polls are stored in a map stored in VotingArbiter - Custom poll options are displayed when an invalid '/poll' command is used in addition to the built-in ones (e.g. kick, ban, kill)

This plug-in is designed to demonstrate how to create custom polls

This will allow plugins to decide whether they need to handle the poll or not

- Fix misc issues from feedback (BZFlag-Dev#61) - bz_PollStartEventData_V1 no longer contains the 'pollValue' variable. This information is only available on certain cases and overloaded values are poor in an API. This information contains the IP of the targeted player and there are other ways of getting that information

allejo · 2017-01-20T08:26:16Z

Made some updates with the given feedback. Still waiting on a few more opinions before this can be merged in.

allejo · 2017-01-20T20:19:23Z

I think I'm all set. Any other feedback is welcome and would be appreciated before I merge this in the coming days.

macsforme

This looks good to me, and I'm looking forward to having this new feature available.

Since values being passed to these functions won't need to be changed, it's better to make them 'const char*' and instead of passing the player ID as an int we'll pass through a player record

JMakey · 2017-02-04T02:08:08Z

I know this is tangential to this pull request, but this program:

#include <stdio.h>
static int f(const int value)
{ return printf("value = %d\n", ++value); }
int main()
{ return f(3); }

illustrates the type of situation where it may be reasonable to declare a function parameter as const int. It indicates to the compiler that the value variable is not to be modified within the function, and gcc dutifully throws an error at the attempt to modify it. Such a style can help prevent programmers from mistakenly thinking that the modified variable will be passed back outside of the function.

jwmelto · 2017-02-05T04:16:06Z

So are you advocating for the style? Because if a programmer doesn't know the difference between "int value" and "int& value", it's unlikely that even the compiler error is going to help them.

The function signature is a contract between the caller and the writer of the method, not between the writer and himself. YMMV

JMakey · 2017-02-06T05:24:10Z

A programmer can know the difference between those two declarations and still fail to be aware of which is in effect for a specific variable. You may not need more safety in your programming style, but I appreciate it when I see code that shows an extra effort (a not-so-superfluous use of const in this case) to prevent errors both in the past and in the future.

allejo added the enhancement label Jan 16, 2017

allejo added this to the 2.4.10 milestone Jan 16, 2017

allejo self-assigned this Jan 16, 2017

allejo force-pushed the feature/poll_wip branch 4 times, most recently from 096db40 to a998b29 Compare January 17, 2017 00:13

jwmelto reviewed Jan 17, 2017

View reviewed changes

Change how VotingArbiter tracks players

4d77ece

VotingArbiter will keep track of player IDs now and fetch the player callsigns from that ID when a poll is created

allejo force-pushed the feature/poll_wip branch 2 times, most recently from bcb07d5 to 217804b Compare January 17, 2017 05:45

jwmelto reviewed Jan 17, 2017

View reviewed changes

allejo commented Jan 17, 2017

View reviewed changes

allejo added 5 commits January 19, 2017 23:32

Add customPollOptionSample plugin

0a5cba7

This plug-in is designed to demonstrate how to create custom polls

Poll Allow Event now contains action & value

ebedbd1

This will allow plugins to decide whether they need to handle the poll or not

allejo force-pushed the feature/poll_wip branch from 217804b to 0ebffef Compare January 20, 2017 08:21

allejo force-pushed the feature/poll_wip branch from 1cfbf61 to 54c8ee0 Compare January 25, 2017 06:50

macsforme approved these changes Jan 25, 2017

View reviewed changes

allejo force-pushed the feature/poll_wip branch from 54c8ee0 to 82c5284 Compare January 25, 2017 19:40

allejo added 2 commits January 27, 2017 00:13

Change function signature for poll option handler

c9cdba8

Since values being passed to these functions won't need to be changed, it's better to make them 'const char*' and instead of passing the player ID as an int we'll pass through a player record

Rename PollOption to PollType

c15c18a

allejo force-pushed the feature/poll_wip branch from e180807 to c15c18a Compare January 27, 2017 08:21

Use correct parameter in PollOpen() demo

cd1f01a

allejo merged commit f15deaa into BZFlag-Dev:2.4 Jan 27, 2017

allejo deleted the feature/poll_wip branch January 27, 2017 09:09

allejo changed the title ~~[WIP] Add support for custom polls via plug-ins~~ Add support for custom polls via plug-ins Feb 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for custom polls via plug-ins #61

Add support for custom polls via plug-ins #61

allejo commented Jan 16, 2017 •

edited

jwmelto commented Jan 17, 2017

jwmelto commented Jan 17, 2017

jwmelto commented Jan 17, 2017

jwmelto Jan 17, 2017

allejo commented Jan 17, 2017

jwmelto commented Jan 17, 2017

allejo commented Jan 17, 2017

jwmelto left a comment

jwmelto Jan 17, 2017

allejo Jan 20, 2017

jwmelto Jan 17, 2017

jwmelto Jan 17, 2017

allejo Jan 20, 2017

jwmelto Jan 17, 2017

jwmelto Jan 17, 2017

jwmelto Jan 17, 2017

macsforme commented Jan 17, 2017

allejo Jan 17, 2017

allejo Jan 17, 2017

allejo Jan 17, 2017

allejo Jan 17, 2017

allejo commented Jan 20, 2017

allejo commented Jan 20, 2017

macsforme left a comment

JMakey commented Feb 4, 2017

jwmelto commented Feb 5, 2017

JMakey commented Feb 6, 2017

		@@ -0,0 +1,80 @@
		// CustomZoneSample.cpp : Defines the entry point for the DLL application.

Add support for custom polls via plug-ins #61

Add support for custom polls via plug-ins #61

Conversation

allejo commented Jan 16, 2017 • edited

New API Events + Data Objects

New API Functions

bz_CustomPollOptionHandler Usage

jwmelto commented Jan 17, 2017

jwmelto commented Jan 17, 2017

jwmelto commented Jan 17, 2017

Choose a reason for hiding this comment

allejo commented Jan 17, 2017

jwmelto commented Jan 17, 2017

allejo commented Jan 17, 2017

jwmelto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macsforme commented Jan 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allejo commented Jan 20, 2017

allejo commented Jan 20, 2017

macsforme left a comment

Choose a reason for hiding this comment

JMakey commented Feb 4, 2017

jwmelto commented Feb 5, 2017

JMakey commented Feb 6, 2017

allejo commented Jan 16, 2017 •

edited