Search Unity

Feedback @Unity: Please +SaveAssets() when you CTRL+S

Discussion in 'General Discussion' started by MrLucid72, Aug 5, 2019.

  1. zombiegorilla

    zombiegorilla

    Moderator

    Joined:
    May 8, 2012
    Posts:
    9,051
    This was in very early days of that company (and in start of the gaming boom here in SV), when we thought that a completely flat org chart was teh awesome! Everyone / no one is in charge! We are all adults, we'll just resolve our differences professionally like grown-ups! Rules and limitations inhibit creativity! Problems are solved with clever new t-shirts! Less documentation... more Nerf guns! Yay!
     
    Lurking-Ninja, Ryiah and frosted like this.
  2. Tzan

    Tzan

    Joined:
    Apr 5, 2009
    Posts:
    736
    I'll assume SV is Southern Vermont and problems are solved with maple syrup fights.
     
    Ryiah, Baste, frosted and 1 other person like this.
  3. frosted

    frosted

    Joined:
    Jan 17, 2014
    Posts:
    4,044
    That's a fair question.

    - The readonly keyword tells the developer that the value being set to the field will not change after construction. Period. This is enforced by compiler and is a language rule.

    - The serializefield attribute tells developers that this is going to be edited from Unity editor.

    Since these two meanings are contradictory - the reader has to take a guess at which meaning overrides the other - and what the intended reasoning is. At minimum, you're violating principal of least surprise.

    More fundamentally though, "from user side (ie. the code we write), it's not something we can interact with." I think this is a bad thing to say to your teammates. If the reason you're telling your teammates "not to interact" with that value is because it's being set by the user - that should already be implicit with the [SerializeField] tag alone. We should all know that changing user input should only be done with good reason.

    Imagine for a moment that you're not the original author and that you come to this code and think to yourself, "oh, we should validate the input. Make sure this number doesn't go negative" - you write a validation method hit compile and find that the field is marked readonly.

    "Why?" you ask yourself. Is it because we should respect user input in general? Is it because there's a very specific technical reason that this value should not be changed outside of the editor? Should you respect the readonly flag and skip adding the validation method, or should you delete the flag?

    All that it does here is add confusion. What reason would your teammate have to mutate that field that wasn't a good reason? Are there any, aside from just imagining your teammate being a doofus?

    There are reasons to violate principal of least surprise, or use filthy hacks to violate language constraints, but these should really be used with regret after a long series of unfortunate events, not just to place arbitrary usage guidelines on teammates.
     
    Socrates and angrypenguin like this.
  4. I respectfully disagree and you just told us the reason:

    The readonly does not tell you anything about the editor, it is a C# keyword, it applies to the code. The code cannot change this value. (We do not use constructors when we use Unity Editor, so constructor is not an issue here)
    Technically the "construction" happens when the deserialization happens, so if I want I can squeeze the logic in there...
    But more importantly:
    - readonly keyword tells you that the code won't change the value
    - serializefield tells you that the editor set the value and during the deserialization it will be set

    End of story. No surprise here, everything is crystal clear. That's what I learned from using Unity, sometimes you need to let go the coder-only views, we're working with an application editor here.
     
  5. Ryiah

    Ryiah

    Joined:
    Oct 11, 2012
    Posts:
    21,115
    Clearly the solution here is to include a document detailing the coding guidelines that your team follows. Below are a few examples of coding guidelines for well known open source projects.

    https://github.com/torvalds/linux/blob/master/Documentation/process/coding-style.rst
    https://developer.gimp.org/faq.html#id461985
    https://wiki.blender.org/wiki/Style_Guide
     
    Last edited: Aug 8, 2019
    Lurking-Ninja and frosted like this.
  6. frosted

    frosted

    Joined:
    Jan 17, 2014
    Posts:
    4,044
    Firstly, this isn't true in two ways.

    1 - you can use vanilla c# classes or structs in editor, including the use of constructors. Just mark the class as [Serializable] and you can edit a property of that class as a member.

    2 - Construction of the object still occurs, even for subclasses of monobehaviour. Assigning the field a value in the declaration is essentially prepends the assignment to the constructor.
    Code (csharp):
    1. public class A{
    2.   public readonly int Field = 1; // this is syntactical sugar for prepending the assignment to constructor
    3. }
    You can read about the details of object initialization here: http://www.csharp411.com/c-object-initialization/

    This still occurs with monobehaviours, its just that a whole lot of other non-standard stuff does too, so non default construction isn't recommended. That said, deserialization is not part of construction. Your mental model when working in unity may feel that it is, but it's not. This is a technical term and has a well defined meaning.


    Finally. Unity did the right thing at least in 2018.4.5.1f - and there is no ability to edit readonly fields in editor.
    Code (csharp):
    1. public class A{
    2.   [SerializeField] public/private readonly int Field = 1;
    3. }
    This field will not show up in editor. Resharper/Rider will issue a redundant tag warning. The example originally given doesn't work. So the point has already been decided on and not violating language constraints was (hopefully) a no brainer.
     
  7. frosted

    frosted

    Joined:
    Jan 17, 2014
    Posts:
    4,044
    I would agree with this.

    That said, your style guide shouldn't be violating language constraints without a really really good reason to be doing so.

    There is a time and a place for doing everything, including filthy hacks, but it's one thing to recognize that some specific context requires it (like a very specific setup for automated testing, or some weird legacy code maintenance, or having coded yourself into a corner while on a tight deadline), its another thing to advocate usage of those dirty hacks all willy nilly like. :p

    But yeah - if your whole team is down with something and it works - cool. When I work solo, I do all kinds of crap I would never ever advocate in a proper team setting. This is one of the luxuries of working solo or on a small team: you can do whatever the hell you want.
     
  8. Since I know what you're talking about and I don't have an argument with that and you didn't address my real argument here, I really don't have any room to continue this debate. :)

    (I don't care about language constraints - I don't like l'art pour l'art restrictions - at all if violating it gives me a better tool. I think this time would give us a better tool. People here, especially with substantial programming background tend to forget that our goal here is not to build the perfect C# application, our goal here supposed to be build the best game possible and the best tooling possible, language formalities are just language formalities IMHO. But it's just hypothetical, so does not matter)
     
  9. frosted

    frosted

    Joined:
    Jan 17, 2014
    Posts:
    4,044
    The real core of my argument though is that it's not a better tool. Just marking a field serializefield and private does the job without violating language constraints.
    • Why are you marking a field as readonly while allowing its mutation in the editor?
    • What's the scenario that this is protecting you from?
    • If I am a developer coming onto the project and I see value in adding validation code to the field, should seeing the readonly tag dissuade me?
    I mean it's not just about language formality, it's about how we communicate with our team and understanding exactly what we're saying to eachother in the code we write.

    Anyway, I'll respect your desire not to argue anymore, since the original example doesn't even work.
     
  10. angrypenguin

    angrypenguin

    Joined:
    Dec 29, 2011
    Posts:
    15,619
    I'm with @frosted here.
    1. Yes, the constructor is a thing, it's just a thing that we don't get to use in Unity.
    2. C# documentation is very clear that readonly fields can only be set at initialisation or the constructor, which in Unity means at initialisation only (see #1).
    3. Deserialisation happens after construction.
    4. Therefore, serializefield and readonly on the same thing are contradictory as far as clear, documented intent is concerned. You're relying on implementation details allowing you to break those rules for this to work.

    I'll admit that it's borderline, and if someone did this in code I was working on I'd not be angry or whatnot (games often rely on implementation details to get the job done, though usually for performance rather than things like this), but I would insist that they clearly comment it. Partly for the reasons @frosted mentions (least surprise), and partly because if the deserialisation implementation changes there's a risk here.

    Not one that's likely to be relevant in the vast majority of cases. And, if performance matters, there are things of much higher priority than "field vs property".
     
    Last edited: Aug 9, 2019
    Lurking-Ninja likes this.
  11. Baste

    Baste

    Joined:
    Jan 24, 2013
    Posts:
    6,331
    What I'm not getting with the argument about how this violates c# constraints is that this thing here:
    Code (csharp):
    1. [SerializeField] private int foo;
    2. public int Foo => foo;
    is already totally doing that! The backing variable is getting written to outside the class, which "violates" the intent of the
    private
    access modifier.

    It's even causing the compiler to complain that the value of the field is never set! So we're clearly well into violating language constraints with the [SerializeField attribute].

    This argument mirrors the argument here. The only fundamental difference I see is that Unity already supports
    [SerializeField] private
    .


    I used to think that as well. These days I'm more inclined towards a simple rule; if there's a way to write the code that's faster, and that way of writing doesn't incur a cost in readability or development speed, it's bananas to write the slower version. All of the little "performance doesn't matter in this case" calls end up mattering when summed together.


    Same reason I'm marking a field as private while allowing it's mutation in the editor (surely the editor is a different type!). I need the semantics of readonly/private at runtime, but I don't want those semantics at edit time.

    Mainly what I'm imagining is working in a large file. You remember that there's a member in this type named "foo", so you write foo, and then you do things with it. You forget to go check if it's actually a backing field for a property.

    I mean we already kinda solve this by having backing fields have a special syntax - we usually use _name for the backing field and Name for the property.

    Eh? We don't put validation code on very many fields. If they need to fit within a range, that's solved by custom drawers. If the validation needs to be introduced, then the field can't be readonly anymore, it's no biggie.


    Fundamentally, this isn't very important - we're completely fine as-is, and I don't think Untiy should drop anything in order to fix this. That being said, I assume that since reflection can set readonly fields, there's a chance that the only reason why this doesn't already work is that there's a line of code in Unity's serialization API that goes "skip fields that are marked as readonly". If you could just remove that, and have this work, then I think that's worthwhile.

    Or there might be more esoteric reasons why it's not supported, in which case taking the time to support it is probably not worth the cost.
     
  12. angrypenguin

    angrypenguin

    Joined:
    Dec 29, 2011
    Posts:
    15,619
    Is it?

    That's well documented functionality which could be validly implemented in the MonoBehaviour base class. We happen to know it's not, but that's more to do with the engine/serialisation implementation than anything else (I think?).

    More importantly, it's an intentional and well documented behaviour of the environment we're working in. It shouldn't surprise or confuse anyone familiar with Unity.
     
  13. Baste

    Baste

    Joined:
    Jan 24, 2013
    Posts:
    6,331
    It's a private field. It can't be written to by the base class. To implement this through the MonoBehaviour class itself, the MonoBehaviour class would have to do reflection, so there's literally no difference.


    This is correct. The same would hold for serializable readonly fields if they were implemented. That was the point I was trying to get accross.
     
  14. angrypenguin

    angrypenguin

    Joined:
    Dec 29, 2011
    Posts:
    15,619
    What changed your mind?

    How..? Why are they all happening simultaneously to be able to be summed together?

    If they are things that happen enough to impact performance then there are other efficiencies I'd consider first which would probably make this moot, eg: a data oriented approach.
     
  15. angrypenguin

    angrypenguin

    Joined:
    Dec 29, 2011
    Posts:
    15,619
    It's an attribute, so isn't the use of reflection assumed here?

    SerializeField is provided by the MonoBehaviour / Unity environment. readonly is a C# feature. I think that's why I see them differently.
     
  16. angrypenguin

    angrypenguin

    Joined:
    Dec 29, 2011
    Posts:
    15,619
    That said, I do see your point. If it's ok in the context of this environment for "private" to be used differently to usual in the presence of an attribute, why can the same not apply to other things?

    And, off the top of my head, my initial answer is that yes, it could apply elsewhere, but that we should do such things as seldom as possible. If you have too many special cases they cease to be special cases, and are instead just a mess. ;)
     
  17. Baste

    Baste

    Joined:
    Jan 24, 2013
    Posts:
    6,331
    That was what I was going to reply :p And, yeah, totally fair, adding another way Unity C# is different from standard C# is maybe something that should be avoided. The UnityEngine.Object == null implementation has baffled quite a lot of C# devs that's new to the Unity ecosystem.

    What I mean is that there's probably no measurable difference between if a member is a field or a property. There's a measurable difference between if all members are fields and if all members are properties. Member access happens a lot accross the frame, so it will sum up.

    Those kinds of things are not things that are easy to go back and fix, and they won't show up when profiling. But, if you do them right the first time around, you might end up saving quite a few cycles in the long run. As I said, this only applies if there's no loss of readability or development speed when you do things the faster way.

    That's orthagonal to broader design decisions (ie. DOD).
     
  18. frosted

    frosted

    Joined:
    Jan 17, 2014
    Posts:
    4,044
    The thing is, there are real, pragmatic reasons for using
    [SerializeField] private int foo
    . Probably most importantly, it allows you to filter the variable out from intellasense when you're a user of the class. Reducing the size of the public interface has value, even in monobehaviours that are sadly bloated by a half million excess members.

    Private also informs the developer that a variable has a certain limited scope, and that modification to the class can be made without changing the public interface.

    As I've said before, there are arguments to be made for hacks, workarounds or violations. Sometimes the benefits do outweigh the costs, and marking a private field with [serializefield] is one of those times when the benefits are real.

    Look, here's the reality. Marking a field as [SerializeField]readonly - in real term is attempting to 'protect' downstream developers from mistakes they won't realistically make. Developers don't just randomly mutate values for no reason.

    Again, this comes down to trusting and empowering your team. Trust your fellow developers to not do something like modify a value being fed from user without a good reason to do so.

    Trust your team to not be terrible.
     
    Baste likes this.
  19. Baste

    Baste

    Joined:
    Jan 24, 2013
    Posts:
    6,331
    I'm not sure if I don't trust myself to not be terrible. But, yeah, you're right, it's probably overly protective.

    Then again, you could make the same argument for access modifiers in general. The Python community prides itself in that it doesn't need modifiers to know which data should and should not be touched.
     
    Lurking-Ninja and frosted like this.
  20. frosted

    frosted

    Joined:
    Jan 17, 2014
    Posts:
    4,044
    IMO, the reason I believe in access modifiers is that it lets you 'slim down' the public interface.

    Philosophically, you can think of that as protecting downstream devs from making mistakes, but I prefer to think of it as helping to filter out noise so they can find what they're looking for easier. Admittedly that's kinda arbitrary, but there's some truth to it. I like my intellasense popups to be as minimal and obvious as possible.

    In general, we all err on different points - but I really believe strongly in empowering other devs I work with.
     
    IARI, angrypenguin and Baste like this.
  21. Peter77

    Peter77

    QA Jesus

    Joined:
    Jun 12, 2013
    Posts:
    6,609
    According to the following page, readonly also comes with a performance penalty in some cases. The overhead comes from doing a copy of the readonly field always, even if caller code doesn't change it and the overhead depends on the size of the field.
    https://codeblog.jonskeet.uk/2014/0...e-surprising-inefficiency-of-readonly-fields/

    However, it seems using readonly does not make a difference when using IL2CPP:
    https://jacksondunstan.com/articles/4689
     
    Ryiah and Baste like this.
  22. Baste

    Baste

    Joined:
    Jan 24, 2013
    Posts:
    6,331
    That's pretty interesting! I'll remember that.
     
    Peter77 likes this.