What I find wrong with Unity Developers

What I meant by "intelligent recursion" is something like this.

Code (CSharp):

1. using System.Collections;
2. using System.Collections.Generic;
3. using UnityEngine;
4. namespace LocksAndDoors
5. {
6.     public class Door : Interactable
7.     {
8.  
9.         public GameObject hinge;
10.         public bool isOpen;
11.         public float maxOpen;
12.         private bool isWorking;
13.         public float openTime;
14.         private Vector3 rot;
15.         private float time = 0.05f;
16.         public bool direction;
17.         public PickableLock optionalLock;
18.         public enum OpenAxis { yAxis, xAxis, zAxis}
19.         public OpenAxis axis;
20.         public override void ToggleTask()
21.         {
22.             if (optionalLock != null)
23.             {
24.                 if (optionalLock.isLocked)
25.                 {
26.                     optionalLock.ToggleTask();
27.                 }
28.                 else
29.                 {
30.                     optionalLock = null;
31.                     ToggleTask();
32.                 }
33.             }
34.             else if (!isWorking)
35.             {
36.                 Vector3 rotationAxis = new Vector3();
37.                 float axisPosition = 0;
38.                 if (axis == OpenAxis.yAxis)
39.                 {
40.                     rotationAxis = hinge.transform.up;
41.                     axisPosition = hinge.transform.eulerAngles.y;
42.                 }
43.                 else if (axis == OpenAxis.xAxis)
44.                 {
45.                     rotationAxis = hinge.transform.right;
46.                     axisPosition = hinge.transform.eulerAngles.x;
47.                 }
48.                 else
49.                 {
50.                     rotationAxis = hinge.transform.forward;
51.                     axisPosition = hinge.transform.eulerAngles.z;
52.                 }
53.                 if (!isOpen)
54.                 {
55.                     if (direction)
56.                     {
57.                         if (axisPosition < maxOpen)
58.                         {
59.                             hinge.transform.Rotate(rotationAxis * maxOpen / openTime * time);
60.                             Invoke("ToggleTask", time);
61.                         }
62.                         else
63.                         {
64.                             isOpen = true;
65.                             isWorking = false;
66.                         }
67.                     }
68.                     else
69.                     {
70.                         if (axisPosition > 0 && axisPosition > maxOpen || axisPosition < 1)
71.                         {
72.                             hinge.transform.Rotate(rotationAxis * -(maxOpen / openTime * time));
73.                             Invoke("ToggleTask", time);
74.                         }
75.                         else
76.                         {
77.                             isOpen = true;
78.                             isWorking = false;
79.                         }
80.                     }
81.                 }
82.                 else
83.                 {
84.                     if (direction)
85.                     {
86.                         if (axisPosition > 1 && axisPosition < 180)
87.                         {
88.                             hinge.transform.Rotate(rotationAxis * -maxOpen / openTime * time);
89.                             Invoke("ToggleTask", time);
90.                         }
91.                         else
92.                         {
93.                             isOpen = false;
94.                             isWorking = false;
95.                         }
96.                     }
97.                     else
98.                     {
99.                         if (axisPosition > 1)
100.                         {
101.                             hinge.transform.Rotate(rotationAxis * maxOpen / openTime * time);
102.                             Invoke("ToggleTask", time);
103.                         }
104.                         else
105.                         {
106.                             isOpen = false;
107.                             isWorking = false;
108.                         }
109.                     }
110.                 }
111.             }
112.         }
113.  
114.     }
115. }

 

There goes any credibility your initial rant might have had.

 

Absolutely not.

Problems:

• This code snippet looks like it should be made at least 4 times shorter.
• You use euler angles, which is a bad idea (gimbal lock),
• you use Invoke to call methods, which is a horrible idea (firstly because it uses magic hardcoded strings and second because calling a function by name makes it bypass compile time checks).
• You use fixed timestep in rotation code and do not utilize Time.deltaTime. This is a major mistake.
• You have a deep if/else tree which usually indicates that there's a dire need for refactoring.
• As far as I can tell rotational code is duplicated and could be collapsed/parametrized to a single block

For an openable door script, if you don't want to use Animator component (the way it is supposed to be done), either store start/end localPosition and localRotation as Vector3+Quaternion, or specify rotation axis as a Vector3.

So. For Update-based openable door script, it'll be

Code (csharp):

1.  
2. //untested half-pseudocode
3.  
4. public class Door: Monobehavior{
5.     public bool open = false;
6.     [Header("Door positions")]
7.     [SerializeField] Vector3 openPosition = ..
8. //you can also draw this stuff as a wireframe in drawgizmos. Or something.
9.     [SerializeField] Vector3 closedPosition = ..
10.     [SerializeField] Quaternion openRotation = ..
11.     [SerializeField] Quaternion closedRotation = ..
12.     [Header("Speed")]
13.     [SerializeField] float openSpeed = 1.0f;
14.     [Header("Internals")]
15.     [SerializeField] float lerpFactor = 0.0f;
16.  
17.     ...
18.     void Update(){
19.         float lerpChange = open ? 1.0: - 1.0;
20.         lerpChange *= Time.deltaTime * openSpeed;
21.         var nextLerp = Mathf.Clamp(lerpFactor + lerpChange, 0.0f, 1.0f);
22.         if (nextLerp == lerpFactor)
23.             return;
24.         lerpFactor = nextLerp;
25.         transform.localPosition = Vector3.Lerp(closedPosition, openPosition, lerpFactor);
26.         transform.localRotation = Quaternion.Slerp(closedRotation, openRotation, lerpFactor);
27.      }
28. }
29.  

Pretty much the only issue here is that the door script busy waits when it is neither opening or closing, wasting cpu time.

Coroutine based version will eliminate this problem and could look like this:

Code (csharp):

1.  
2.     IEnumerator animationCoroutine(){
3.         while(true){
4.             float desiredLerp = open ? 1.0f: 0.0f;
5.             float lerpDirection = open ? 1.0f: -1.0f;
6.             lerpFactor = Mathf.Clamp(lerpFactor + lerpDirection * Time.deltaTime * openSpeed, 0.0f, 1.0f);
7.             if (lerpFactor == desiredLerp)
8.                 break;
9.             transform.localPosition = Vector3.Lerp(closedPosition, openPosition, lerpFactor);
10.             transform.localRotation = Quaternion.Slerp(closedRotation, openRotation, lerpFactor);
11.             yield return null;
12.         }  
13.     }
14.  

While you might need to do some basic safety checks to prevent coroutine from being started multiple times, coroutines effectively allow you to create bunch of variables related to current task and confine them at function level, rather than making mess in member variables.
---

Also, animator based version could look like this:

Code (csharp):

1.  
2. public class Door: MonoBehavior{
3.     [SerializeField] string openParamName = "open"
4.     public bool open{
5.         set{
6.             setOpen(value);
7.         }
8.     }
9.  
10.  
11.     void setOpen(bool value){
12.         var anim = GetComponent<Animator>();
13.         if (!anim){
14.             //throw exception, complain, or just bail silently. Whichever you prefer.
15.             ...
16.         }
17.         anim.SetBool(openParamName, value)
18.     }
19. }
20.  

 

Ok, now I understand what you mean by 'recursion' is using Invoke() to make it asynchronous. Thanks for the clarification.

But honestly, I fail to see how it's more 'intelligent' or legible than an equivalent code written with a coroutine.

 

Oh wait. If the whole idea was to perform fake recursion by spamming invoke (because no recursion takes place), that's an auto-fail.

Problems:
1. The door will open at fixed steps, without producing smooth movement. It will be noticeable at high fps. 0.05 means door opening will happen at 20 fps which is below cinematic framerate.
2. The timer most likely is imprecise, so rather than 0.05 seconds you'll it'll be random number in vicinity of 0.05, meaning animation will take different amount of time depending on fps.
3. Violates principle of least astonishment.

 

Holy crap, seriously.

You can't rant about code, then post code like that...hahahah. A serious WTF moment.
________

I believe that literally every line of that function is an example of "bad"... it is the worst "door" code imaginable. Good god, that's incredible.

Code (CSharp):

1.  
2.             if (optionalLock != null)
3.             {
4.                 if (optionalLock.isLocked)
5.                 {
6.                     optionalLock.ToggleTask();
7.                 }
8.                 else
9.                 {
10.                     optionalLock = null;
11.                     ToggleTask();
12.                 }
13.             }

The only thing that could make this worse, is if it were covered in "FindObject" in a coroutine while being sold on the asset store.

 

You could side step the Lerping and Invokes by using Unity's Animation system to create an animation for the door opening and closing.

Pro's

• Let unit's animation system do the donkey work.
• Allow your game artists/animators to get the door to look right.
• Should simplify your code a bit, at least you can forget about Invoke/Co-routines.
• With Animation call backs you can toggle the door as open only when it is open enough. Or imagine wanting to trigger a cloud of mist/fog or special fx to kick in as the door opens.

Con's

• You need to build a couple of animations for each door. It used to be tricky but there is a new feature set where you could even make your door with physics record it to animation and play it in the game, folding door/chainmail door?

Also I think this could simplfy your door system down to a class with a couple of Boolean flags and an animation. To switch off a door to completely you could just turn off the component. Actually a keydoor could be a door toggling component.

 

You don't need to store anything in that class, because booleans are already stored in animation controller. My previous post had a sample for animator-based door at the end:
https://forum.unity3d.com/threads/what-i-find-wrong-with-unity-developers.482238/page-2#post-3137758

 

Apologies neg TLDR, I tend to ignore your posts or just scan them!

 

What's the comparison like in terms of performance between rotating a door using an animation and rotating a door using that update-based openable door script?

 

You should know with your sig, surely you have tested the performance of multiple door solutions?

 

I have, and in my experience the performance hit is about the same. I prefer using a physics based approach over animation for numerous reasons and with enough options available for the user like choosing between a linear rotation, smoothstep, etc. the rotation should be just as good looking as an animation.

Using animations for rotating objects is preferable over physical rotations when you're dealing with a lot of objects rotating at the same time but since we're talking about doors this is not the case in a normal setting and you usually have a few doors max rotating at the same time. It's just that this comment made me confused: "For an openable door script, if you don't want to use Animator component (the way it is supposed to be done)".

I find physical rotations to have a lot of benefits over the use of animations.

 

if you have performance issues from door animations - something somewhere went horribly wrong.

 

Just checking, is this a reply to my post? Because I have not been having any performance issues at all, with both physical rotations and/or animations.

 

Just a general comment, you shouldn't even have to worry about the performance cost of rotating a door. You certainly shouldn't have "intelligent recursion" or anything like that abomination.

 

Do you have hundred thousands doors in your level?

 

Not even close

 

Then get back to work. Door Simulator 2k17 has to be out before December 31st.

 

Your code was painful to understand, I see no way of how this is better than a coroutine. Sorry but I can't take your complaints about bad code seriously anymore

 

Posting sample code when ranting about others code is like trying to correct someones grammar. It's almost inevitable you make a mistake yourself.

With code its even worse because there's a lot more flexibility in whats wrong or right depending on stylistic preferences and how you balance readability, simplicity, performance, etc.

 

While I understand the OP's rant, I also think if you end up using bad programming habits yet deliver a good game with good performance, why should we even rant? There was a time where I cared a lot about how I coded things, I always tried to follow best practice and what not, but over time I realized that I was over-analyzing stuff and wasting time, I mean, we're only making games here, not coding a rocket to send people to Mars.

If it does not take a huge performance hit and it makes your life easier to use GameObject.Find, go for it, don't waste your time worrying about things like this unless they truly effect the performance of your game.

 

Just to be clear, I believe the problem with the OP was not that he or she insisted we should follow best practice or try to write efficient code, which I believe to be quite a sane advice as it is.

Rather, the problem was neither of the items the OP suggested can be considered a best practice per se, because the one regarding Find() method largely depends on circumstances, and the other criticism against coroutine proved to be quite insubstantial.

 

Well, the OP did bash the GameObject.Find quite good, but I don't think a good reason was given. Comes to think of it, I'm not even sure what the purpose of this thread is, obviously it's an open forum and you can post whatever, just not seeing the purpose of the topic, is it to bash others? Or just vent? Because I'm not seeing something useful to be argued here.

What I would have liked to see is why these things are bad to use in Unity, and perhaps sharing alternative methods, that would have been a better approach if you want others to learn.

 

Use this thread as an opportunity to compare bad habits you have used in the past or found in others works examples.

And share work arounds or better practices e.g:

Instead of using GameObject.Find I often use the Singleton pattern.

Code (CSharp):

1. public static ComponentName use;
2.  
3. void Awake()
4. {
5.    use = this;
6.  
7.  

Then any code that needs a reference or to call a method of the component just calls ComponentName.use...

A DontDestroyOnLoad() and singleton is a good way to get around the scene transition problem in Unity.

At least until Unity introduces an optional root scene node that survives transitions.

 

Sorry i dont find any thing intelligent with the OP code. If i would to compare that to a Coroutine, i would take Coroutine without a doubt. Overhead is not smart. Invoke is not smart.. Your also over complicating your task.

 

That's the type of singleton implementation I replaced in our code base with something that works more robust in my opinion.

The issue I have with assigning the instance field in Awake() is, at some point this Component needs to be created by something, before other code can safely access the static instance field.

We ended up with a lot of additional null checks to make sure the instance exists before accessing it, but a few programmers on the team didn't, thus the game was running in null-reference exceptions from time to time.

It caused the game code to become uglier due to the additional null-checks and from an usage point of view made it error prone, because someone needed to know about the creation order.

Over the time, my personal definition of a singleton became:
A singleton instance must never return null, but return a valid instance always.

Following this definition gets rid of additional null checks where the singleton instance is accessed and makes code more robust in my opinion.

The singleton implementation I ended up developing looks like this one:

Spoiler: C# source code

Code (CSharp):

1. static GameObjectPool _Instance;
2. public static GameObjectPool Instance
3. {
4.     get
5.     {
6.         if (_Instance != null)
7.             return _Instance;
8.  
9.         _Instance = MonoBehaviour.FindObjectOfType<GameObjectPool>();
10.         if (_Instance == null)
11.         {
12.             var obj = new GameObject("Zzz_GameObjectPool");
13.             _Instance = obj.AddComponent<GameObjectPool>();
14.         }
15.  
16.         return _Instance;
17.     }
18. }

I don't claim that this approach is the perfect solution, but I don't know of a better one for our use-case and it's working very well for us.

Accessing the .Instance property will always create an instance, only if none exists already. The funky "Zzz_" prefix for the GameObject name is to move the GameObject at the end of the hierarchy (out of sight) when using alpha-numeric sorting (this comes from a time prior multi-scene editing and was handy to have).

There are side-effects to this approach though. For example, calling GameObjectPool.Instance while a scene gets destroyed in a Components' OnDestroy() method, might create a new instance, if the GameObjectPool instance had been destroyed already. However, there are simple solutions to avoid this problem.

 

I actually have highly similar code in multiple places

Code (csharp):

1.  
2. public class GameObjectPool: MonoBehaviour{
3.     static GameObjectPool cachedInstance = null;
4.  
5.     public static GameObjectPool instance{
6.         get{
7.             if (cachedInstance)
8.                 return cachedInstance;
9.             var poolObject = new GameObject();
10.             poolObject.name = "GameObjectPool";
11.  
12.             cachedInstance = poolObject.AddComponent<GameObjectPool>();
13.             return cachedInstance;
14.         }
15.     }
16. ......
17. }
18.  

Creating an instance of an object on the first attempt to access it works well.

Amusingly the problem you're describing (with null checks) is quite similar to static initialziation order issues in C++.

 

I would really not recommend to use string or AddComponent in any singleton. Pooling can be done smarter with a CRUD ID system. For caching its ok, but not where cast happens allot.

 

I use singletons quite a bit now, and I ran into the same problem. Initially I used null checks but now I just work with the "Awake() for setup of this component, Start() for linking to any other components" idea.

To be honest, the idea of creating a new instance strikes me as simply a way of feeding useless information into the code to make it think everything is OK (PS at least the way that I use them, I can see it would work in some cases - i.e. pool manager where the instance does not need to contain any particular data). If the singleton instance is being used for anything remotely useful, or one-off (i.e. as part of an Awake() method where data related to the instance is critical to something being set up properly) then it wouldn't really help anyway and might just mask the fact that something wasn't done that should have been done.

Initially, I used to think having just Awake and Start weren't enough, that there should be another event method there, but I've learned (at least IMO) that if you need 1 more you might as well need 100 more. So I pretty much just re-factor my code around those two.

 

This is ultimately the main concern, no code is perfect, what should matter is the end product, that's where the focus needs to be. Of course there are cases where you need to be more organized and be able to build scalable code, but there are other times where you just need to get the job done. This is what I learned from a 10+ year old career as a professional programmer. There are times where you can afford such solutions, but in most cases what's important is the end result.

In this specific case, if I'm looking to release something in a certain time-frame and I know I'll be doing most of the programming by myself, I don't mind using hacks here and there just to get the game done. So long as it's a good game and it functions well (No lags, bugs, etc), I'm a happy camper.

 

Another thing people should really worry about its, when you put allot of stuff in awake and start, is that make sure your script execution order is correct, else you get all sort of ghostly bugs..

 

That's why I stick to the idea of preparing the 'this' component in Awake() and only accessing other components in Start(). If there's something that needs a more complex chain of events, I will manage it from a single script.

I don't really bother with script execution order, to me it's one of those things that makes life easy, until it doesn't.

 

There is a recommended practise from Unity and it goes like this: Use awake for internal wiring, caching components etc, and use start for those components to start talking to each other.

Do that right and you almost never need script execution order except for general update order. This is because Unity will first run all awakes then run all starts.

It won't do awake, start, awake.

 

Yes exactly, well recently i been removing most of my Start methods with my own Initializing method, so that calls happen
at the correct event. Connecting the component seems to work better when you just link them with public editor serializing.

 

Not everyone is a programmer nor does everyone need to be a programmer to make a game in Unity. Which is why the Asset Store puts Unity above all other free game engines.

Who cares if you could code for yourself what an asset does for you? People buying assets don't want to take the time or can't code what the asset is doing- that's why they bought the asset in the first place. The asset buyer is getting a powerful tool that saves them countless development time and headaches and the asset creator is getting paid for it. Win win for both parties. It's mind boggling you could try to spin this as a negative

Seems to me you're just going on a rant about not everyone who uses Unity being a crack programmer. Which is obviously absurd because programming is just one part of game development

 

I think it's a bit more meta than just a programmers skill set, it's the scalability and flexibility needed to take a small project up to a larger game.

• You have the component architecture that you will probably have to move to a managed architecture due to complexity or scale.
• You have the memory management where you start with Instantiate and Destroy and then have to move to object pools (I think object pooling should be built into the API).
• You have the prefab system that works great but only with a small level of complexity/scale after which you will probably start thinking about configurable bespoke solutions for your game.
• You have the static nature of Unity's systems that make it more difficult to go procedural or larger world.

OK say you release or are about to release a game and you need to add multiple languages, does Unity have a solution for this, were you encouraged to use it in your games?
Unity are making great strides in cloud based systems that should help provide your game a better platform I just think like others that the Unity API and best practices could be improved.

Most of this is covered in the manuals then ignored in the examples/tutorials.

Is the Playable API a first step towards a kind of programmable lego block for Unity and have you used it yet?

Do Accredited Unity developers write better* code?

*Better code is code that works and is easy to maintain (commented well).

 

Or, to put it simply... "All hail the Script Lifecycle Flowchart":

I think for me it is probably the most useful page in the entire unity documentation.
--------

Nope. It is all about "know thy tools, their uses and limitations".

For example, Create/Destroy functionality is highly similar to using object pools. You basically replace "Instantiate(prefab)" with "ObjectPool::createObject(prefab)" and "Destroy(object)" with "ObjectPool::destroyObject(object)".

 

The script lifecycle chart only makes sense when looking at one script in isolation. It actually implies all scripts will have awake, then start called before moving to the next monobehaviour to do the same but that's not the case.

Above, someone struggled with order of events with different monobehaviour derived classes, and that's where the behaviour changes.

While it's logical to us, the chart will have newbies scratching their heads and wondering why there is an awake at all and not just a start if one will be called after the other, and what would the relationship be with enable? In any case it could be improved.

 

Which is fine for experienced industry developers, but does not allow for people new to the industry.

I just think Unity could make it easier and simpler to navigate these problems.

 

You have a point. It would be nice if it was specified somewhere in big red letters that "all scripts execute the specified functions, and only after that the next function is performed.". Meaning "for all components: call awake, then for all components: call start".

I think it is more reasonable to cater the product to experienced developers and not beginners.
A beginner is supposed to stop being a beginner at one point, while professionals will be there for a long time. Also, professionals are the ones bringing in the money.

Basically, being an experienced developers means that you know how to deal with situation when something is a bit rough around the edges - because you'll never have a product where all the issues are eradicated completely. When "non-beginners" start to complain, then there's a real trouble.

 

I've personally never had many problems with awake execution orders. Other than a little experiment where I wanted two different scripts to access and modify same mesh on start ... so what happened is when you change a mesh, I didn't know it actually destroys it and makes a new one, and other scripts no longer have access to it.

But, what I do recall the most is having problems and glitches related with Update execution order. Usually where things' position depends on other things' positions, and such.
This is of course due to very bad coding, and I don't get into this sort of trouble (that much), but it did get me scratching my head very often.

 

Agreed, and Unity are doing this. There is the LLAP, HLAPI for networking. For new navigation/AI. Basically (for the benefit of readers) Unity is making the low level API generally engine source with easy to use high level open source components.

It's a structure that can keep both parties relatively content, and thus everyone wins. If they would only show a bit more love to the HLAPI components ...

 

Sounds interesting. I think I'll need to check it out at some point.

 

They're using a nuget ish thing to deploy these high level components, you can find more here https://forum.unity3d.com/threads/what-is-packman.482001/

 

"BERATE THIS FOOL!!!" - you guys 2017 ... LOL

but yeah seriously

wtf is this ??

Code (CSharp):

1.                         }
2.                     }
3.                 }
4.             }
5.         }
6.     }
7. }

 

This is all so much easier in Unreal Engine. Someone with no coding experience could throw together one of these doors in like 5 minutes with a Blueprint.

UE Timers > Unity Coroutines any day of the week etc...

 

Come on you HAVE to convert the beginner to professional at some point to get more professional, therefore more money lol. If you scare the beginner away, that's less future pro!

Also competence is a variable geometry, highly competent in art might not be as competent in code, yet be a pro! And we haven't bring sound and sfx yet ...

 

Nonsense. See Photoshop, Maya, 3DS Max, CAD, Solidworks, etc....

All world class applications that don't give two S***s about beginners. If somebody needs to or wants to perform certain tasks, they will and should invest time into the appropriate tools to learn them.

 

I use blender, and kids use photoshop, also photoshop has a metaphor that is a legacy of easing user to digital photo by replicating the tools they used as icons. I don't see what's hard in photoshop, and most people have recoil at blender.

Making something accessible is not the same as dumbing things down, though you can tell competence by the amount of bad filter use in photoshop, ie facilitation. I mean context aware fill is all about having less copy and pasting.

Efficiency of execution is what world class applications hive a S***, Productivity mean you don't have to lose time being a performative snob get the job done, si what is important.

That does not prevent spending time mastering the tools either. The goal is to turn beginner into master after all.

 

If Unity manages to scare them away they likely didn't have much of a future with game development.

Or just about any engine that has a visual scripting solution. GameFlow and PlayMaker would work well too.