Sheet Workers- on change

So I have quite an ever expanding sheet worker section. I have several of the various on event listeners that i need working great. Despite having some complex "on change" listeners working just fine, i'm writing a simple one and have run into trouble, it just doesn't work. The various sheet worker documentation i've read indicates that an on change listener listens for the change of an attribute. I drew from that ( perhaps incorrectly ) if an attribute changes it would be fired, even if it was  done via setAttrs. So my question is that does an on change listener fire if an "input hidden" attribute listened to is modified by setAttrs? My on change (an easy one) doesnt appear to fire at all, none of my debug code tells me that the code block is active.. just trying to rule out the question above as the root cause. (Also Im a software engineer so understand about circular cascades, so dont need any warnings here :) 

Yes, if a hidden attribute changes, it will fire. Check if your cascading sheet workers have {silent: true} set in setAttrs, setAttrs functions with that wont trigger other sheet workers. If that's not the case, there's a lot of things that can be the cause, so you'll need to share more information. It's generally a good idea to keep cascading changes to a minimum (setAttrs triggers another sheet worker, which then triggers another etc) for performance reasons - but as long as the sheet workers are set up properly, cascading sheet workers do work and many sheets rely on them.

So in the snippet below, a change to attr_agility_curr correctly changes attr_agility_mod in the first code block. However in the second code block the on change for attr_agility_mod does not fire...  Im guessing its a simple syntax error at this point but it... alludes me. As a side note to the above: The attr_defense-talent-mod (a direct user input field) when changed doesn't fire the second code block either. on("change:strength change:strength_curr change:constitution change:constitution_curr change:agility change:agility_curr " +         "change:peraware change:peraware_curr change:willpower change:willpower_curr change:intelligence change:intelligence_curr " +          "change:social change:social_curr", function(eventInfo) {         const attrib = eventInfo.sourceAttribute;         getAttrs([attrib], function(values) {             const attribVal = values[attrib];             if (attribVal < 0){                 setAttrs({ [`${attrib}`]: 0});              }             else if (attribVal > 40){                 setAttrs({ [`${attrib}`]: 40});             }             else if (attrib.endsWith("_curr")) {                 setAttrs({ [`${attrib.slice(0, (attrib.length-5))}_mod`]: calculateMod(attribVal) });             }         });     });     on("change:agility_mod change:defense-talent-mod", function() {         getAttrs(["agility_mod", "defense-talent-mod"], function(values) {             let agm = parseInt(values.agility_mod)||0;             let dtm = parseInt(values.defense-talent-mod)||0;             let dr = (10+agm+dtm);             setAttrs({"defense_rating":dr});         });     });

You have a syntax error in the second worker - it is being triggered, but it is crashing. The error is here:  let dtm = parseInt(values.defense-talent-mod)||0; Javascript variable names cant have hyphens in them, so that is being read as  let dtm = parseInt(values.defense)||0; and the rest of that name is causing the crash because JS doesn't know what to do with it. There are two syntaxes you can use, the dot syntax as above, and the [] syntax, which accepts a string, like this:  let dtm = parseInt(values['defense-talent-mod'])||0; You can access properties that have hyphens in them this way - changing your sheet worker to that will fix it. The hyphens on the change and getAttrs line are okay, because they are inside strings. An alternative is of course to replace hyphens with underscores in your variable names. Underscores are valid in JS variable names.

Thanks so much Gigs - i knew that too *shameful glance to the floor* - I can just change them to underscores, it was not supposed to have hyphens if you look at the naming convention i used for other like fields.... Tired eyes and all.   Appreciate the time ! Edit: Made the change it works fine.. Thanks Again !

I do want to reiterate the advice that GiGs gave about avoiding sheetworker cascades. setAttrs and getAttrs are just as efficient whether you are grabbing/setting a single attribute or 10000 attributes. I did some tests with getAttrs and setAttrs a while ago (you can find the original forum post with a google search); getAttrs takes 10's of milliseconds per get and setAttrs takes up to several hundred milliseconds per set. This means that even after a cascade that is just 3 get/sets long, you're looking at potentially half a second of lag on your sheet. Get up to a cascade that is 10 get/sets long, and you're up to multiple seconds of lag (maybe as much as 5 seconds if you're real unlucky with how long each get/set takes to resolve). In contrast, if you grab every attribute you might potentially need to react to a change, you can do a single getAttrs, do all your calculations and aggregate your changes in an object. Your calculations within that single getAttrs will take maybe a few milliseconds instead giving you nearly instantaneous responsiveness.

Scott, Did you look at the code in the example above? I could accomplish the goal of in the second block of code, with a calculated field (rather than a sheet worker), I didn't however as I felt this was better. If you can suggest a way to incorporate the second block of code into the first and still have one setAttrs it would make a time saving. If you can't there will still be two setAttrs, therefore the extra cost of the second block would then only be a second getAttrs and as you mention overhead here is only 10's of milliseconds. As an enterprise architect, if i were to get an unacceptable response rate, i will rework my solution. Right now as it is, it's pretty tight, elegant and quick. Cascading will be kept to a minimum (the suggested goal) one level, and simple. I'm not surprised with your test results, the delta between typical getAttrs and setAttrs are congruent with deltas between reads which are simplistic and write/commits which are not. (Im also an Oracle master certified DBA). I'm not worried at all that a seldom used operation such as changing an ability attribute, also then kicks off an update defense rating calculation. This will be done very rarely not even every level up. A better issue to think about would be the many many dice roles that kick off custom sheet templates that will wait for seconds (i think perhaps a second or so too long) for a 3d dice to complete rolling.. I would agree your advice is good for a layman who may not understand cascades and may develop themselves into a poor situation.

Calculated fields are worse in every way than sheet workers, even cascading ones. What Scott says is absolutely correct , but to me it's more of a "theory clashing against practice" kind of argument - most sheets will never actually need to benefits you'd get from doing it this way. So you can get big theoretically efficiency improvements, absolutely, but it will also create a sheet that is much harder to maintain. And especially for people who are relatively inexperienced with coding, building such a sheet is really hard. And consider: The flagship roll20 sheet - the D&D 5e one - uses cascading effects and relies on them, and if roll20 thought they were a bad thing to be avoided at all costs, that sheet wouldn't use them (nor would the platform enable them). In practice, having a delay of 1 or 2 seconds on your sheet is not a big deal. You cant manually attempt to change attributes fast enough for it to be an issue, and that allows 5-10 cascades, which is more than will ever happen on most sheets. If your sheet isn't a massively complex one, cascades will never be an issue. Recommending sheet authors to avoid them at all costs - when it creates a burden and makes sheets harder to develop - is just bad advice IMO. It is good practice to build your sheet to minimize cascades, but it's always a balancing act between managing code complexity and sheet efficiency, and it can be totally valid to use cascades for convenience.

While I use the 5e sheet, I don't think that it is a good example of coding best practices. Its code is incredibly complex and difficult to understand. As for harder to develop, I think that relying on functions that are called as needed is easier to understand than having several change listeners that react to each other as I find it easier to see interactions when function calls are used than looking for several event listeners that I wrote 6 months ago that are now going to be triggered by new code that I just wrote. Sorrow, for your example code above, at a minimum I'd probably write it like this: let getArr = ['strength','strength_curr','constitution','constitution_curr','agility','agility_curr','peraware','peraware_curr','willpower','willpower_curr','intelligence','intelligence_curr','social','social_curr','agility_mod','defense-talent-mod']; on("change:strength change:strength_curr change:constitution change:constitution_curr change:agility change:agility_curr " + "change:peraware change:peraware_curr change:willpower change:willpower_curr change:intelligence change:intelligence_curr " + "change:social change:social_curr", baseChange); on("change:defense-talent-mod", defenseChanged); const defenseChanged = function() { getAttrs(["agility_mod", "defense-talent-mod"], function(values) { const setObj = {}; setObj.defense_rating = defenseAffected(values); setAttrs(setObj,{silent:true}); }); }; const baseChange = function(eventInfo) { const attrib = eventInfo.sourceAttribute; getAttrs(getArr, function(values) { const attribVal = values[attrib]; const setObj = {}; if (attribVal < 0){ setObj[attrib] = 0; }else if (attribVal > 40){ setObj[attrib] = 40; } values[attrib] = setObj[attrib]; //I'm assuming you actually wanted this if to be separate from the above if/else chain if (attrib.endsWith("_curr")) { let modName = `${attrib.slice(0, (attrib.length-5))}_mod` setObj[modName] = calculateMod(attribVal); values[modName] = setObj[modName]; if(modName === 'agility_mod'){ setObj.defense_rating = defenseAffected(values); } } setAttrs(setObj,{silent:true}); }); }; const defenseAffected = function(values){ let agm = parseInt(values.agility_mod)||0; let dtm = parseInt(values.defense-talent-mod)||0; return (10+agm+dtm); }; For this, I'm assuming that the mods are readOnly, or otherwise not changeable by the user directly. EDIT: Also, if this was my sheet I'd standardize the underscores vs. dashes in attribute names to all use underscores.

Scott C. said: While I use the 5e sheet, I don't think that it is a good example of coding best practices. Its code is incredibly complex and difficult to understand. Honestly, I have to concede that point. :)

GiGs said: Calculated fields are worse in every way than sheet workers, even cascading ones. What Scott says is absolutely correct , but to me it's more of a "theory clashing against practice" kind of argument - most sheets will never actually need to benefits you'd get from doing it this way. So you can get big theoretically efficiency improvements, absolutely, but it will also create a sheet that is much harder to maintain. And especially for people who are relatively inexperienced with coding, building such a sheet is really hard. And consider: The flagship roll20 sheet - the D&D 5e one - uses cascading effects and relies on them, and if roll20 thought they were a bad thing to be avoided at all costs, that sheet wouldn't use them (nor would the platform enable them). In practice, having a delay of 1 or 2 seconds on your sheet is not a big deal. You cant manually attempt to change attributes fast enough for it to be an issue, and that allows 5-10 cascades, which is more than will ever happen on most sheets. If your sheet isn't a massively complex one, cascades will never be an issue. Recommending sheet authors to avoid them at all costs - when it creates a burden and makes sheets harder to develop - is just bad advice IMO. It is good practice to build your sheet to minimize cascades, but it's always a balancing act between managing code complexity and sheet efficiency, and it can be totally valid to use cascades for convenience. I couldn't agree more.

Scott C.  said: //I'm assuming you actually wanted this if to be separate from the above if/else chain For this, I'm assuming that the mods are readOnly, or otherwise not changeable by the user directly. EDIT: Also, if this was my sheet I'd standardize the underscores vs. dashes in attribute names to all use underscores. Scott, 1)  no the if/else chain was how i wanted it, if a stat was altered due to the  >40 or <0 if sections, the setAttr would cause another invocation of the same on change listener, which this time would skip past the 40 and 0 checks and would catch the appropriate  "_curr" third if else section. I liked my solution it was eloquent, even If at the cost of an imperceptible amount of millis. 2) the defense_talent_mod is directly entered. (i didn't share this fact, by only sharing a snippet my apologies here) 3) As soon as GiGs pointed out my syntax error (thanks again) I changed the oversight dashes to underscores as that was my intended naming convention that was used throughout.. I do need to however need to improve my inconsistent use of BEM css naming standard. I also need to rewrite better css so as to further minimise my use of style="" I cant help but want to reuse the same css code block as many times as i can but with small changes applied via style="".... I like your code block that you propose it has some really nifty mechanisms, i guess our styles are different though, you have almost doubled the length of code, and made the code more complex-- opposite from the k.i.s.s. approach we like to take. For me it feels like you've used a sledgehammer to crack a walnut. We will keep the existing code, I'm not sure how it will be in game at this point, but in the sandbox the existing code is instantaneous(low millis as to be imperceptible), fewer lines, and simple. Will be a pleasure to maintain at 2am in the morning (typical dev time for me), and easy for someone else to maintain should I get run over by a bus. I would reserve the overengineering to when i'm making firmware, or when i'm trying to profile and improve regular use code that has a performance problem.   Thanks for the discussion, I really appreciated looking at your solution, and chatting back and forth.

I agree with your point about code length and complexity, but I do very strongly agree with Scott that you should keep the number of setAttrs in a single sheet worker to 1. You can do this in your worker with a very simple change. Change this if  ( attribVal  <  0 ){      setAttrs ({  [ ` ${ attrib } ` ]:   0 });  } else   if  ( attribVal  >  40 ){      setAttrs ({  [ ` ${ attrib } ` ]:   40 }); } else   if  ( attrib . endsWith ( "_curr" )) {      setAttrs ({  [ ` ${ attrib . slice ( 0 , ( attrib . length - 5 )) } _mod` ]:   calculateMod ( attribVal ) }); } to const   output  = {}; if  ( attribVal  <  0 ){      output [ ` ${ attrib } ` ] =  0 ;  } else   if  ( attribVal  >  40 ){      output [ ` ${ attrib } ` ] =  40 ; } else   if  ( attrib . endsWith ( "_curr" )) {      output [ ` ${ attrib . slice ( 0 , ( attrib . length - 5 )) } _mod` ] =  calculateMod ( attribVal ); } setAttrs ( output ); This is what Scott was referring to when he mentioned stirring attributes in an object for setAttrs. Now it looks like only one of the branches of your if statement will ever fire, so its not necessary, but it's best to use this kind of construction whenever using multiple setAttrs in the same worker, to make sure you don't accidentally trigger it multiple times. An object like this can hold any number of attributes, so it is a very efficient way to write your code and make sure you only use one setAttrs. Also you can simplify the code on the first two iff brackes, like const   output  = {}; if  ( attribVal  <  0 ){      output [ attrib ] =  0 ;  } else   if  ( attribVal  >  40 ){      output [ attrib ] =  40 ; } else   if  ( attrib . endsWith ( "_curr" )) {      output [ ` ${ attrib . slice ( 0 , ( attrib . length - 5 )) } _mod` ] =  calculateMod ( attribVal ); } setAttrs ( output ); The object[ ] syntax will affect variables, so you don't need to encode them string literals. The same is true in your original setAttrs code. You only need to use the string syntax in the last one, because its being constructed in parts.

I agree it is a goal (mitigating circumstances excepted) to execute only one setAttrs not multiple in a single sheet worker listener's action function. However that is precisely what's happening in my original code as only one settAttr executes due to the if/else condition statement. (I recognise you noted this yourself also).          I do like the minimalism GiGs of your last example, and it is very nice  and appealing to me to sign off the method with setAttrs at the bottom, i think I will use this for sure, as it feels right when it feels right.. I should also put a comment in for the _curr attributes as these would get changed via recursion of the triggered action in the case that the attribute was >40 or less 1, not obvious if your dry running the code in ones head.. It is funny though that your example GiGs is less efficient than my original code (this is something that Scott may care about more than I). As my original  if/else condition is surely faster and uses less memory than your if/else + creating an array and assigning values, but I only mention this for fun as its miniscule : ) -- but then again i do have a couple of string literals that you do not and as we know in the code profiling world strings are expensive... Thanks again guys, loved the conversation, the camaraderie and what a lovely community !

Yes, it will be faster in this instance but as you say the amount is negligible. (You'd probably need to run dozens if not hundreds of iterations of that code to be able to measure a meaningful performance difference.)  The benefit you get from having the habit of doing it this way, so it becomes second-nature, pays off when you have a worker that you'd otherwise end up with multiple separate setAttrs in it. The thing is, with roll20, you have client-based code (almost everything), and server-based code (setAttrs, getAtts, etc.). Your client-based code doesn't need to be remotely efficient - any pc anyone is using with roll20 will handle whatever client-based code in a character sheet you throw at it.  (Obviously, you could go out of your way to prove this statement wrong, but in any functioning character sheet, the client-based code is very very unlikely to be a source of lag). The server-based code though does need to be handled with consideration, because if you're not careful, you can end up with very laggy sheets indeed. So while I earlier argued against code complexity, this isn't a contradiction: you can use techniques like this without increasing code complexity by very much, while at the same time stopping unnecessary inefficiencies creeping into your code.

Yeah I agree,  I designed and wrote enterprise server based software for several decades as an enterprise architect. Designed, wrote and lead teams to create many systems for fortune 500 companies as a principal consultant in e business development, lead multiple engineering departments of a fortune 5000 multi multi million dollar software house as exec director of engineering. Been in software engineering field (not that it was called that back then) since 1986. Hold many certifications including, an enterprise java architect certification, Oracle Master certified DBA. Worked in software engineering field in three different continents. So I understand the server side a little. Particularly AWS used by Roll20 : )

Hehe, that's good to know - you know way more about it than I do :) Roll20 forums are populated from extreme beginners to experienced pros, and I've always found it best to err towards the beginner side - if I'm saying something a Pro already knows, then there's no harm done, but if I'm saying something a beginner doesn't, it can be very helpful.

No, in this domain Roll20, Sheets and the API, you are the pro, i'm playing catchup. Also by the time CSS was invented I didn't code for a living anymore I was leading teams or departments, so i'm no pro at javascript or css. So your advice, eyes and thoughts have been very helpful ! Thank you again!