Search Unity

  1. Unity 6 Preview is now available. To find out what's new, have a look at our Unity 6 Preview blog post.
    Dismiss Notice
  2. Unity is excited to announce that we will be collaborating with TheXPlace for a summer game jam from June 13 - June 19. Learn more.
    Dismiss Notice
  3. Dismiss Notice

Feedback Please replace PropertyCollectionAttribute with a bool property in PropertyAttribute

Discussion in '2023.2 Beta' started by MechaWolf99, Jun 2, 2023.

  1. MechaWolf99

    MechaWolf99

    Joined:
    Aug 22, 2017
    Posts:
    295
    I think the idea behind PropertyCollectionAttribute is great. Being able to use attribute property drawers on collections has been a long requested feature!

    However the current implementation using
    PropertyCollectionAttribute
    has some serious usage issues. It requires you to have separate attributes and drawers for collections. So you would need to make your own
    CollectionHeader
    and
    CollectionSpace
    attributes, and when making custom attributes like a
    ReadOnly
    , you would also need a
    CollectionReadOnly
    , and do this for every attribute. This makes it more complicated to create attribute drawers and doubles the number of attributes and drawers you need to write and users need to use/remember.

    After looking at the code, it looks like simply adding a bool property like
    applyToCollection
    to
    PropertyAttribute
    would solve both issues. The bool would allow existing attributes to just automatically work with collections, would be less code to maintain for Unity, and fewer APIs. And would not require developers to write two attributes and drawers for everything.

    And since the attribute is passed to the
    PropertyDrawer
    , we can still have collection specific implementations by simply checking the bool of the attribute from within the drawer

    The only usecase this does not cover is if you wanted a attribute to only apply to a collection and nothing else. But, to me that feels like it falls under the same sort of situation as things like the Range, and Min attributes. Which only can be applied to float and integer fields. Additionally, the bool could be virtual, so you can override it. Or even add a second get only bool property that can be overridden to determine if it is collection only.

    Thank you!
     
    PraetorBlue and stevphie123 like this.
  2. vertxxyz

    vertxxyz

    Joined:
    Oct 29, 2014
    Posts:
    110
    From a drawer you could also be able to perform different implementations by checking the property itself, but it's a great point that decorators can check the attribute.
    I would assume this approach was chosen because there's some specific usecase or problem it's solving, but I can't for the life of me figure out what it is.
     
  3. TomasKucinskas

    TomasKucinskas

    Unity Technologies

    Joined:
    Dec 20, 2017
    Posts:
    60
    Hello @MechaWolf99 . You do raise some good points.

    Initially, we wanted to do a new attribute since that would introduce the least risk for attributes that already exist and derive from PropertyAttribute. However, looking at your and @vertxxyz posts it seems there is value in implementing that functionality in PropertyAttribute. As of today, I have a draft solution that works the way you want but I need to show it around and gather feedback if that really is something we can go forward with.

    I'll keep you updated.
     
    MousePods, Ruchir, bdovaz and 2 others like this.
  4. Baste

    Baste

    Joined:
    Jan 24, 2013
    Posts:
    6,355
    Yeah, I'm in agreement here! Requiring two additional classes instead of just a bool is a bit much.

    I'm a bit sad if backwards compatibility will require the default to be that the attributes works as before (spreads to collection elements), because that behavior is the wrong behavior in 100% of our use cases. But backwards compatibility is a holy grail in Unity so womp womp.
     
  5. MechaWolf99

    MechaWolf99

    Joined:
    Aug 22, 2017
    Posts:
    295
    That's great to hear, thank you! I look forward to hearing the results! And I will second what @Baste said, having it default to work on the collection field instead of the collection elements by default would definitely be my preference. Though would 'break' previous usages. It is a fairly minor break imo. And I also agree that the vast majority of the time, working on the collection field is what is wanted.
     
  6. vertxxyz

    vertxxyz

    Joined:
    Oct 29, 2014
    Posts:
    110
    I don't really understand what breaks, surely the bool is a new optional property, and so every previous application of an attribute stays the same, and if anyone wants to add something like a Header to a collection they just go
    [Header(TargetCollection = true)]
    . Am I missing something fundamental?
     
    MechaWolf99 likes this.
  7. TomasKucinskas

    TomasKucinskas

    Unity Technologies

    Joined:
    Dec 20, 2017
    Posts:
    60
    @vertxxyz Yes, your example looks doable.

    However, if we go one step further and make all PropertyAttribute-derived attributes apply to collections by default then that would break a lot of existing attributes. Our attributes can be fixed along with the change but there are hundreds or thousands of community-created attributes for various assets or standalone that are intended to work on fields only. So changing the default behavior for attributes would break a lot of assets and make them not usable anymore.
     
  8. TomasKucinskas

    TomasKucinskas

    Unity Technologies

    Joined:
    Dec 20, 2017
    Posts:
    60
    Hello, again @MechaWolf99!

    Well, it took a bit longer than I personally expected but I've just received the notification that your idea has just been approved into Unity. The changed API should be available in future versions of Unity 2023.3.
     
  9. MechaWolf99

    MechaWolf99

    Joined:
    Aug 22, 2017
    Posts:
    295
    YAY! Thank you so much @TomasKucinskas both the addition and getting the change through! :D
     
    karl_jones and TomasKucinskas like this.