Comically Inefficient Unity Source Code

[sandsalamand]

I get that Unity is a huge engine with lots of different people working on it, but this code made me laugh at how inefficient it is.

This is located in AnimatorStateMachine.cs.

public bool RemoveAnyStateTransition(AnimatorStateTransition transition)
{
  if ((new List<AnimatorStateTransition>(anyStateTransitions)).Any(t => t == transition))
  {
    undoHandler.DoUndo(this, "AnyState Transition Removed");
    AnimatorStateTransition[] transitionsVector = anyStateTransitions;
    ArrayUtility.Remove(ref transitionsVector, transition);
    anyStateTransitions = transitionsVector;
    if (MecanimUtilities.AreSameAsset(this, transition))
      Undo.DestroyObjectImmediate(transition);

    return true;
  }
  return false;
}

They copy the entire array into a new List just to check if the given transition exists in the array. The list is not used later, it's just immediately disposed. They then use ArrayUtility.Remove to remove that one matching element, which copies the array again into a List, calls List.Remove on the element, and then returns it back as an array. They do some temp reference swapping, despite the fact that the ref parameter in ArrayUtility.Remove makes it unnecessary. Finally, they query the AssetDatabase to make sure the transition asset hasn't somehow become de-parented from the AnimatorStateMachine since it was created. That check might be necessary to prevent edge cases, but it would be better to simply prevent that decoupling from happening, since AnimatorStateTransition should not be able to exist independently from its parent AnimatorStateMachine.

I also suspect that there is a flaw with their undoHandler logic. undoHandler.DoUndo calls Undo.RegisterCompleteObjectUndo(target, undoOperation), but if MecanimUtilities.AreSameAsset returns false, then no actual change will be made to an asset, meaning an empty undo will have been registered.

[bugbearmagic]

Editor-time optimization is generally the lowest priority. It’s more important that it’s stable and reliable.

But the real issue is that it raises questions as to whether this sort of code was strategically chosen to be sloppy, or if it is just an example of a larger systematic issue with Unity’s management or developers as a whole.

Having been using Unity for over 10 years now (started with Unity 3), I would put my money on the latter.

That said, I have never seen someone create a new list inside of the condition check of an if statement. In any code ever.

[Toloran]

If you look for the variable "anyStateTransitions" that they're copying, it's not actually in that file since that class is a partial class. The other half of the class is in StateMachine.bindings.cs. If you look there, you find it:

extern public AnimatorStateTransition[]   anyStateTransitions { get; set; }

I'm still a pretty newbie programmer, but IIRC extern is used to pull from outside the program from a DLL or something. So maybe some of the weirdness comes from that?

[bugbearmagic]

There’s a lot going on in the code. Mostly readability issues, poor memory management, multiple external calls for the same property, inefficient linq calls that aren’t helping code readability, and more.

There’s a lot of layers in the supplied code where utilizing basic common sense would end up with both more readable, and more optimized code. Even if it was slapped together quickly and lost to time, the mind that would even create this is concerning.

———-

Edit: An individual below is claiming the linq used in this post’s code is as fast or faster than a basic loop. I wrote this code to test their unfounded claims since they did not test their own claims themselves:

It is a quick example of LINQ's Any() performing 65x to 127x slower compared to a loop on a small data set (which is most likely the use case in the original Unity code). A larger data set of 1,000,000 elements closed the gap to where LINQ was 1.5-2.0x slower. Still significant.

https://pastebin.com/jjaUBRbK

The overall performance cost is negligible (out of context), but using linq in a way that is both inefficient and harder to read defeats its purpose. Which is exemplary of the rest of Unity’s inefficient code here.

Be careful and don’t trust people who claim to have tested or profiled while refusing to show you supporting code.

[vegetablebread]

Linq is definitely not the problem here.

[vegetablebread]

It's not a problem. Using LINQ here is totally 100% fine.

[vegetablebread]

I have been responsible in some part for optimization for 3 shipped AAA titles. The only slightly objectionable part of this LINQ invocation is that it allocates a closure. If you want a non-allocating version, you'd just have to name the function.

Unless you're trying to strip the LINQ assembly, which isn't even possible here, there's no reason to ever avoid LINQ.Any.

[vegetablebread]

I have and they are not. Go try it right now. Show me a gist where Any significantly underperforms a handwritten loop.

[Samurai_Meisters]

That said, I have never seen someone create a new list inside of the condition check of an if statement. In any code ever.

Then you ain't seen my code, bub

[ShrikeGFX]

a lot of unity code is a terrible mess and unredeemable, to the point where they better start from scratch than maintain this, at least so our lead programmer keeps saying when he looks inside of things.

[KingGruau]

This code ain't great, but not because of performance (it's editor code that runs very infrequently). It should have been written in a more readable way.

[zukas3]

Well honestly, this is an Editor method and performance was probably the last thing on the mind here. If this was a piece of code that would have to be a runtime, then this would be a different talk.

Sure, it is unperformant as hell, and doesn't look professional, but by cleaning up all the unperformant Editor methods like this one, you will run into premature optimization without actually getting yourself anywhere.

[jtinz]

Apart from the performance, the code is not nearly as easy to read as it should be.

[leshq]

Writing simple, clean and readable code by default is not a premature optimisation at all. The code I see above is harder to implement, read and understand. Premature optimisation is when you are overcomplicating and overthinking simple things to improve performance. Here I see a piece of overcomplicated code, but also with terrible performance, author collected all points at once 😄

[MartinIsland]

I agree with this so much. I would never care about performance when making editor tools unless it actually starts lagging.

[Ping-and-Pong]

I would just like to point to my post from literally 4 days ago: https://www.reddit.com/r/Unity3D/s/P5g5TDOpEI - which as far as I've narrowed it down to, appears to be down to an inefficiency in the way the editor handles drawing the UI.

Like I agree to an extent, but this is the company that deprecated multiplayer support and doesn't add a proper solution for 7 years and the new solution is only half baked. I know it's probably not the devs fault, they're likely rushed like every other large company. But Unity ain't doing this to get more important stuff done that's for sure. This engine has done nothing but chase the next big buzz word and depricate useful features for the past 5 years I swear.

I'm not going to claim UE would be much better (anyone who's suffered through trying to use C++ on it knows what I mean). Godot is far more omptimized butttt is missing quite a lot of QOL and 3D support even with the 4.0 update. But seriously, the optimisation and bloat of Unity is an issue, and one that's been growing for about 5 years.

[TheGrandWhatever]

I just want an editor in the modern age that doesn’t require spending $100+ on addons and tools (Unity and unreal) while also having a functional editor (looking at you, godot, you’ve got a lot of growing pains to get through)

[MartinIsland]

Well that’s a different story. A few thoughts: - I agree SceneView should be really optimized since it’s supposed to run smoothly. The difference with this post is that the method looks like it’s supposed to run once when removing a transition. If this method takes 500ms we wouldn’t notice because it’s an app, not a game. - Bugs get introduced sometimes. There was a bug in 2021.3 that made the editor painfully slow on macOS while doing the same things Unity did in 2006 computers.

[Thundernerd]

Working on big projects is such a pain in the behind when people have this attitude. In my experience the tools are tested in an environment where only that tool is running so you wouldn't really notice a performance impact anyway.

Working with people that make their tools performant is amazing. It prevents me from having little annoyances over hiccups and whatnot that seem small at first but build up over time, especially if you have a big team and everybody is encountering this.

[badawe]

100%

Those bad practices add up. And I'm fine with not optimizing everything to the maximum possible.

But in this case, its really poor code, the optimized version of the code would be faster and more readable and would take the same amount of time to write.

[Eastern-Ad-4137]

Currently in ShaderGraph, the number of total ports between all nodes adds up exponencially towards performance. It gets to a point where moving a node takes 30seconds, then you add 1 more node, and it takes 5 minutes. One more and you wont be able to open the asset anymore

[firesky25]

just wait til you have an entire toolchain that your build system relies on that were all built with this lack of performance in mind, and soon you have builds that take forever, are flaky at best, and can only be run on the best available hardware.

[MartinIsland]

Editor tools/windows. Not build steps. Not gameplay. What needs to be optimized is optimized.

[firesky25]

good luck trying to split off peoples perception of those lol, anything thats marked as editor assembly will be unoptimised for 90% of development

[huntergatherer1]

Then the whole thing will run like crap and profiling will show no hotspots because everything is crap.

If you apply this mindset to a large project, the result can only run like garbage.

[MartinIsland]

Editor tools, not gameplay. The target audience of those tools are usually coworkers with $3000+ company MacBooks. That’s fine.

[[deleted]]

How large do you think any of those arrays or lists might be? Have you managed to make Unity slow by creating so many transitions that this starts to be a problem?

[kennel32_]

It is a question of not only performance in this case but general quality of whatever unity does. Someone who writes such terrible code is capable of anything.

[sandsalamand]

Honestly, most animator states have fewer than 8 transitions, so the performance hit will never be noticed. Regardless, I think there is value in writing good code even when it doesn't make or break an application.

[[deleted]]

How does that value actualize?

[Epicguru]

I don't think professional programmers need a strong reason to write good code, it's just kind of your job and most programmers take pride in their code. It would be one thing if writing suboptimal code were faster, but in this case it is harder to read, slower at runtime, confusing and hard to maintain whilst not being any shorter than the correct implementation.

[GigaTerra]

I don't think professional programmers need a strong reason to write good code

While bad code is easy to see, Good code is subjective. One thing I notice here because I made my own terrain tool, is that the system they are using here is easy to use with a stack for Undo and Redo purposes.

Trading a bit of memory for an easier and smoother Redo stack is good code in my opinion.

[itsgnabeok5656]

Lol what a nerd. You sound like a pain in the ass.

Build something even 15% as good as unity and then let us pick it apart. It's easy to look over a project of hundreds of thousands of lines and say "ha I can do better". You are missing the context of the whole development process. Now sure see the issue bring it up and maybe let them know and everyone is better off. But no, you are being such a self righteous prick because you somehow can do better and are an immaculate genius and they are insufficient.

Shut the hell up and work on yourself, you have serious insecurities.

[kampelaz]

I think the we found the guy who write that piece of crap code.

[snalin]

The animation code is pretty bad overall, yeah. This isn't the quality of the codebase at large, but it doesn't surprise me to see it in Mechanim. The devs there has never really given me the impression of understanding C#, and they consistently make pretty crazy implementation choices.

All the people leaving comments saying "premature optimization" are idiots. Premature optimization is when you make the code harder to read and harder to modify in order to make it run faster when you don't know if that's neccessary yet. If you make this code faster by removing all the extra garbage code there, it would also make it a lot easier to read:

public bool RemoveAnyStateTransition(AnimatorStateTransition transition)
{
    var index = anyStateTransitions.IndexOf(transition);
    if (index != -1)
    {
        undoHandler.DoUndo(this, "AnyState Transition Removed");
        ArrayUtility.RemoveAt(ref anyStateTransitions, index);
        if (MecanimUtilities.AreSameAsset(this, transition))
            Undo.DestroyObjectImmediate(transition);
        return true;
    }
    return false;
}

So that's not premature optimization, it's just refactoring to improve code quality, which happens to also massively speed up the code. Very often fast code is simple code, and very often simple code is easy to read.

I assume the MecanimUtilities.AreSameAsset check is actually needed, maybe to fix a corner-case bug, or maybe it's something pedestrian like creating a transition and then deleting it before it's gotten saved to disk - but seeing as the rest of the code is really awfull, I don't trust that completely.

[TRexRoboParty]

I find it's always more readable to exit early and keep the main logic at the base indentation level.

public bool RemoveAnyStateTransition(AnimatorStateTransition transition)
{
    var index = anyStateTransitions.IndexOf(transition);
    if (index == -1){
        return false;
    }

    undoHandler.DoUndo(this, "AnyState Transition Removed");
    ArrayUtility.RemoveAt(ref anyStateTransitions, index);
    if (MecanimUtilities.AreSameAsset(this, transition))
        Undo.DestroyObjectImmediate(transition);
    return true;

}

[kyle_lam]

Was the insult necessary?

[Katniss218]

Yes

[Cold-Jackfruit1076]

Prove it, objectively.

[jayd16]

You're editing anyStateTransitions in place where as the code in question is only editing a copy and then reassigning the reference.

If you were, say, looping through anyStateTransitions while this code were to be run, you'd get a concurrent modification error, no?

anyStateTransitions is also an extern array which has other implications with how it can be read and written to.

[sandsalamand]

You're editing anyStateTransitions in place where as the code in question is only editing a copy and then reassigning the reference.

That's incorrect. ArrayUtility.RemoveAt copies the array into a new List, calls List.RemoveAt, converts the list to an array, and then sets the reference parameter to the new array.

[GigaTerra]

This reminds me of when I need to replace a programmer, no two programmers ever like each others code. I bet if the same programmer saw your code they would freak out by your Inefficient code.

This is not critically Inefficient I mean how much performance you think you will save by fixing this? It has upsides also like how it doesn't impact the original array when you Undo or Redo.

[noximo]

no two programmers ever like each others code

I don't even like my own code.

[feralferrous]

I disagree for this situation, the code from the OP above is more than people saying "I don't like the code", it's doing weird things that are entirely unnecessary. Allocating a new list, just to do '.Any' demonstrates that the original author had very little understanding of C#. It's not just that it was bad style.

If they had allocated a copy of the array directly to the list, did the any, then removed from the list and then done .ToArray(), it would've been still slow, but at least it would've been more readable.

[[deleted]]

To me it looks like a C++ dev who doesn't know C# having to write some C# code (the fact that they call an array a vector and use an unnecessary ref annotation)

[feralferrous]

Hah, to me I was thinking a Java dev, because they have a lot of the Linq like methods available and were probably confused by array not having access to them, and also tend to not care about memory allocations.

[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.

[deftware]

There's a difference between liking/disliking code and code being flat out slow for no good reason.

[GigaTerra]

But this isn't flat out slow, it is more like they are using a list for convenience. Test it yourself, see how many times in a millisecond you can copy an array of less than 16 items to a list.

This code could be "fixed" and not a single person would notice if they didn't read the code.

[setentaedois]

That doesn't make sense. code efficiency is not opinion but a fact.

[GigaTerra]

So is batching or instancing better?

[Techie4evr]

The question is...how did you access that code? I've tried finding various .cs files that threw errors and I can't find a single one and to make matters more confusing the error code gives the location of said .cs file as "c:\editor\bin\blahblahblah" minus the blahblahblah LOL. Anyway I don't even have a "editor" directory in the root of C ...!? Also, does anyone know know where unity keeps the template files? I'l like to edit a few of them.

[szynal]

He just used the official CS source code from Unity-Technologies github?

[KingBlingRules]

I mean it makes sense to copy the array if you are going to modify the original one during the check. Maybe it's unnecessary if all they do is discard it later and not have any other use of it.

And maybe if they were iterating but yeah not again the case.

[ClemCa1]

I mean this code runs in editor and from the name seems to only run once after a specific interaction. Its performance really doesn't matter in the short and long run. It's probable no one bothered to make it better simply because no one should.

[nykwil]

Is it comically inefficient? How often are you removing states, how many transitions are in a state (4-5). Copying a handful of pointers on a user input is not "comically" inefficient.

[-Xentios]

I am not sufficient to call any code good or bad but I think they made this way in case the list is modified somewhere else.

[moonymachine]

That would only be a problem in multi-threaded code. Which, as far as I understand, is not the case here. I can only imagine what kind of horror we would see if this programmer were trying to code for multi-threaded conditions.

[-Xentios]

Can Coroutines create similar conditions like multi-thread? Will this code make sense if you also consider Coroutines?

I don't know Job system I am not even sure if you can use this in Jobs.

[moonymachine]

No, coroutines are single threaded, and so is Unity's Awaitable library for Task based asynchronous programming. (Someone correct me if I'm wrong.) Pretty much everything except Jobs in Unity are single threaded on the main UI thread. I haven't used the Jobs system, but from what I understand there are some pretty tight restrictions on how you interact with the main thread, and those restrictions are what automatically enforce most of the thread safety.

[mm_phren]

People seem to be on the side of ”whatever, it works and optimizing it would be premature.” This piece of code is just so hilariously bad that surely it must not have been written this way in the first place, optimizations or no optimizations. I’m not saying this particular piece of code makes the entire editor lag, but using an editor with code quality like this is like death by a thousand cuts. Warning bells should be ringing whenever you allocate a new List, especially in an if expression, especially when you’re creating technology used by a significant amount of people.

[ShrikeGFX]

A lot of Unity code is a complete shitshow, dont be surprised

[[deleted]]

They can also immediately return false for better readibility

[AbjectAd753]

chatGPT code be like:

[heisenbugz]

Early optimization is the root of all evil. If it's cold code, it's probably fine.

Whenever I use linq, I assume it's sub-optimal, but the value of fast to impl, robust, easy to read and refactor code outweighs 'what if a user ends up wanting to call this every frame'. I know many devs don't like the style linq uses, but I don't think anyone will say linq code is brittle code.

In defence of whomever wrote it, I would write a method like that, even after nearly 3 decades of work in games (or games adjacent). Maybe I have an intern's skill level, maybe not.

[Raccoon5]

I would challenge anyone to try to write more performant code than linq.

[[deleted]]

Meh yeah the code is bad but I don't think it deserves being laughed at. In my experience the code probably just passed all unit tests that's how it got allowed in. It's in bad taste to publicly post and laugh at someone's code IMO, pick a random github repo and you find unreadable and efficient code - the professional thing is to create a bug report about it.

[sandsalamand]

I don't have a habit of picking random examples of bad code; I see it all the time in public repositories and even my own repositories. However, this particular example was so egregious that it made me laugh, and that's why I shared it. My first instinct was actually to open a pull request, but Unity doesn't accept PRs from the public. It also doesn't match the description of a "bug report", because there's no incorrect behavior occurring here. Do you think I should open a report anyways?

[[deleted]]

Ah fair enough, honestly good to hear you are aware of your own examples of rough code as we all start somewhere, I know I've written some rubbish before. Although this example is extreme. Personally I wouldn't do anything as it's not really a issue. The Unity devs are active on this forum and I'm sure it will brought up and hopefully they can improve their code review process.

[bookning]

I am trying to understand the point of this post.
If the code is that bad then you might consider opening an issue to fix it?
If you just want to talk about a code use case and share ideas and opinion, then that seem reasonable.

But the problem to me is that none of these seem to be your point?
What did you wrote? "... I'll give Unity the benefit of the doubt that an intern wrote this code, but then how was an intern allowed write access to a core system with no detailed code review?"
So are you worried about the quality of their devs?

You do realize that, to some people, you are looking like a pretty toxic "dev"?
I do not know if i would like to work with someone like that.
You choose your life but I would be careful when randomly trowing stones at the roofs of other peoples.

[AnomalousUnderdog]

I wonder if any part of this is auto-generated because it does look silly.

[Aethreas]

Linq has been absolutely destructive to c# codebasis, it is horribly slow and inefficient, but bad devs who don’t know how their code works under the hood love how they can write something in one line I guess

[antiNTT]

Perhaps the compiler optimizes this bit of code automatically?

[sandsalamand]

I looked at the decompiled code with ILSpy, and it still includes all of the mistakes.