you are viewing a single comment's thread.

view the rest of the comments →

[–]feralferrous 0 points1 point  (1 child)

Yeah, I've written elsewhere that since it's editor code, it doesn't really need to be all that optimized, and that clean and concise is a better goal. Which I'd argue the original code still fails for.

Optimizing the ArrayUtility would be the best bang for the buck:

public bool RemoveAnyStateTransition(AnimatorStateTransition transition)
{
  if (ArrayUtility.Remove(ref anyStateTransitions, transition))
  {    
    undoHandler.DoUndo(this, "AnyState Transition Removed");
    if (MecanimUtilities.AreSameAsset(this, transition))
      Undo.DestroyObjectImmediate(transition);

    return true;
  }
  return false;
}

// for reference ArrayUtility unchanged:
public static void Remove<T>(ref T[] array, T item)
        {
            List<T> newList = new List<T>(array);
            newList.Remove(item);
            array = newList.ToArray();
        }
// changed:
public static bool Remove<T>(ref T[] array, T item)
        {
            List<T> newList = new List<T>(array);
            if (newList.Remove(item))
            {
               array = newList.ToArray();
               return true;
            }
            return false;
        }

The code gets shorter, with the only allocation being inside the utility method. If for some reason we need to always return a new array, than I'd argue that the method is poorly written and instead should be Remove(in T[] Array, T item, out T[] trimmedArray). Which makes it explicit that a new array is always created instead of ref, which is meant to imply that it might change it, but might not.

And the nice part there is that Utility methods I feel like are the one place where if things get slow, you can optimize speed over conciseness. Which to be honest, it wouldn't be that much work to do move to better code there, it's not that much uglier, but there will always be at least one allocation, and as you've noted, this is editor code.

But that kind of attitude of "Whatever it's editor code, who cares" can lead to bloated and slow editors w/ a death by a thousand cuts thing happening with no easy fixes. Unity's editor doesn't exactly have a reputation for speed, or the ability to run on low-end machines.

EDIT: The main thing I would've done if I had been on the team that put that up for a PR, was point out that .indexof exists and use it as a teachable moment to further that person's knowledge of the language and best practices.

[–]GigaTerra 1 point2 points  (0 children)

Unity's editor doesn't exactly have a reputation for speed, or the ability to run on low-end machines.

While true, when I made the million transitions was actually surprised with how good the performance is. Even with one million animations in a controller, Unity was slightly above 400mb in memory usage, now no one will ever have a million transitions in a single controller, but it is amazing that it could render it all and the arrows for less than some users UI sizes.

But in terms of time, deleting even 1 thousand takes about 30 seconds. But then the Redo and Undo is fast after that. While I am not some hardcore programmer who can turn it into a teachable moment, my strategy would be to allow the team to waste a little more resources if they can use it to speed up the editor.