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

Request for critique of first script

1621624601

Edited 1621626864
Brian C.
Pro
Marketplace Creator
Compendium Curator
I don't write a lot of JavaScript, and I have not written anything for the Roll20 API before. I wanted to patch my 5e adventure products to clear the name from the 5e NPCs to avoid those situations where the NPC name in a template can accidentally give away information about who or what the PCs are fighting. I figured that the easiest (?) way to do this was a custom script. I heavily plagiarized parts from various examples, read a bit of the documentation, and put the following together which seems to work. The "npc_name_flag" on the 5e by Roll20 sheets are set to "0", which avoids emitting the NPC's name when a roll template is triggered from the NPC sheet. Is there anything I am missing? Any deficiencies? Problems that could crop up? Unintended side effects? // Register a function for the 'ready' event. When it occurs, which happens only once // this function will get called. This avoids getting events resulting from the // loading of all the game state into the API on('ready',()=>{ // Declare a function called myFunc. This is using the "Fat Arrow" function syntax // and the const declaration from Javascript ES6 const myFunc = ()=>{ sendChat('myFunc','myFunc was called!'); var characters = findObjs({_type: 'character'}); characters.forEach(function(chr) { var attr = findObjs({name: "npc_name_flag",_type: 'attribute',_characterid: chr.id}, {caseInsensitive: true})[0]; if (!attr) { attr = createObj('attribute', { name: "npc_name_flag", current: "0", max: "", characterid: chr.id }); } else { attr.set('max', ""); attr.set('current', "0"); } sendChat(chr.get('name'), attr.get('current')); }); }; // Register a function for the 'chat:message' event. This event occurs for all // chat messages, so it's important to filter down to just the ones you care about on('chat:message',msg=>{ // First check the type is an API message. API messages are not show shown in chat // and begin with a ! in the first character of the message. // // Next, make sure this is our API message. The regular expression checks that the // command starts with "!call-my-func" and either ends or has a space, all case // insensitive. if('api'===msg.type && /^!call-my-func(\b\s|$)/i.test(msg.content)){ myFunc(); } }); });
1621625559

Edited 1621626065
The Aaron
Roll20 Production Team
API Scripter
Some of those comments look familiar. =D That's a nice first start, here's a few adjustments with commentary to follow: 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 // Register a function for the 'ready' event. When it occurs, which happens only once // this function will get called. This avoids getting events resulting from the // loading of all the game state into the API on ( 'ready' ,() => { // Declare a function called assureNPCFlag. This is using the "Fat Arrow" function syntax // and the const declaration from Javascript ES6 const assureNPCFlag = () => { let characters = findObjs ({ _type : 'character' }); let msgs = []; characters . forEach ( ( chr ) => { let attr = findObjs ({ name : "npc_name_flag" , type : 'attribute' , characterid : chr . id }, { caseInsensitive : true })[ 0 ]; if ( ! attr ) { attr = createObj ( 'attribute' , { name : "npc_name_flag" , current : "0" , max : "" , characterid : chr . id }); msgs . push ( ` Added attribute for $ { chr . get ( 'name' )}. ` ); } else { attr . set ( 'max' , "" ); attr . set ( 'current' , "0" ); msgs . push ( ` Updated $ { chr . get ( 'name' )}. ` ); } }); sendChat ( 'Fix NPC Output' , ` / w gm < div >< h3 > Adjusted $ { msgs . length } Characters < /h3><ul><li>${msgs.join('</li><li>')}</li></ul></div>`); }; // Register a function for the 'chat:message' event. This event occurs for all // chat messages, so it's important to filter down to just the ones you care about on ( 'chat:message' , msg => { // First check the type is an API message. API messages are not show shown in chat // and begin with a ! in the first character of the message. // // Next, make sure this is our API message. The regular expression checks that the // command starts with "!fix-npc-output" and either ends or has a space, all case // insensitive. if ( 'api' === msg . type && /^!fix-npc-output(\b\s|$)/i . test ( msg . content )){ assureNPCFlag (); } }); }); On line 8, I renamed the function to something more descriptive. On lines 9 and 12, I removed var and used let instead.  var and let both define a variable, but let is more precisely scoped and will prevent some problems.  It's better to use let and const in modern javascript and not use var (except in one very strange edge case you're not likely to hit...). Also on line 12, I removed the leading _ on the attribute names.  They're there to tell you those are read only properties, but you can omit them in most cases.  I feel like the code is cleaner without them, and since the sometimes later become read-write properties, you don't end up with a broken script if the _ disappears. I removed the sendChat() that just said the function was called, there's a better output later on line 28. I added a msgs array on line 10, and then collect what was done on lines 21 and 25.  That lets you tailor the message to the action that happened, then pull them all together for output on line 28. On line 28, it outputs how many things were changed, than then has a bulleted list of the changes that happened.  This single message is whispered at the end of the call, so only the GM sees it and it has all the data: On line 40, I changed the command to be more meaningful.
1621625975

Edited 1621627460
The Aaron
Roll20 Production Team
API Scripter
One thing to be aware of for future scripts: you might hit the "Possible infinite loop detected." error with code of this style.  Right now, in a single call stack, you get an array of every character in the game, then iterate over them and get or create an attribute for each one.  You could end up processing so many things that you run out of time and the sandbox considers the process to be hung.  To avoid that, you need to defer your work across several asynchronous calls.  I use a process I call a "Burndown Queue", here's an example: 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 on ( 'ready' ,() => { let attrs = findObjs ({ type : 'attribute' }); let count = attrs . length ; sendChat ( 'FixNaNAttrs' , ` / w gm Processing $ { count } attributes ... ` ); let nanCount = 0 ; const burndown = () => { let a = attrs . shift (); if ( a ){ if ( isNaN ( a . get ( 'current' ))){ a . set ( 'current' , '' ); ++ nanCount ; } if ( isNaN ( a . get ( 'max' ))){ a . set ( 'max' , '' ); ++ nanCount ; } setTimeout ( burndown , 0 ); } else { sendChat ( 'FixNaNAttrs' , ` / w gm Finished . Fixed $ { nanCount } instances of NaN . ` ); } }; burndown (); }); On line 2, it gets an array.  On line 6, it defines a function that processes one entry in the array, then defers a call to process the next one.  Eventually, line 19 gets executed when there are no more entries in the array.  The whole process is kicked off on line 22.
1621626770

Edited 1621628103
Brian C.
Pro
Marketplace Creator
Compendium Curator
The Aaron said: One thing to be aware of for future scripts: you might hit the "Possible infinite loop detected." error with code of this style.  Right now, in a single call stack, you get an array of every character in the game, then iterate over them and get or create an attribute for each one.  You could end up processing so many things that you run out of time and the sandbox considers the process to be hung.  To avoid that, you need to defer your work across several asynchronous calls.  I use a process I call a "Burndown Queue", here's an example: 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 on ( 'ready' ,() => { let attrs = findObjs ({ type : 'attribute' }); let count = attrs . length ; sendChat ( 'FixNaNAttrs' , ` / w gm Processing $ { count } attributes ... ` ); let nanCount = 0 ; const burndown = () => { let a = attrs . shift (); if ( a ){ if ( isNaN ( a . get ( 'current' ))){ a . set ( 'current' , '' ); ++ nanCount ; } if ( isNaN ( a . get ( 'max' ))){ a . set ( 'max' , '' ); ++ nanCount ; } setTimeout ( burndown , 0 ); } else { sendChat ( 'FixNaNAttrs' , ` / w gm " Finished . Fixed $ { nanCount } instances of NaN . ` ); } }; burndown (); }); On line 2, it gets an array.  On line 6, it defines a function that processes one entry in the array, then defers a call to process the next one.  Eventually, line 19 gets executed when there are no more entries in the array.  The whole process is kicked off on line 22. Thanks for your help with this. I didn't clean this up because it was a bit of a throwaway piece, but the changes look great and make sense. For the burndown queue: If I am reading this correctly, you are using a recursive function to iterate through the array and setting a 0ms timeout on each recursive call so other things can happen? Separately, what are you using for the pretty printing of the code block?
1621627326

Edited 1621627400
The Aaron
Roll20 Production Team
API Scripter
That's basically correct, though it's not actually recursive since it doesn't call itself.&nbsp; It schedules itself to be called again at a later point (0ms).&nbsp; Javascript is single threaded, so to get performance and the ability to do many things, it basically uses Cooperative Multitasking.&nbsp; OSs used to do that in the old days, where basically each process would yield it's time running to allow the kernel to schedule another process for a while.&nbsp; In javascript, the way you do that is by calling an asynchronous function.&nbsp; Between each asynchronous call the thread of execution (conceptually) returns back to the sandbox to allow other things to happen, like ticking the watchdog timer that avoids an infinite loop.&nbsp; By deferring the execution by 0ms, it basically calls the function as fast as possible but allows it to be interrupted by other things that take come up. For highlighting, I'm using&nbsp; <a href="http://hilite.me/" rel="nofollow">http://hilite.me/</a> &nbsp;which isn't perfect (has a problem with template literals ` ) but does a nice enough job. =D&nbsp; (I'm using the Tango color scheme.)