Roll20 uses cookies to improve your experience on our site. Cookies enable you to enjoy certain features, social sharing functionality, and tailor message and display ads to your interests on our site and others. They also help us understand how our site is being used. By continuing to use our site, you consent to our use of cookies. Update your cookie preferences .
×
Create a free account

[Script] Pathfinder Diplomacy Check

September 17 (9 years ago)

Edited September 17 (9 years ago)
This is my first script in the API, so it's not much. I am very open to constructive criticism from all of you! And if someone other than me actually finds this useful and would like additional features then please post and I will see what I can do.


Purpose: This script is intended to manage the base Diplomacy DC  and attitudes of NPC characters in the Pathfinder system. It assumes the Pathfinder character sheets are being used and will use the current CHA-mod for a character. It will create any attributes that are not present assuming a default attitude of "Indifferent" and a CHA score of 12. If you wish a character to start with an attitude other than "Indifferent" you will need to manually create a "Disposition" attribute on the character and set it to the desired value.


Commands:

!diplomacyCheck

This command will whisper the GM the current disposition and base Diplomacy DC of an NPC represented by a selected token.

!diplomacyCheck score
If you include a numeric score representing the Diplomacy Check score of a PC trying to change the attitude of the NPC the script will instead compare that score to the DC of the NPC represented by the token you have selected and change that NPC's attitude accordingly. The script will whisper the results of the check to the GM.

Code: Download from GitHub

September 18 (9 years ago)
The Aaron
Pro
API Scripter
Nice first addition!

Since you're asking for constructive criticism, I'll point out a few things.
  • Since all of your sendChat()s begin with "/w gm", it seems evident you only intend GMs to use this script.  You might want to change line 7 to be:
if('api' !== msg.type || !playerIsGM(msg.playerid) ) {
	return;
}
That will cause the script to terminate early if the player executing it is not a GM.

  • For clarity, you might consider extracting all your embedded functions outside your chat handler:
(function(){
  var checkTokenRepresents = function (tokenID) { /* ... */ };
  var defaultAttributeValue = function (input) { /* ... */ };
  var getCharacterAttribute = function (tokenID, target) { /* ... */ };
  var diplomacyDCDisposition = function (tokenID) { /* ... */ };
  var getCHAMod = function (tokenID) { /* ... */ };
  var stepUpDispositionTwo = function (tokenID, attribute) { /* ... */ };
  var stepUpDisposition = function (tokenID, attribute) { /* ... */ };
  var stepDownDisposition = function (tokenID, attribute) { /* ... */ };

  on('chat:message',function(msg){
    // ...

    if(checkTokenRepresents(selectedTokenID)) {

    // ...
  });

}());
(At that point, you're very close to The Module Pattern that I use, which is quite nice for several reasons...)

  • stepUpDisposition, stepUpDisplositionTwo and stepDownDisposition are very similar.  Right now they each do 2 things:
    • Get a starting disposition from a character
    • return a new disposition based on that disposition.
I would pass in the starting disposition instead of a tokenID and let them return the new disposition based on that.  Further, I'd combine them all into an adjustedDisposition function which takes a disposition and an adjustment amount (1, 2, -1, -2, etc), then returns the disposition that is that many steps away in the right direction.   You could store the dispositions in an array and just calculate the index:
var adjustedDisposition = function(start,steps){
var dispositions = ['Hostile', 'Unfriendly', 'Indifferent', 'Friendly', 'Helpful'];
return dispositions[
Math.max(0,Math.min(dispositions.length-1,
( -1 !== _.indexOf(dispositions, start) ? _.indexOf(dispositions, start) : 2 )
+ (_.isNumber(steps) ? steps : 0)
))
];
};

That's probably enough for now. =D  Keep up the scripting!
September 18 (9 years ago)
Thank you very much for the suggestions (all of which are great, especially the disposition array... that one was eluding me) and the encouragement Aaron! I've updated the script to include your suggestions.
September 18 (9 years ago)

Edited September 18 (9 years ago)
The Aaron
Pro
API Scripter
No worries. =D  It's fun to help.  I've got a few more thoughts for you:

  • You can apply the same logic you did the the step*Disposition*() functions with the diplomacyDCDispostion() function.  Right now it's doing 2 things:
    • Get a disposition from a character
    • return a DC based on that disposition.
I meant to point out above the Single Responsibility Principle from Robert C. Martin's SOLID Principles of Object Oriented Design.  The basic ideas is that any object should have just one reason to change.  It's described colloquially as Curly's rule from City Slickers: "One thing!"

Anyway, when I look at code, I try to find places where there are duplication or places where something feels like it's not in the right place.  In the case of those functions and this function, the fact that they are looking up a character attribute is a red flag, because they don't have anything to do with characters, just dispositions.  I can imagine situations where you might want to list the DC for a particular disposition, but you don't have a character, or even a token.  Should you still be able to find the DC of a disposition in that case?  I'd say yes, so that lookup doesn't belong in there.
var diplomacyDCDisposition = function (start) {
    var dispositions = {'Hostile':25, 'Unfriendly':20, 'Indifferent':15, 'Friendly':10, 'Helpful':0};
    return ( _.has(dispositions, start) ? dispositions[start] : 15 );
};
It also makes testing easier.  The other function depended on token Ids and lookup functions, this one I can test in the javascript console of Chrome without creating a bunch of mock functions.  This implementation uses an object to lookup the DC, and defaults to 15 if it doesn't know about the disposition passed in.

I'm starting to see a whole little dispositions subsystem within your script.  You can almost imagine a sub-module that just handles translating dispositions around.  Shifting them by steps, providing their DCs, etc.  Exercise for the User. =D

  • One thing I did in the extraction of functions above is wrap the whole thing in an immediately executing anonymous function.  This is a technique that lets you encapsulate code within a scope that is separated from the scope around it.  It is the first half of creating a closure (some might argue it is a closure, but there is intent to be considered).  In particular, what it does above is create a smaller scope where your functions are defined, such that only the functions in that scope (in that closure) can access them.  It also prevents them from being overwritten by other scripts and keeps them from overwriting other scripts.
(function(){
  /* your code */
}());
It is common practice to wrap an immediately executing anonymous function in parentheses so that other programmers will recognize it for what it is.

  • Speaking of duplication, looking at your getCharacterAttribute()/setCharacterAttribute() functions, they each do the following:
    var token = findObjs({type: 'graphic', _id: tokenID })[0];
    var character = findObjs({type: 'character', _id: token.get('represents')})[0];
These are also the first 2 out of 3 lines of checkTokenRepresents().  You could replace them in both functions with:
var character = checkTokenRepresents(tokenID);
Additionally, you're calling getCharacterAttribute() a minimum of 8 times, which means you're looking up the same information with findObjs() over and over.  findObjs() is a fairly expensive operation, as such things go, as it has to look through every defined object, O(n) runtime.  That means as games get more complicated, it will get slower and slower.  I've seen some scripts that used it and ran so slowly that they had to break up the execution of the script to run over the course of 20 or so timeouts.  Yikes!

Luckily, it's pretty easy to avert this problem by introducing some caching.  Reducing the duplication to just calling checkTokenRepresents() means you only have to add the caching logic in one place.  =D
var tokenCharacterCache = {};

var checkTokenRepresents = function (tokenID) {
    if(_.has(tokenCharacterCache,tokenID)){
        return tokenCharacterCache[tokenID].character;
    } 

    var token = findObjs({type: 'graphic', _id: tokenID })[0];
    var character = findObjs({type: 'character', _id: token.get('represents')})[0];
    tokenCharacterCache[tokenID]={
        token: token,
        character: character
    };
    return character
}
You might also consider renaming it to getCharacterIfExists() or similar, to reflect what its roll has become.

Second installment finished!  =D  I'm happy to see you embracing scripting and scripting improvements.  It's very hard as a programmer to be "ego-less" about your code, and many programmers struggle with it (I do!!!).  You seem to be off to a great start!
September 18 (9 years ago)
vÍnce
Pro
Sheet Author
 Aaron must have just downed a triple espresso...  Lol
Thanks for sharing your script.  I can't wait to use it in-game.  Cheers
September 18 (9 years ago)
The Aaron
Pro
API Scripter
Ha!  I read that as "downloaded a triple espresso".  =D

No, just happy to help when someone asks for feedback. =D
September 18 (9 years ago)
The script has been updated again.

Thank you again Aaron for the great feedback. I really appreciate it. Especially the cacheing! I'm a long-time graduate student and a scientist so I'm VERY used to having people point out everything that is wrong/can be improved with my work in great detail! =D

And I hope it works alright for you Vince!
September 19 (9 years ago)

Edited September 19 (9 years ago)
The Aaron
Pro
API Scripter
And now I feel foolish for not pointing out getObj(), which takes a type and ID and returns the matching object in O(1) time.
var checkTokenRepresents = function (tokenID) {
    if(_.has(tokenCharacterCache,tokenID)){
        return tokenCharacterCache[tokenID].character;
    } 

    var token = getObj('graphic', tokenID );
    if(token) {
      var character = getObj('character', token.get('represents'));
    }

    tokenCharacterCache[tokenID]={
        token: token,
        character: character
    };
    return character
}
I also wrapped the character getObj() in a guard to prevent an error if token is undefined.  The return and caching is fine, as they will just have undefined in them, and cache the fact that you know you don't have a matching character. =D
September 23 (9 years ago)
No worries. I never would have figured that out. Again, thank you very much for all the help. It is greatly appreciated.
September 23 (9 years ago)
The Aaron
Pro
API Scripter
No problem, anything I can do to help!

If you're interested in digging in to Javascript, I highly recommend Javascript: The Good Parts by Douglas Crockford.
September 23 (9 years ago)
vÍnce
Pro
Sheet Author

The Aaron said:

No problem, anything I can do to help!

If you're interested in digging in to Javascript, I highly recommend Javascript: The Good Parts by Douglas Crockford.

Knew that was coming... lol ;-P
September 23 (9 years ago)
The Aaron
Pro
API Scripter
Yeah, I've been lax lately. =D