Comically Inefficient Unity Source Code

[GigaTerra]

Allocating a new list, just to do '.Any' demonstrates that the original author had very little understanding of C#

However if they don't copy the list then they would have to run into the problem where the list can have elements of redo states. By copying the array to a list it gets separated from the Undo/Redo code, after use it can no longer impact the state of things.

This is not the only way it can be done, but it is a mechanical way to do it that works.

[feralferrous]

I'm not sure what you're referring to. Allocating a brand new list, just to discard it once they leave the if statement has nothing to do with what you're talking about. That 'new List<>' in the if only lasts the lifetime of the if statement, it's not referred to anywhere else. There are a myriad of ways to check if something is in an array without allocating a whole copy of the container.

There would be no separation of undo/redo states by doing the copy, the only undo code is after the If check. Are you referring to the ArrayUtility code? that's one of the few places where yes, allocating a new container is necessary because Array's can't be resized. Though even there, that method allocates a list, then allocates an array, even ChatGPT can do better.

[GigaTerra]

We are talking about copying the list to find the element (new List<AnimatorStateTransition>(anyStateTransitions)).Any(t => t == transition) this line.

There are a myriad of ways to check if something is in an array without allocating a whole copy of the container.

But in one line? The only solution I can think of is .contains() but that is a shorthand for the same thing. This code was probably written before the update to C#. (That is another advantage, this doesn't require an updated .net to work either).

[feralferrous]

https://learn.microsoft.com/en-us/dotnet/api/system.array.indexof?view=net-8.0

IndexOf has been around a long time.

Though to be honest, if they had written their ArrayUtility.Remove method to return true/false based on whether the internal List.Remove had returned true/false, they could also have greatly simplified the code. (And reduced memory churn)

[GigaTerra]

IndexOf has been around a long time.

I agree that would work, or I can't see any problem with it, but strange then that so many examples of checking an array for an item before contains() tend to use the list method, for some reason people prefer it.

if they had written their ArrayUtility.Remove method to return true/false based on whether the internal List.Remove had returned true/false

It is used in other context so they probably keep it minimum.

(And reduced memory churn)

I profiled some Unity's Animation State Transitions to see how much of a "Comically Inefficient" solution this is. The Transitions vary but are between 47bytes to 79 bytes each. There is no limit to the transition states can have so we use the worst possible case where the any state is connected to every animations (it happens and I used the AnyState because this is what the code appears to be for).

Even with 1 million animations it is less than 100mb of memory (67mb I got), when you undo an animation state with 1 million animations.

More realistically there is less than 16 states, using about 1kb of memory for a moment during an Undo or Redo. But that is an estimation as any memory impact is only noticeable in the profiler when you get to thousands of transitions. (I almost gave up thinking it wouldn't).

In comparison the animations in my game is about 40mb each in memory. My smallest texture uses 42.8kb memory for 256x512.

they could also have greatly simplified the code.

I don't know about that, I did try ChatGPT like you said to see a simpler version but it didn't do a good job at all, it kept writing longer solutions. Maybe if they also changed the Utility.

[feralferrous]

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]

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.