ValueDropdownLists call IValueDropdownItem.GetValue() far more often than they should

Issue #418 open
Dominic Antonelli created an issue

I'm trying to use my own class that implements IValueDropdownItem to create a dropdown that calls a delegate when an item is selected (and returns the result of the delegate). However, the value dropdowns seem to call IValueDropdownItem.GetValue() many many times, instead of only when the item is actually selected.

In fact, it seems to call IValueDropdownItem.GetValue() even when the dropdown hasn't been opened yet.

To reproduce, toss the following script onto an empty, and notice that every time you select the GameObject in the hierarachy, you'll see the debug line in the Console. You'll also see 3(!) calls whenever you open the dropdown.

using System;
using System.Collections.Generic;
using UnityEngine;

using Sirenix.OdinInspector;

public struct DelegateValueDropdownItem<T> : IValueDropdownItem
{
    private string m_text;
    private Func<T> m_delegate;

    public DelegateValueDropdownItem(string text, Func<T> p_delegate)
    {
        m_text = text;
        m_delegate = p_delegate;
    }

    public string GetText()
    {
        return m_text;
    }

    public object GetValue()
    {
        return m_delegate.Invoke();
    }
}

public class OdinDebug : BaseBehaviour
{
    [ValueDropdown("GetObjects")]
    public object Test;

    protected IEnumerable<DelegateValueDropdownItem<object>> GetObjects()
    {
        return new List<DelegateValueDropdownItem<object>>()
        {
            new DelegateValueDropdownItem<object>("Test", () => { Debug.Log("IValueDropdownItem.GetValue() called!"); return null; }),
        };
    }
}

Comments (6)

  1. Bjarke Elias
    • changed status to open

    Thanks!

    I see that the GetObejcts method is unnecessarily called twice when you click on it. I'll see if I can do something about that.

    However, we do need to call the GetObjects() method when the is first initialized in order to find the name to put in the value drop-down.

    If you use yield return in your GetObjects method, instead of returning a list, then it'll stop as soon as it finds the currently selected object.

  2. Dominic Antonelli reporter

    I understand that it needs to call GetObjects() to put the text into the list, but my GetObjects passes a delegate as the second parameter. It's that delegate that I'm having issues with. It should only be called (via GetValue()) when the actual value of the dropdown item is needed - namely only when the item is selected. In all other cases, only GetText() should be called in order to find the name to put in the drop-down.

  3. Bjarke Elias

    But in order to to get the name of the current value, we need to iterate over all ValueDropdownItems and compare the current value with the value (GetValue) until we find a match, and then take the name from that ValueDropdown item. That's the general case at least. But I can see why that is inconvenient.

    We could perhaps add an Equals / CompareObjects method to the IValueDropdownItem interface, and that way avoid calling GetValue()?

  4. Dominic Antonelli reporter

    Hmm, I'm not sure that would work, unless the "current value" that you're storing is the ValueDropdownItem, not the value itself. If it's just the value, I'm not sure how I would compare it with the DelegateValueDropdownItems in the list without calling GetValue() myself.

  5. Dominic Antonelli reporter

    At the end of the day, the only thing I'm really doing this for is so I can have a "Create New Object" item in my dropdown that will instantiate a new GameObject when selected and set that new GameObject as the value. The "Create New" item is never actually "selected", because as soon as you do "select" it, it creates a new object in the scene, sets that as the value, and now that will be the selected value.

    So maybe there's a simpler way of doing this that would work. For now, I'm just using a workaround of a new DropdownItem<MyObjectType>("Create New", null) and in my OnValueChanged function, I go through the list and replace nulls with new instances. It feels really hacky, but it works for now.

    And it's only really an issue because you can't just create new GameObjects whenever the heck you feel like since they end up in the hierarchy. With types that do not inherit from GameObject (or ScriptableObject too, maybe?), there is no real issue.

  6. Bjarke Elias

    We could also just add support for exactly what you're trying to do here and let the user add funcs to the value, which we then on our end detect and invoke when selected and in those cases just accept the fact that we can't detect a "selected value".

    I've added this to my todo, and will give an update when it's implemented :)

  7. Log in to comment