5
\$\begingroup\$

I have a script that alphabetizes the layers in Adobe Illustrator. It does so by sorting the layers in an array (using string names) and then using that to change the item order position (zOrder) of the layers. The script runs fine, however if there is a very large layer (which I deal with a lot) of over 8000+ it gets very sluggish and slow (over 10 minutes to sort). I would like a way to optimize the code to run faster, possibly a faster sort or search algorithm (it searches the array at one point which is the main source of the problem).

#target illustrator

/**
* sort all layers (and sublayers)
*/
if (app.documents.length > 0) {
    var doc = app.activeDocument;
    var docLayers = doc.layers;
    var allLayers = [];
    var currentLayers = [];

    start();

    function start() {
        try {
            recurseLayers ( docLayers  );

            var result = currentLayers.sort( function(a,b) { return a > b } );
            sortLayer(allLayers, result);
            alert("Done sorting!");
        } catch (error){
            logger(error);
        }
    }

    function addLayer(currentLayer) {
        var layerName;

        if (currentLayer.typename == 'TextFrame') {
            layerName = currentLayer.contents;
        } else {
            layerName = currentLayer.name;
        }

        currentLayers.push(layerName);
        allLayers.push(currentLayer);

        return currentLayers;
    }

    function sortLayer (obj, array) {
        try {
            var length = array.length;

            for (var i=length; i--;) {
                var name = array[i];
                var item = search(obj, name);

                if (item != -1) {
                    item.zOrder( ZOrderMethod.BRINGTOFRONT);
                }
            }
        } catch(e) {
            logger(e);
        }
    }


    // Recursive loop to search all layers in active document
    function recurseLayers ( layers ) {
        var length = layers.length;
        var currentLayer ;
        var result;
        var locked, visible;

        try {
            for (var i = length; i--;) {
                currentLayer = layers[i];

                locked = currentLayer.locked;
                visible = currentLayer.visible;

                if (visible == null || locked == null) {
                    visible = checkLayerVisibility(currentLayer);
                }

                if (visible == true && locked == false) {

                    // sort layers
                    addLayer(currentLayer);

                    // Search for sublayers, page items or group items
                    if (currentLayer.layers) {
                        recurseLayers(currentLayer.layers);
                        recurseLayers(currentLayer.groupItems);
                        recurseLayers(currentLayer.pathItems);
                        recurseLayers(currentLayer.compoundPathItems);
                        recurseLayers(currentLayer.symbolItems);
                        recurseLayers(currentLayer.textFrames);
                    }
                }
            }
        } catch (error) {
            logger (error);
        }
    }// end recurseLayers

    //Very slow with lots of layers
    function search(arr, obj) {
        var len = arr.length;

        for (var i = len; i--;) {
            var item;

            if (arr[i].typename == 'TextFrame') {
                item = arr[i].contents;
            } else {
                item = arr[i].name;
            }

            if (item === obj) {
                var found = arr[i];
                return found;
            }
        }

        return -1;
    }


    function checkLayerVisibility(layer) {
        var visible = layer.visible;
        if (layer.typename != 'Layer') {
            if(visible || visible == null) {
                for(var parent = layer.parent; parent.typename=='Layer'; parent = parent.parent) {
                    var pvis = parent.visible;
                     if(!pvis) {
                        visible = false;
                        return visible;
                    } else {
                        visible = true;
                    }
                 }
             }
            return visible;
        } else {
            var parent = layer.parent;
            visible = layer.visible;
            if (visible != false) {
                if (parent.typename == "Layer") {
                    for(var parent = layer.parent; parent.typename=='Layer'; parent = parent.parent) {
                        var pvis = parent.visible;
                        if(!pvis) {
                            visible = false;
                            return visible;
                        } else {
                            visible = true;
                        }
                     }
                }
            } else {
                return layer.visible;
            }
        }
     }


    // Prints stack trace
    function logger(e) {
        var errorMsg = "";

        errorMsg = errorMsg.concat("An error has occurred:\n", e.line, "\n", e.message, "\n", e.stack);
        $.writeln(errorMsg);
    }
}

The main problem is the 'search' function. I need to search the array and can't use indexOf (which is much slower anyways).

I'm primarily a java programmer and could figure this out in java, i'm fairly new to javascript so am unsure how to go about optimizing my search/sort functions. Also most questions and answers are in the browser with javascript, any help is appreciated.

\$\endgroup\$
  • \$\begingroup\$ You scan every object in a layer every time, and discard the results after one use. Consider scanning a layer once and storing a map from object name to object itself: obj_by_name[obj.name] = obj;. Then finding an object by name should become very fast. Store results for all layers, too. It will take some RAM, but it must be peanuts compared to what Illustrator uses anyway. \$\endgroup\$ – 9000 Jan 20 '17 at 21:12
  • \$\begingroup\$ @MikeMcCaughan I didn't know there was that. I'll check it out (and yes it meets all of those) \$\endgroup\$ – Lloyd Smith Jan 25 '17 at 13:53
  • \$\begingroup\$ @9000 I think I understand what you mean, but am unsure how to implement in javascript. I tried doing something similar to that, but it said 'no element exists' or something like that. I currently loop through the layers and copy the layers themselves, and their string name to two different arrays, which I use for sorting. I need to be able to search the 2nd layer (actual layers) for the current string name (1st layer). Can you give me a short example? \$\endgroup\$ – Lloyd Smith Jan 25 '17 at 14:36
4
\$\begingroup\$

What you are finding here is that one data structure does not fit all use cases. While it certainly makes sense to utilize an array to perform the sort (since you end up having to compare all the values anyway), an array certainly does not make sense for performing a lookup against any given layer name.

I would suggest that rather than using an array for your allLayers store of layer objects, that you build a key-value store - a hashmap - to allow for O(1) lookup of any given layer. In javascript this would be modeled using an object that might look like this:

{
    'alayer': {// layer object},
    'blayer': {// layer object},
    ...
    'zzz': {// layer object}
}

This object would not represent order of layers, as technically object properties have no order, so you would still need to maintain your currentLayers array to maintain ordering.

For convenience (and to prevent O(n) seeks against the ordered array), you might also want to store the value of the current order of the object in the array on the each layer object in the key-value store

For example:

{
    'alayer': {
        sortIndex: 0,
        ...
    },
    'blayer': {
        sortIndex: 1,
        ...
    },
    ...
}

To present this in terms of operational complexity (Big O). Your current methodlogy does this (here n would represent number of layers)

recursion and building of data structures - O(n)
layer name sort - O(n log n) average, O(n^2) worst case
applying the sort - O(n) for iteration, O(n) for lookup, thus O(n^2) worst case

Overall worst case = O((2 * n^2) + n)

With key-value store for layer objects:

recursion and building of data structures - O(n)
layer name sort - O(n log n) average, O(n^2) worst case
applying the sort - O(n)

Overall worst case = O(n^2 + 2n)

I would recommend getting into the habit of using exact comparisons (===, !==) instead of loose comparisons (==, !=) as your default means of comparison. This can help prevent against unexpected comparison behavior. I would go so far as to say that loose comparisons should ONLY be used in cases which truly warrant it and should ideally be accompanied by a comment explaining why the loose comparison is appropriate for that case.


var doc = app.activeDocument;
var docLayers = doc.layers;

Not sure that you are getting any value for storing duplicate copies of this information in this scope. You are only using app.activeDocument.layers a single time, in your start() function. Why not have first line of that function simply be recurseLayers(app.activeDocument.layers)?


var currentLayers = [];

This is a really bad variable name. As I was reading through this code I was thinking that this array stored layer objects, when in fact, it stores only the layer names. I have no idea what current portion of the variable name would be referring to either. Be specific and meaningful in your variable, function, etc. naming. Here layerNames would probably be better.


var result = currentLayers.sort( function(a,b) { return a > b } );

sort() sorts an array in place, so no need to assign to new variable. No need to define a function here as you are just using default sort behavior. This line should probably just be:

currentLayers.sort();

function addLayer(currentLayer) {
    var layerName = currentLayer.name;
    if (currentLayer.typename == 'TextFrame') {
            layerName = currentLayer.contents;
    }

    currentLayers.push(layerName);
    allLayers.push(currentLayer);

    return currentLayers;
}

Why return the array of names from this function?

If using key-value store this function might look like:

function addLayer(currentLayer) {
    var layerName = currentLayer.name;
    if (currentLayer.typename == 'TextFrame') {
            layerName = currentLayer.contents;
    currentLayers.push(layerName);
    // assumes allLayers is name of key-value store
    allLayers[layerName] = currentLayer;
}

Note that this eliminates need for a search() function.


function sortLayer (obj, array) {

Bad function name. There is no sorting happening. Here you are actually applying the previously determined sort to the layers. Perhaps applySortToLayers or similar would be better name. Why do you need to pass any parameters to this method at all? You already have necessary data available to this function scope.

If using a key-value store for your layer objects, this function might look like:

function applySortToLayers() {
    currentLayers.forEach(function(name, i) {
        // assumes allLayers is key-value store for layers
        // also assumes that there is SENDTOBACK opposite of BRINGTOFRONT
        // if not, you would need to iterate layerNames in reverse as
        // in your current code
        allLayers[name].zOrder( ZOrderMethod.SENDTOBACK);
        // if you want to add reference to sort order
        allLayers[name].sortIndex = i;
    });
}

// Recursive loop to search all layers in active document
function recurseLayers ( layers ) {

This function seems poorly named. Yes it is recursive, but what is really happening here is you are flattening a hierarchical structure. Perhaps a name like flattenLayersRecursive would be more fitting. Also, the comment here is misleading. You are not doing any sort of "search" at all.


var result;

Not used. Get rid of this.


You seem to only be adding visible / unlocked layers to your data structure. Will this cause you problems? If one of these settings is later changed for any single layer, you would then have to recalculate all of these data structures. Is this desirable?


                // sort layers
                addLayer(currentLayer);

This comment confuses me. There does not seem to be any sorting here.


                // Search for sublayers, page items or group items
                if (currentLayer.layers) {
                    recurseLayers(currentLayer.layers);
                    recurseLayers(currentLayer.groupItems);
                    recurseLayers(currentLayer.pathItems);
                    recurseLayers(currentLayer.compoundPathItems);
                    recurseLayers(currentLayer.symbolItems);
                    recurseLayers(currentLayer.textFrames);
                }

You only check for presence of layers here to recurse down into. Should you be checking for presence of other items as well?

Perhaps something like:

var recursionTargets = ['layers', 'groupItems', ...];
recursionTargets.forEach( function(prop) {
    if (prop in currentLayer) {
        recurseLayers(currentLayer[prop]);
    }
});   

The code in checkLayerVisibility() seems unnecessarily verbose, redundant and ripe for recursion for checking parent attributes.

Perhaps something like this (assuming that true can only be returned if both current layer and all parents are visible):

function checkLayerVisibility(currentLayer) {
    if (currentLayer.visible === false) { return false; }
    if (currentLayer.parent !== 'undefined') {
        return checkLayerVisibility(currentLayer.parent);
    }
    if (currentLayer.visible === true) { return true; }
    return false;
}

I guess I also don't understand exactly how this is intended to work with regards to earlier comments around visibility in your main tree-flattening recursion. Assuming that you are starting with the root nodes in parent-child layer trees, why would you ever need to check visibility of parents here? I would think that with your current logic, your main recursion would never even hit a a child of a parent layer that is not visible.

Also note that I am getting out of your pattern of unnecessary variable assignments like this:

                        visible = false;
                        return visible;

When you could be more direct and just do return false.


I don't understand usage of try-catch and logger here. This is great in principle to have good error handling, however you have not defined any errors to be thrown, so this is just superfluous at this point.

\$\endgroup\$
  • 1
    \$\begingroup\$ Comments are not for extended discussion; this conversation has been moved to chat. \$\endgroup\$ – Jamal Jan 25 '17 at 20:42
0
\$\begingroup\$

You're searching through everything but (at least from this code) it looks like you're only interested in searching when currentLayer.typename == 'TextFrame'. It means you can have a different array where you only store those and search on that one instead. I'm not familiar with Adobe Illustrator, but currentLayer.textFrames sounds like a ready-made list of text frames in which you could do your search.

If this is still not fast enough, you could cheat a bit and decide that you don't need the element in the list after you've found it, so you could remove it, making the linear search faster next time. You can do that using something like:

function searchAndRemove(arr, obj) {
    var len = arr.length;

    for (var i = len; i--;) {
        var item = arr[i].name;
        if (arr[i].typename == 'TextFrame') {
                item = arr[i].contents;
        }
        if (item === obj) {
            var found = arr[i];
            arr.splice(i, 1);
            return found;
        }
    }

    return -1;
}

Honestly though, this sounds like something that you may want to do server-side.

\$\endgroup\$
  • \$\begingroup\$ Not quite. I updated the code to hopefully be more clear. I need to search for everything, I am only using the if for text frame because otherwise it would get nothing (sometimes the .name on textframes returns ""). I'm more interested in layers and objects (pageitems, etc). Also what does 'arr.splice[]' do? (no possible way for me to do this server side as there is none and this script is ran directly in AI). \$\endgroup\$ – Lloyd Smith Jan 25 '17 at 16:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.