[HELP PLEASE] CustomPropertyDrawer CreatePropertyGUI called multiple times with Interface inheritance

Hello hello!

I hope you’re doing alright. Basically, I’m facing an issue since some weeks now, and tried a lot of alternatives to what I want to do, but nothing helped me really much.

Here, I got a basic Interface:


 public interface ITestingAttribute { }

And then, I this first custom Attribute:


[AttributeUsage(AttributeTargets.Field, Inherited = true, AllowMultiple = false)]
public class IFirstTesting : PropertyAttribute, ITestingAttribute { }

And finally, this one:


[AttributeUsage(AttributeTargets.Field, Inherited = true, AllowMultiple = false)]
public class ISecondTesting : PropertyAttribute, ITestingAttribute { }

After that, I made a CustomPropertyDrawer to do some testing. Here it is:


using System.Linq;
using UnityEditor;
using UnityEditor.UIElements;
using UnityEngine;
using UnityEngine.UIElements;

[CustomPropertyDrawer(typeof(ITestingAttribute), true)]
public class ITestingDrawer : PropertyDrawer
{
    public override VisualElement CreatePropertyGUI(SerializedProperty property)
    {
        // Tracking every Attribute of the property

        TrackEveryAttribute(property);

        // Creating the container to return

        VisualElement propertyContainer = new()
        {
            name = property.name + "_Container"
        };

        // Creating the PropertyField of the property

        PropertyField propertyField = new(property)
        {
            name = $"{property.name}_PropertyField"
        };

        // Adding the PropertyField to the container and returning it

        propertyContainer.Add(propertyField);
        return propertyContainer;
    }

    private void TrackEveryAttribute(SerializedProperty property)
    {
        // Get every custom attributes of the property field

        string[] attributes = fieldInfo.GetCustomAttributes(typeof(ITestingAttribute), true).Select(attr => attr.GetType().Name).ToArray();

        // Log each Attribute

        if (attributes.Contains(nameof(IFirstTesting)))
        {
            Debug.Log($"The property '{property.name}' contains the Attribute {nameof(IFirstTesting)}");
        }

        if (attributes.Contains(nameof(ISecondTesting)))
        {
            Debug.Log($"The property '{property.name}' contains the Attribute {nameof(ISecondTesting)}");
        }
    }
}

Bascially, when I use my two Attributes on a property, like that:


using UnityEngine;

public class FirstTester : MonoBehaviour
{
    [IFirstTesting]
    [ISecondTesting]
    public string testString;
}

‘CreatePropertyGUI’ is called two times. Now here is the twist:

  • When I return ‘default’, ‘null’, or ‘new()’, then ‘CreatePropertyGUI’ is called only one time.
  • What I’m trying to do, is to draw a property, according to its Attributes, in a single Drawer, because I have to do some different settings when 2 Attributes or more of the same Interface are added to it.

When I draw the property with this code, I obtain this in the UI Toolkit Debugger:

Meaning that:

  • The PropertyField is not really replaced by my container, but got it as its child,and when CreatePropertyGUI is called a second time, the PropertField that is overrided is not the first one (PropertyField:testString), but the previous one that I made (testString_PropertyField).

For me, it’s a problem because it keeps adding children to the main element based on the amount of Attributes I add to my property, instead of replacing the previous ones.

Also, I tried to add inside a static Dictionary the first returned container, in the hope of returning it again if CreatePropertyGUI is called again, but it created a StackOverflow error.

So, how can I handle multiple Attributes with a single drawer, without having CreatePropertyGUI called more than one time? And if it has to be called more than one time without any other alternative, then how can I avoid creating more and more children every time? Can I just find a way to return what I already created?

.

I’m sorry for the very long message, if anyone can help, I’d be very happy about it. Take care,

Ilario

.

EDIT: I added the scripts I’m using for you if you need them.
Drawer-Testing-Ilario.unitypackage (1.9 KB)

Should I consider it as a bug and report it to the staff?

I think this is the problem.

A PropertyField will use a custom property drawer, so you trigger the same PropertyDrawer twice. Im surprised its not more than twice.

I think for this use case you may want a DecoratorDrawer Unity - Scripting API: DecoratorDrawer

Hello!

Thank you for your answer!

Actually, I use my Attributes not to decorate my properties, but to place them at a different position, with a different size, color, font, etc, and under a certain condition.

I tried to look at the Decorator, but apparently, it doesn’t help me achieving what I’m trying to achieve unfortunately.

I don’t want to use a compose Attribute of my others two, because I won’t have only 2 Attributes available to use on a property.

This is why I’m still stuck. I can still keep going like I already do, meaning that I draw the property once, and re-draw it again, but it doesn’t replace the previous one as I saw, it just creates some new child element under the previous drawn one. This can be a problem when we use a lot of Attributes, or when we got a lot of properties to customize, making the UI elements 2 times heavier at least…

Sorry in advance for my poor english vocabulary, I’m still learning it. Thank you again, Karl!

EDIT: The scritps I shared here are just examples of what is happening. On my real project, my Attributes are detailled with some parametters, and the drawers are very long, this is why I chose to not share them here, because it’s kind of personnal, and also too long and time wasting to read, even if they are fully commented and sorted.

I wouldnt recomened having each of your attributes use a custom property drawer.
There are other ways to approach this to avoid these issues.

For exampe. Have a single Attribute and pass in a flag to control the different type of drawing you want

[Flags]
enum DrawChoices
{
    First = 1,
    Second = 2
    Third = 4
}

public class TestingAttribute : PropertyAttribute
{
    public DrawChoices choices;

    public TestingAttribute(DrawChoices c) => choices = c;
 }

[CustomPropertyDrawer(typeof(TestingAttribute ))]
public class TestingDrawer : PropertyDrawer
{
    public override VisualElement CreatePropertyGUI(SerializedProperty property)
    {
         var attribute = attribute as TestingAttribute ;
         // Draw based on choices
    }
public class FirstTester : MonoBehaviour
{
    [TestingAttribute(DrawChoices.First | DrawChoices.Third]
    public string testString;
}

I thought about that as well yes, but this alternative was making my Attributes way too long because of their parameters amount… This is why I decided to avoid it, in order to keep a clean code, without any line too wide on screen… But thank you for the suggestion tho, I appreciate

You can continue to use attributes. Dont have them all use a custom property drawer. Have a single custom property drawer version that then checks the attributes attached to the field.

This is already what I do yes, but I don’t really know how to avoid the CustomDrawer to only override inside ‘CreatePropertyGUI’ the properties that actually need to be overrieded.

At the moment, if I have 2 of my x Attributes on a variable, ‘CreatePropertyGUI’ will be called 2 times, unless I return either ‘null’, ‘default’, or ‘new()’, which then lead to a ‘No GUI Implemented’.


This is also why I want to avoid having a PropertyField with another PropertyField in its children with another one is the other PropertyField children, and so on.

I think I’m doing it wrong, but I can’t understand where is my issue

At the moment all the attributes use the same interface which has a custom property drawer. Instead have a separate attribute which the custom property drawer use and then checks the other ones.
E.g something like [TestingDrawer], then the property drawer should be for type of(TestingDrawer) only, not the interface.

1 Like

So basically, if I do that, I’ll have to add a sort of ‘indicator’ like ‘[UseThisDrawer]’ to every property I add my Attributes. It’s a good alternative I think yeah, even if it forces to add it every time on each property, which can be quite repetitive and space consuming, but thanks to you it is actually the best alternative I’ve got! I’ll try it soon.

1 Like

Alright, this is still the best alternative I’ve got, I’ll keep on that one for the moment. If anyone got more ideas / suggestions to add, feel free to do it, I’ll keep the post as unresolved since I’m still searching for a fully working solution, but thank you @karl_jones again for your great help! :slight_smile:

1 Like

Hello again, and sorry in advance for the amount of messages already.

I found a second alternative, which can be good in my opinion, but I don’t know if it would consume more performances or not.

First, here is the code:


using System.Linq;
using UnityEditor;
using UnityEngine;
using UnityEngine.UIElements;

[CustomPropertyDrawer(typeof(ITestingAttribute), true)]
public class ITestingDrawer : PropertyDrawer
{
    public override VisualElement CreatePropertyGUI(SerializedProperty property)
    {
        Debug.Log($"Called for the property '{property.name}', with the attribute '{attribute}'.");

        // Tracking every Attribute of the property

        TrackEveryAttribute(property);

        // Creating the container to return

        VisualElement propertyContainer = new()
        {
            name = property.name + "_Container"
        };

        // Creating a TextField manually instead of using PropertyField (if the property is a string)

        TextField textField = new(property.displayName)
        {
            value = property.stringValue,
            name = $"unity-input-{property.name}"
        };

        // Handling updates to the property

        textField.RegisterValueChangedCallback(evt =>
        {
            property.stringValue = evt.newValue;
            property.serializedObject.ApplyModifiedProperties();
        });

        // Adding the TextField to the container and returning it

        propertyContainer.Add(textField);
        return propertyContainer;
    }

    private void TrackEveryAttribute(SerializedProperty property)
    {
        // Get every custom attributes of the property field

        string[] attributes = fieldInfo.GetCustomAttributes(typeof(ITestingAttribute), true).Select(attr => attr.GetType().Name).ToArray();

        // Log each Attribute

        if (attributes.Contains(nameof(IFirstTesting)))
        {
            Debug.Log($"The property '{property.name}' contains the Attribute {nameof(IFirstTesting)}");
        }

        if (attributes.Contains(nameof(ISecondTesting)))
        {
            Debug.Log($"The property '{property.name}' contains the Attribute {nameof(ISecondTesting)}");
        }
    }
}

Basically, not calling this line preciselly:


 PropertyField propertyField = new(property)

Will avoid the CustomPropertyDrawer to be called more than one time, because there is no Binding happening with the new display of the property. Instead, the property is manually redrawn, and a Callback is then added to it.

This lead to the exact solution of what I needed, but it means that I’ve got to code a manually handling method for each type of property (int, string, bool, Vector3, etc etc).

To resume that, here are the pros and cons:

  • pros:
    – No more multiple calling of the CustomPropertyDrawer
    – Reduced amount of UI elements in the Debugger

  • Cons:
    – Way more code to write to handle every type of property (but it’s not bothering that much)
    – A Callback registered on every redrawn property may or may not consume more ram? I don’t know, I need a professionnal to tell it, because I am far from it… ^^’
    – I find this solution kind of ‘dirty’, so I think it may lead to more issues later on, I just didn’t found any yet. Let’s just hope, maybe!

That’s it for now. I’ll still keep the thread as unresolved, because again, it is just a possible viable alternative, and again, I don’t know if it’s better than the previous one in terms of RAM usage or things like that!

Hi. I think karl_jones’ suggestion of using a single dedicated attribute is the best solution. It provides the most re-usability for the drawer. I just want to point out a couple of things.

Unity has a mechanism to avoid infinite recursion in property drawers. I think it was added to UITK drawers in 2021 or 2022. I believe previously it made the nested PropertyField act as if there were no drawers for the property, but it seems now it tries to use the next drawer available. This can be quite useful for using attribute drawers in combination with field type drawers.

So I’d say the reason the drawer is called twice is because there are two attributes that trigger the same property drawer. I don’t think it’s a bug. I think it’s a nice feature.

If you are sure the drawer will only be used with types that work with a TextField, this could be good enough, but you should use the system UITK provides to bind the TextField. Just set it’s bindingPath to the property’s propertyPath.

1 Like

Hi Oscar, thank you for the answer.

The reason I rather use the second alternative I found instead of the previous one from Karl is because:

  • I use more than one custom Attribute on the same properties.
  • The previous solution means to either have one more Attribute that checks for the others, and then draw everything in a single time, or use a composite Attribute.
  • I don’t want to have always n+1 custom Attributes on every property.
  • I don’t Bind the property redrawn because I saw that it doesn’t call the drawer again after it, meaning that I can link multiple Attributes on the same drawer, it will draw everything in a single phase.
  • No, the drawer will not only be used with a TextField only, it was just an example, it will be used for every type of property, even the classes, the structs, and so one. I know how to do it.
  • If the drawer is called twice, then it will add more UI elements in the hierarchy, which I saw in the UI Toolkit Debugger, and found quite annoying because it can lead to a source of stress if for example we had 10 Attributes on the same property. (It’s an exageration, but I use a drastic example to be sure even that can’t happen).

I think I said the main reasons why I switched to the second alternative, but if you got questions, I’d be happy to answer them. Tho, I’m still a bit ‘worried’ about the Callback, I don’t know if it will need a lot of performances to execute on for example 100 properties, or if I’m safe. Anyways, I’ll see that quite soon by experimenting and checking the profiler.

I’m still opened to any suggestion, I’d be glad to use the binding way again instead of the callback one! Take care!

Hi! No problem :).

I’m not sure we are clear on what “Bind” means here. You can bind many kinds of field; it doesn’t need to be a PropertyField. If you bind the TextField to a string property, it will sync the field for you. It will handle Undo, Redo, prefab overrides, animation, preset exclusions and changes from code without any more work on your part. It won’t trigger a Property Drawer. It’s pretty nice. The link I posted explains this with more detail.

To be frank, I think your current approach will be quite hard to implement with support for every type of property. There are too many types of property to support.

Is one more attribute really that bad? Personally, I would prefer that. I think it would actually make things clearer. It wouldn’t even need to have parameters. That said, have you considered using optional parameters for your attribute? C# has a feature where you declare a public property in an attribute, and then you can use it as an optional parameter. The classic example is this:

    class MyAttribute : Attribute
    {
        public MyAttribute(string positionalString)
        {
            PositionalString = positionalString;
        }

        public string PositionalString { get; }

        public int OptionalInt { get; set; }
    }
    //Then you use it like this:
    [MyAttribute("string", OptionalInt = 37)]

That way you only have one attribute, with multiple optional parameterization opportunities.

1 Like

Hello again,

I did a mistake while writing that calling the Bind method would lead to a multiple call of the drawer every time. In fact, it does it only if it is called with a PropertyField, but not for example with a TextField, which if in fact a better option to handle Undo, Redo, etc, instead of a Callback.

I know taht there is a ton of types, this is why I’m looking at their ‘sub fields’. For example, I won’t handle a Vector2, but I do for a Single (float), and a Vector2 got 2 child Single. I am already looking for every sub property/field inside each unsupported type by my drawer, which reduce the amount of type to really handle.

Is one more attribute really that bad?

It bothers me, even if it’s a single line ahah, I don’t know how to really explain it, but when I know that there is a way to avoid it, I’ll try to do so. For now, the alternative I’ve got is in fact a way to avoid it, but it leads to way more code, which is also bothering, but less (for me at least, I enjoy coding random things, even if they are slow, as long as I learn, I’m happy).

And yeah, I already use some parameters in every Attribute I’ve got, for the property Label size, display, flexGrow, as well as for the field itself, etc. I’m basically adding as parameters most of the UI Toolkit main settings, to fully customize the diplay like in a CustomEditor.

My attributes themselves already got a certain amount of parameters, which would be way to wide on a single line for a single attribute to handle them all, and not really aesthetic if I have to use multiple lines for it. I’m trying to keep things visually pleasant and simple, even if it leads to a way more complex code behind it. It’s part of the game I chose to play! :grin:

It still sounds hard to me. Many composite types need their own special behavior. Even if you decide you don’t want to support custom Property Drawers, there’s still lists, UnityEvents, AnimationCurves, Colors, Gradients, etc. There are also all kinds of enums. Even Vectors will look better and be easier to use if you use fields made for them.

Haha. Wouldn’t you rather use your attention to details superpower for something that takes you further? It doesn’t even need to be an extra line if you use a short enough name. I’ll put it this way:

You acknowledge that this will take you time and it would add a lot of complexity to your code, but that’s not the only thing you’re losing. If you add this extra attribute, you’ll open your UI to a lot more possibilities. You’ll be able to combine your drawer attribute with drawer attribute’s from Unity like Range. You’ll be able to support all kinds of Custom Drawers that exist now and in the future, even ones from external plugins. You won’t need to add a single line of code to support all this. And if you want to add more features to your Editor code, it’ll be a lot easier because it’s a lot less complex.

Life isn’t that long, friend. But hey, if you have fun doing this, I’m happy for you.

1 Like

I got to admit that you have a great point here, I think I’ll consider both of the solutions proposed, prioritising yours and learning on the way then! :wink:

I don’t know if I should put the thread as resolved tho, I still wonder why a lot of UI elements are stacking inside the Debugger the more drawer calls we got, but I think the best thing to do is to ask this question in a new thread if I really want to know the answer within the next days/weeks.

Thanks again :slight_smile:

1 Like

An alternate solution would be to have the same property drawer for all of your attributes, and then do some kind of Dictionary<SerializedProperty, VisualElement> to store the drawers for properties in. Then you only create new VisualElements if they haven’t already been created.

You’d probably have to do some hacky stuff and return null or an empty visual element or whatever from the calls where you’re not creating the VisualElement.

As an alternative - for stuff like this where you want a different paradigm for what kinds of things using PropertyAttributes in a script does (like eg. Odin), you might want to define a custom editor for all UnityEngine.Objects (like Odin), and handle drawing all fields yourself.

1 Like