Search Unity

Resolved Function doesn't execute fully???

Discussion in 'Scripting' started by nikofon007, Dec 15, 2023.

  1. nikofon007

    nikofon007

    Joined:
    Sep 10, 2020
    Posts:
    38
    I have a function:
    Code (CSharp):
    1. public async Task<Weapon> Create(WeaponSpawnConfig config)
    2.         {
    3.             Debug.Log($"Started spawning weapon: {config.baseWeaponConfig.WeaponType}");
    4.             Weapon res = (Weapon)Activator.CreateInstance(config.baseWeaponConfig.WeaponType, args: config.baseWeaponConfig);
    5.             Debug.Log($"Created instance of weapon type {config.baseWeaponConfig.WeaponType}: {res == null}");
    6.             var weaponModel = Addressables.LoadAssetAsync<GameObject>(config.baseWeaponConfig.ModelPath);
    7.             await weaponModel.Task;
    8.             res.InstantiatedModel = GameObject.Instantiate(weaponModel.Result, config.spawnPosition, config.spawnRotation, config.weaponParent);
    9.             Debug.Log($"Spawned weapon: {res.WeaponConfig.WeaponType}");
    10.             return res;
    11.         }
    When I run it, the first Debug.Log gets shown in the console normally. But the second one just does not appear. There is no error or something, the game runs completely normally, except the rest of the function is not executed, so the weapons do not appear.

    Wtf is happening?
     
  2. nikofon007

    nikofon007

    Joined:
    Sep 10, 2020
    Posts:
    38
    Ok, I figured it out.

    It was due to my class that I created an instance of with Activator.CreateInstance not having a suitable constructor.

    But why wasn't it throwing an error or something?
    This is super weird
     
  3. Kurt-Dekker

    Kurt-Dekker

    Joined:
    Mar 16, 2013
    Posts:
    38,697
    You should NEVER have any constructor for MonoBehaviour or ScriptableObject classes.

    Use Awake() for that sorta thing.

    You also could never call
    new
    to get such a type. You can for a GameObject() however.

    For similar reasons, Activator also won't get you anywhere with Unity objects, AFAIK.

    Otherwise, I like this pattern for factory "completeness:"

    Factory Pattern in lieu of AddComponent (for timing and dependency correctness):

    https://gist.github.com/kurtdekker/5dbd1d30890c3905adddf4e7ba2b8580
     
    CodeSmile likes this.
  4. CodeSmile

    CodeSmile

    Joined:
    Apr 10, 2014
    Posts:
    5,892
    Agree, this Activator.CreateInstance reeks of bad design in this place. Especially if the created instance derives from UnityEngine.Object - that's an absolute no-go! Even if it's a regular C# System.Object class you wouldn't want to use Activator to create it based on its type => see Factory pattern.

    Also note that you can do:
    var weaponModel = await Addressables....

    Rather than awaiting the Task property.
     
  5. nikofon007

    nikofon007

    Joined:
    Sep 10, 2020
    Posts:
    38
    It's not a MonoBehaviour.
    Can you elaborate why is it a bad design pattern. I have different implementations of base class Weapon(). Creating a separate factory for each implementation while those factories maintain practically the same functionality would be a bad design, no?
     
    Last edited: Dec 16, 2023
  6. nikofon007

    nikofon007

    Joined:
    Sep 10, 2020
    Posts:
    38
    Btw, non of the above answer the question why there was no error thrown and the game continued to execute normally
     
  7. CodeSmile

    CodeSmile

    Joined:
    Apr 10, 2014
    Posts:
    5,892
    The problem with Activator.CreateInstance is that it lets you create any type. It is too powerful for this purpose.

    It will create types you did not anticipate, or types that do not exist, or types of the same name but they happen to be in another assembly. It also bypasses type safety, the WeaponType property is probably of type "Type" and therefore you could also assign a System.Object or string type or even 'null' and then that code would fail because it cannot be cast to (Weapon). It may even be a security risk, injecting any type here could run code that isn't yours.

    It is definitely bad to just cast to (Weapon) here (type cast exception) rather than using the 'as' operator (res will be null if it's not the correct type).

    In fact I think this is what's happening here. The first log appears, the second doesn't, therefore the CreateInstance line or the Debug.Log somehow failed. More than likely, you have an exception thrown here but perhaps you have the "error" messages filter turned off in the console?

    You should set a breakpoint on that line and just debug to see what's being fed into it and if there is any exception thrown and if not, that the resulting instance actually is the expected type.

    Oh and it may not throw an exception as expected either due to async/await or perhaps due to the calling code catching and ignoring the exception.
     
    nikofon007 likes this.
  8. Kurt-Dekker

    Kurt-Dekker

    Joined:
    Mar 16, 2013
    Posts:
    38,697
    Probably because the part that barfed was handled on another thread. Unity runs a loading thread that is responsible for calling all the constructors for Components and probably almost anything in the UnityEngine.Object namespace.

    Besides, once you start breaking Unity's rules and calling new (which Activator of course does) on engine objects, who knows what is happening inside the software?

    Unity supports and relies heavily upon dependency injection. This is done via Prefabs or Scenes containing all the information necessary to create (eg, inject) all the necessary types on demand.

    Yes you can do it all in code but then guess what? You get to maintain all that code!

    Instead, make Prefabs out of all the "things" you contemplate making and just call Instantiate<T>() to make them.

    This also might interest you:


    Using Interfaces in Unity3D:

    https://forum.unity.com/threads/how...rereceiver-error-message.920801/#post-6028457

    https://forum.unity.com/threads/manager-classes.965609/#post-6286820

    Check Youtube for other tutorials about interfaces and working in Unity3D. It's a pretty powerful combination.
     
    CodeRonnie likes this.
  9. nikofon007

    nikofon007

    Joined:
    Sep 10, 2020
    Posts:
    38
    As I said before I don't use activator on MonoBehaviours

    IMO, you never build your architecture around loading prefabs. Unity doesn't even have a proper serializer, so you need to build some gimmicks to make it possible to set needed params in editor which just adds more work, not mentioning the fact that your IDE and version control will not help you track all the changes that were done to those prefabs by someone else which increases work for your designers.
    And it's also bad for performance because you pile up a bunch of unused data instead of discarding it like you do.

    You are probably right about unity not logging errors that happen not on the main thread, because I had different types of errors happening on the "async threads" and none of them were logged (and they don't stop the game either, at least in editor). I wonder if it leads to some kind of security vulnerability if it also doesn't crush the game when it's built :thinking:
     
  10. Kurt-Dekker

    Kurt-Dekker

    Joined:
    Mar 16, 2013
    Posts:
    38,697
    Well, sure, I guess you can take that approach. In that case, not sure why you're even using Unity tbh.

    I prefer to leverage the power that Unity gives me to employ NON-coders to create content that I can effortlessly use in my games.

    Our work teams (anywhere from 4 to 12 contributors) couldn't do anything if we all had to write code for everything.

    Prefabs are a dream, especially now that prefabs themselves are effectively "override"-able with prefab variants.

    Your code is not the application. Unity is the application. Your code is just a minor guest at the party:

    https://forum.unity.com/threads/res...-problem-when-using-ioc.1283879/#post-8140583
     
  11. nikofon007

    nikofon007

    Joined:
    Sep 10, 2020
    Posts:
    38
    How does my approach limit the non-coders? I just need to create a proper serialization tool for them to work in (which you do need to do anyway because unity can't even serialize simple stuff like dictionaries.

    I wonder what your team have produced with that approach (it's not a joke, can you leave a link? I don't think you are talking about games that are on your appstore). For example I'm really interested to hear how do you run unit-tests. I can create thousands of config files with variable parameters to be fed into automatic testing with a click of a button, and all of them will practically take no space; but I don't see how you do it with prefabs.

    I don't have a lot of experience working as a dev, but I've never met or watched or read a person who would recommend sacrificing SOLID (prefabs = strong dependency) for some illusory convenience that you would get anyway just by writing a simple editor tool that you will probably need anyway.
    +The question with version control still stands, do all of your 4-12 contributors just remember what there was before and what changes they made? It seems super unreliable.
    And how do you introduce knew team members? You need to tell them the whole structure of the project when they are assigned to it? IMO it's much easier if you again have a special tool for creating design configs that looks approximately the same across all your company's projects.

    I really can't see a single point of convenience nor usefulness in that approach. Both from technical and HR points of view.

    Sorry for the rant, maybe I'm just missing something in your point of view and I'm trying to figure it out.
     
  12. spiney199

    spiney199

    Joined:
    Feb 11, 2021
    Posts:
    7,861
    Unity's serialisation is limited because it needs to be fast. If you've ever overused another serialiser, like the Odin Serialiser part of Odin Inspector, you'll realise the amount of overhead it introduces.

    And I feel like you're forgetting about scriptable objects. You can easily use scriptable objects as a factory for plain C# objects if that's what you need.
     
  13. nikofon007

    nikofon007

    Joined:
    Sep 10, 2020
    Posts:
    38
    Well, maybe. But I had never in my life seen anyone who used Odin and then was like: "Nah, that's too slow, Imma go back to native untiy". If you used Odin once you are basically hooked on it, so I really wonder why unity doesn't just make it into the base kit.

    You are right about scriptable objects, I also use them. But I think using them as a factory breaks single responsibility, because you are using the same class both as a data snapshot and a factory. In this project I use scriptable objects that can output a config class that I feed into a factory (which in itself is just a C# class, not a monobehaviour). Thus later if this project scales I can replace scriptable objects with a serializer tool fit specifically for this project's needs.
     
  14. SisusCo

    SisusCo

    Joined:
    Jan 29, 2019
    Posts:
    1,329
    You can get interfaces fully working with serialized fields and the Inspector, so making full use of prefabs does not have to mean you can't also follow all the SOLID principles simultaneously. It's a pity it is not better supported out of the gate, but it is what it is :)

    Components can also be designed to be unit-testing friendly. All you need is to add a mechanism for injecting dependencies as easily in code as it is to do with the Inspector, and after that unit-testing is no longer problematic at all (well, except for coroutines at least).

    That sounds a lot like you might be trying to reinvent the wheel, instead of using all the powerful tools that Unity already offers, that have been polished and improved over several years by many professional developers, and battle-tested by thousands of game studios. There's also a huge ecosystem of extensions built on top of the pre-existing tools in Unity, most of which I guess would be incompatible with your custom solution.

    Almost all Unity developers that would join your team will also be intimately familiar with the prefab-based workflow in Unity. In many cases you basically just have to point out the project folder where the prefabs are located to a developer, and they'll be able to figure out all the rest based on all their pre-existing knowledge and the intuitive visual Inspector. An animator can find the Animator component, and from there the referenced AnimatorController, and go work on the animations. An artist can find the Renderer / Image components, and from there the referenced art assets, and go make changes to them.

    If you have a custom solution, then you will likely have to train every person that joins from scratch how to use it. If you have to open some custom editor window to edit things, people are very likely to forget how they can open that window - especially once a project has grown to the size where there are dozens of custom windows. If you just used prefabs, then every artist would know how to search for a prefab by their name in the Project window.

    I've seen it several times over the years that when somebody tries to fight the Unity way of doing things too much, building complicated custom systems to replace the built-in ones, they more often than not eventually realize it's not actually worth all the effort, and ditch the systems they built. There is another design principle in addition to the SOLID ones, that reminds us not to overdo it: KISS.

    You can for sure use version control to track changes made to prefabs. The commit message can be used describe all the changes made to an asset, just like with code changes. If you use text serialization for prefabs, then you can even see the exact changes made to the YAML listed - not the most human readable format to be sure, but it's usable in a pinch.

    The only big downside in my book is that when two team members accidentally modify the same prefab at the same time, merging their changes together can be way more difficult than with code based changes. For this reason I think that doing everything with assets can also be problematic. There are some things that are better done with assets, and some things that are better done in code.
     
    DragonCoder likes this.
  15. Kurt-Dekker

    Kurt-Dekker

    Joined:
    Mar 16, 2013
    Posts:
    38,697
    My games are just that, hobby / learning. I'm talking about my day job where we operate games at commercial scale.

    We tried unit tests but they didn't catch any bugs.

    The bugs we get aren't findable by unit tests. The bugs we deal with are more like:

    "If user was watching a Vungle ad video and gets a phone call on iOS 17, when the app regains focus all button touch rectangles are offset to the right and down by 1/2 an inch. There are no exceptions thrown and nothing in the logs."

    Unit tests, for all the gallons of digital ink spilled on Medium about it, are essentially useless when the target is a complex UI driven and interleaved with a gaggle of different third-party libraries operating in real time.
     
  16. nikofon007

    nikofon007

    Joined:
    Sep 10, 2020
    Posts:
    38
    So basically I need to do even more work to make it work? Why would I want that?

    I think handing out a paper that says: to make a weapon config file go to Window -> ConfigCreator -> Weapons is easier then waiting for everyone to get familiar with the project structure.

    Let's get this straight, I'm not saying that prefabs are some garbage and you never should use them. I'm just opposing the point that Kurt made about creating a separate prefab for each "instance" of some object in your project. In my opinion you should use prefabs for "static" objects, for example: you have a player object that will always have components like Player.cs Locomotion.cs AttackHandler.cs, MeshRenderer, etc. And you make this into a prefab. But at the same time you create player configs, which contain information like what model the "player object" uses, how much health does it have, what attacks does it use and so on. And the best way to store this configs is json (during development to make it readable) and binary for deployment. That way you save space on hard drive, configs file are faster to load at runtime (bcs they are small) and you don't store excessive in RAM, because you discard the config files after getting everything you need (while it doesn't work that way with prefabs).

    I wish I could find a job offer where they would ask for strong knowledge of KISS and not SOLID :(:(:(
     
  17. DragonCoder

    DragonCoder

    Joined:
    Jul 3, 2015
    Posts:
    1,696
    You know you can have exactly that workflow with scriptable objects through the universally known create-asset menu?

    Have to agree with the comments above. Try avoiding reinventing the wheel. Unity is already complex enough (as will any proper game engine be) :)


    This is literally what SOs have been designed for. Blobs of data that live in asset context (as opposed to scene context).

    I highly doubt your jsons are really saving any significant RAM or space.


    As for prefab variants, those are useful when you need to change things more massively, especially swap components. Or just for faster workflow when the instances are not brought onto the scene via code but through drag and drop since then you don't need to manually assign the SO.

    It will depend on the usecase whether SOs or prefab variants are doing the job better but those are two viable solutions already built into the engine...
     
    Last edited: Dec 17, 2023
    SisusCo and Kurt-Dekker like this.
  18. nikofon007

    nikofon007

    Joined:
    Sep 10, 2020
    Posts:
    38
    And also they rely on unity serializer which, as we concluded before, is not the best thing in the world.
    And also they are in YAML reading which (for version control) is a form of torture according to Geneva convention, which we also established earlier.

    SOs are a viable concept to get your prototype up and running, but if you use them in production (unless your game doesn't require any form of balancing after it's released) ur mad.
     
  19. Kurt-Dekker

    Kurt-Dekker

    Joined:
    Mar 16, 2013
    Posts:
    38,697
    YAML is only really a problem if you end up having to merge.

    There's a Unity YAML-merger plugin you can try but it's not great either.

    Ideally you keep the YAML bits small enough that merging is rarely necessary. This means if you have a humungous scene and longer-lived branches, you may need to do a bit of extra work to make your life better:

    - NEVER put manager objects in scenes (just have them load dynamically on demand)
    - use multiple additive scenes
    - use ScriptableObjects to break apart config data in your scene: rather than a huge list of random config on some random script or prefab deep in your scene, break it into areas of concern, put those into SOs, drag the SOs in, and then when you have to change them, change the SO instance, not the scene using it. This has the side benefit of keeping your YAML hierarchy much flatter and hence more-mergeable.

    We configure everything from SOs. It's a dream compared to the alternatives. It works better than every other non-Unity dependency injection mechanism because they are a first class citizen in Unity. I almost never write custom editors for SOs either, except in cases where a custom editor can really add a lot of value to the editing process.

    On some of our games we have an interface, I think called
    IAsyncInitializable
    that lets us drag all these SOs into an ordered list and have a generic "stander-upper" loader ScriptableObject that also expresses whether it is ready for business yet.
     
    SisusCo likes this.
  20. nikofon007

    nikofon007

    Joined:
    Sep 10, 2020
    Posts:
    38
    Well I guess that you make different kind of games than I. I'm making a prototype and I already run into a problem that scriptable objects don't have multiediting for inherent types (meaning that if you have SO A, and SOs (B: A) and (C: A) you can't edit them all at the same time).
     
  21. DragonCoder

    DragonCoder

    Joined:
    Jul 3, 2015
    Posts:
    1,696
    If that is a common usecase, then rather use composition instead of inheritance.
    Did you implement inheritance in a raw data/json based system?
     
  22. AcidArrow

    AcidArrow

    Joined:
    May 20, 2010
    Posts:
    11,755
    I hate this advice, but you're probably right.
     
  23. nikofon007

    nikofon007

    Joined:
    Sep 10, 2020
    Posts:
    38
    Well, composition is kinda awkward with scriptable objects because untiy doesn't serialize properties, isn't it?

    What do you mean? I did it with scriptable objects
     
    Last edited: Dec 17, 2023
  24. spiney199

    spiney199

    Joined:
    Feb 11, 2021
    Posts:
    7,861
    And I've seen tons of people have to unwind using Odin to serialise everything because the overhead it added to loading times due to the deserialisation overhead was becoming unacceptable.

    You can use it to serialise whatever Unity can't when you need, otherwise, it's better to stick with Unity's lightning fast serialisation.

    And nowadays we have
    [SerializeReference]
    so that is one less reason to become too dependant on Odin's serialisation.

    I think you're too hooked on SOLID principles.
     
    MaskedMouse and SisusCo like this.
  25. DragonCoder

    DragonCoder

    Joined:
    Jul 3, 2015
    Posts:
    1,696
    Since a few years it actually does serialize simple properties; you gotta add "field" before it:
    Code (CSharp):
    1. [field: SerializeField] public double duration { get; protected set; } = 2.0;
    However what does that have to do with composition?
    What I was rather thinking of was something like this which does not need properties in a weapons usecase:

    Code (CSharp):
    1. [CreateAssetMenu(menuName = "Weapons/WeaponType")]
    2. public class Weapon : ScriptableObject
    3. {
    4.     [SerializeField] float cooldown;
    5.     [SerializeField] List<WeaponEffect> effects;
    6.     internal void Shoot(Transform shooter)
    7.     {
    8.         foreach (WeaponEffect weapon_effect in effects)
    9.             weapon_effect.Trigger(shooter);
    10.     }
    11. }
    12.  
    13. public abstract class WeaponEffect : ScriptableObject
    14. {
    15.     internal abstract void Trigger(Transform shooter);
    16. }
    17.  
    18. [CreateAssetMenu(menuName = "Weapons/WeaponEffects/PoisonCloud")]
    19. public class PoisonCloud : WeaponEffect
    20. {
    21.     [SerializeField] private float radius;
    22.     [SerializeField] private float damage;
    23.     internal override void Trigger(Transform shooter)
    24.     {
    25.         // for all enemies in $radius around #shooter
    26.         //    do $damage
    27.     }
    28. }
    29.  
    30. [CreateAssetMenu(menuName = "Weapons/WeaponEffects/SimpleProjectile")]
    31. public class SimpleProjectile : WeaponEffect
    32. {
    33.     [SerializeField] private float speed;
    34.     [SerializeField] private float damage;
    35.     // ProjectileCollider shall be a script at the base of a projectile prefab,
    36.     // that way you can instantiate the whole prefab and don't need to call
    37.     // GetComponent to get this script.
    38.     // Alternatively use GameObject so anything can be created by the weapon.
    39.     [SerializeField] private ProjectileCollider projectile_prefab;
    40.     internal override void Trigger(Transform shooter)
    41.     {
    42.         // ProjectileCollider projectile = Instantiate($projectile_prefab, shooter);
    43.         // projectile.damage_to_cause = damage;
    44.         // Set the $speed and direction of the projectile etc.
    45.     }
    46. }
    47.  
    The weapon is now a composition of several different weapon effects, allowing you to combine them and edit independently.

    Were you not proposing an alternative system that uses json configs?
    I mean you criticize a problem with SOs when you use inheritance, so how do you avoid the problem in your alternative solution?
     
    Last edited: Dec 17, 2023
    SisusCo likes this.
  26. Kurt-Dekker

    Kurt-Dekker

    Joined:
    Mar 16, 2013
    Posts:
    38,697
    It took me a long time to come to this. I really wanted to "do it in the Unity scene..."

    On the one hand my policy limits your good use of the Unity editor... BUT!!

    In reality I find that putting manager objects into scenes just has too many follow-on edge cases requiring you to know a lot about the true lifecycle of an object.

    Do the work, make the hookups in code, your future self will thank you.
     
    AcidArrow likes this.
  27. nikofon007

    nikofon007

    Joined:
    Sep 10, 2020
    Posts:
    38
    Well, this still doesn't work, does it? Forexample if you have:
    Code (CSharp):
    1. public interface IAOE
    2. {
    3.     public float Radius { get; set; }
    4. }
    Code (CSharp):
    1. [CreateAssetMenu(menuName = "Weapons/WeaponEffects/PoisonCloud")]
    2. public class PoisonCloud : WeaponEffect, IAOE
    3. {
    4.     [SerializeField] internal float poisonDuraion;
    5.  
    6.     [field: SerializeField] public float Radius { get; set; }
    7.  
    8.     internal override void Trigger(Transform shooter)
    9.     {
    10.         // for all enemies in $radius around #shooter
    11.         //    do $damage
    12.     }
    13. }

    and
    Code (CSharp):
    1. [CreateAssetMenu(menuName = "Weapons/WeaponEffects/MeteorShower")]
    2. public class MeteorShower : WeaponEffect, IAOE
    3. {
    4.     [SerializeField] internal int NumberOfMeteors;
    5.  
    6.     [field: SerializeField] public float Radius { get; set; }
    7.  
    8.     internal override void Trigger(Transform shooter)
    9.     {
    10.        //Do something
    11.     }
    12. }
    There is still no way to multiedit radius of those two objects:
    upload_2023-12-18_11-10-55.png

    More than that, as was said previously, IMO implementing any kind of logic in SOs that also store stat data breaks single responsibility.
    As I said before, I never stated that SOs are completely useless. They are a tool for prototyping, but they do not scale well, so as soon as you are out of prototyping stage you should ditch them for a proper serialization tool.