Please replace PropertyCollectionAttribute with a bool property in PropertyAttribute

Unity Engine
, ,
1

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!

2 Likes
2

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 Likes
3

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.

5 Likes
4

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

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

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?

1 Like
7

@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

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.

4 Likes
9

YAY! Thank you so much @TomasKucinskas both the addition and getting the change through! :smile:

2 Likes
10

Hello @TomasKucinskas, is this attribute already available in 2023.3.0b6? If so, please let me know the name of the attribute, I want to do some testing :slight_smile:

11

The atttribute was removed. That is the whole point of this thread and per the last message that Tomas left. The change to the PropertyAttribute is in Unity 6, not sure about that specific Unity 2023 version. You can read about it here Unity - Scripting API: PropertyAttribute.applyToCollection (unity3d.com)

13

Here is the result of my tests using Unity 2023.3.0b6

  1. Creating my basic class

Definition

public class MyScript : MonoBehaviour
{
    public GameObject myGameObject;
    public List<GameObject> myGameObjectList;
}

Result

image

  1. Creating Hide Attribute

Definition

public class HideAttribute : PropertyAttribute 
{
    public HideAttribute() : base() { }
}

Implementation

[CustomPropertyDrawer(typeof(HideAttribute), true)]
public sealed class HideAttributeDrawer : PropertyDrawer
{
public override VisualElement CreatePropertyGUI(SerializedProperty property)
    {
        return new VisualElement();
    }
}

Usage

public class MyScript : MonoBehaviour
{
    [Hide]
    public GameObject myGameObject;
    [Hide]
    public List<GameObject> myGameObjectList;
}

Result

First field called myGameObject is no longer possible to see

image

  1. Let’s add the new argument

Implementation

public class HideAttribute : PropertyAttribute 
{
    public HideAttribute() : base(applyToCollection: true) { }
}

Result

Ok… Now can’t see my collection but now I can se my the property again?

It’s only working for collection, which is kind of useless in my opinion, this is pretty much the same thing as having two attributes :person_shrugging:

image

–

I propose adding more control over where the PropertyAttribute is applied by incorporating additional boolean arguments or using an enum. Here is a refined suggestion:

Option 1: Using Boolean Arguments

protected PropertyAttribute() : this(applyToField: true, applyToCollection: false, applyToCollectionElement: false)
{
}

Option 2: Using an Enum

[System.Flags]
public enum PropertyAttributeTarget
{
    Field,
    Collection,
    CollectionElement
}

To be honest, just having no field and no collection is already a HUUGEEEEE gain, but I believe we could explore this even further, such as adding the possibility for Method, Classes and Property with {get; set;}

1 Like
14

In 6.0.12f1 it applies to both root-level fields and collections if you have applyToCollection true, which is the behaviour I would expect. Looks like they fixed it in .8f1 looking at the history for PropertyHandler.

You can override it manually per-field for older versions:

public class HideAttribute : PropertyAttribute
{
	public HideAttribute() { }

	public HideAttribute(bool applyToCollection) : base(applyToCollection) { }
}

public class MyScript : MonoBehaviour
{
	[Hide]
	public GameObject myGameObject;
	[Hide(applyToCollection: true)]
	public List<GameObject> myGameObjectList;
}
1 Like