[Script] Export/Import Attribute Data

Roll20 Production Team

API Scripter

Nice job getting this together. =D

Here's a few things to consider:

findObjs() is an expensive call. It iterates across every object in the campaign, including those that are archived. If you are going to be using it to find a bunch of different attributes (for example), you are far better off to just get all of the attributes with a single call, then pull things out of that collection with something like _.findWhere(). Also, if you know the type and id, you can get an object directly and efficiently with getObj():

var obj = findObjs({ _id: objInfo._id, _type: 'graphic', _subtype: 'token' })[0];
var obj = getObj('graphic', objInfo._id);

All scripts share the same global scope. Your immediate function at the beginning which is replacing the createObj() function with one that corrects the firebase issue is replacing it globally for all scripts. While that might be ok, you are better off doing that in a limited scope so that you don't make assumptions that might break other scripts (not that this probably would, but it's a general thing).

You might want to return after checking for selected objects, as otherwise this will print a message and continue to process the api command:

if (msg.selected == undefined){
	sendChat("API"," Please select something to modify.");
	return;  // done processing this message, skip the rest of the function
};

You probably want to transpose these two lines, as you are referencing an object that might not exist before checking if it exists:

var represents = obj.get("represents") // obj may be undefined!!           
if(obj) {

This:

            		Attribute = n[a].substring(0, n[a].indexOf("|"));
			ValueString = n[a].substring(n[a].indexOf("|") + 1);       
			if(ValueString.indexOf("|") === -1) {
				Value = n[a].substring(n[a].indexOf("|") + 1);
                        	Max = '';
			} else {
				Value = ValueString.substring(0,ValueString.indexOf("|"));
				Max = ValueString.substring(ValueString.indexOf("|") + 1);
			};

can be rewritten more idiomatically as:

var attrs = n[a].split('|');
Attribute = attrs[0] || 'undefined';
ValueString = attrs[1] || 'undefined';
Max = attrs[2] || '';

The || ( logical or operator) will return the first true value. In the case where any of the indexes of attrs are undefined, it will set the variable to the right hand side argument of ||.

Anytime you can avoid findObjs() is an efficiency win. You'd be better off building a collection of all the attributes first, then finding the right one when you need it. This code will be run for <number of selected> x <number of arguments>:

existingattribute = findObjs({_type: "attribute", name: Attribute, _characterid: represents})[0];

Some characters have hundreds of attributes, especially if they are setup with a character sheet. I rewrote a part of a script once that did a few innocuous findObjs() calls for some character attributes, but was crashing after a minute because it turned out to add up to several thousand calls. Something like this will build lookups for all attributes by character id and attribute name, and only requires one findObjs() call:

		var AttrByName = {},
			AttrByChar = {};
 
		_.map(findObjs({
			type: 'attribute'
		}),function(a){
			if(!_.has(AttrByName,a.get('name'))){
				AttrByName[a.get('name')]=[a];
			} else {
				AttrByName[a.get('name')].push(a);
			}
			if(!_.has(AttrByChar,a.get('characterid'))){
				AttrByChar[a.get('characterid')]=[a];
			} else {
				AttrByChar[a.get('characterid')].push(a);
			}
		});

Rather than do procedural iteration with a while loop:

        var a = 1;
    	while (n[a]) {

Consider using the functional style:

_.each(n,function(na){

I think it's cleaner reading, and you don't need to sprinkle array dereferences all over the code.

Consider putting all your commands in a single on('chat:message',...) call. You don't have to repeat all your argument parsing logic that way, and it seems to me that it would be more efficient for the dispatch logic to have fewer registered functions to call.

Hope that's constructive! All I have time to write for now.

August 29 (10 years ago)

Roll20 Production Team

API Scripter

One other thought. You might consider putting this into a Gist. Gists when displayed on the forum have line numbers and code highlighting, which can be nice for discussion. I always put the Gist URL at the top of all my scripts so that users can look there directly for updated versions instead of needing to look on the forum. The one caveat to Gists is that the forum caches anything with the same URL, so if you update your Gist, you have to change the url. All my scripts have a version number, so I just append that to the URL with anchor notation:

http://somegist/url/scriptid#v1
http://somegist/url/scriptid#v2
... etc..

August 29 (10 years ago)

Very constructive, Aaron. Thank you. It's a lot to take in, but I'll tackle it soon. :)

August 29 (10 years ago)

And BTW, now that you can somewhat see what I'm trying to do, still the same opinion on that creation step?

Maybe I should unhinge it from the selected creature somehow?

August 29 (10 years ago)

Roll20 Production Team

API Scripter

Yes and no. You still can't affect the association from character -> token. Tokens have the represents property which is token -> character, which you can set.

That said, you could make some minor changes to your script so that you create a new character with your !setme command. If your !getme command has a token selected (which is really the easiest way to target a character), you could even have your !setme command construct an appropriate token with the same stats as the original token. Then the only thing left to do would be manually go in and click "Use Selected" for the token on the character (which would be pretty quick: shift-double click created token, edit, use selected, save).

August 31 (10 years ago)

Edited August 31 (10 years ago)

Okay, so using this:

		var AttrByName = {},
			AttrByChar = {};
 
		_.map(findObjs({
			type: 'attribute'
		}),function(a){
			if(!_.has(AttrByName,a.get('name'))){
				AttrByName[a.get('name')]=[a];
			} else {
				AttrByName[a.get('name')].push(a);
			}
			if(!_.has(AttrByChar,a.get('characterid'))){
				AttrByChar[a.get('characterid')]=[a];
			} else {
				AttrByChar[a.get('characterid')].push(a);
			}
		});

How do I get it back out?

I've tried .get, _find, _where, etc, and I feel pretty stupid... :)

EDIT - Found one that works

var allattributes = AttrByChar[represents]

September 01 (10 years ago)

Edited September 01 (10 years ago)

Okay, next version, all gist-ified.

!getall now fetches attributes and abilities. !setattr and !setabi are the import for those. I've also switched out to !copyme 'token' which saves a click or two.

Edit: I didn't test with powercards. Oops. Another revision...

https://gist.github.com/mcbobbo/84541c813d819a083651/2a5663f5afcaa407dfb0424223cbe299953bc111#file-createsetget-js

September 01 (10 years ago)

Yeah, so I clearly can't get the gist thing to work... /shrug

September 01 (10 years ago)

Roll20 Production Team

API Scripter

I'm certainly seeing the gist.. (twice.. not sure why...)

September 01 (10 years ago)

Roll20 Production Team

API Scripter

Do you mean that your newer version isn't showing up in the post? The Forum caches the contents of urls it embeds. You can get around that by editing the post, clicking on the url to the GIST, and adding something unique in an anchor to the end. I use the version number of the script (because I have version numbers), but you can use about anything" http://some/url/that/gets/cached#v2

September 01 (10 years ago)

Roll20 Production Team

API Scripter