Show/Hide/Enable/Disable ifs conditions are behaving strangely

Issue #656 resolved
Jakub Holík created an issue

Hi there,

I have a small system that allows user to create some data from a list of options. It consists of two relevant classes: CreateDataMenuVM<T> and DataTemplateSelection<T> as well as couple others that are most probably not relevant.

CreateDataMenuVM is used by an object, that is awaiting new data objects. It is always instantiated uniquely for each using object. It allows user to start or cancel the creation process. Upon start it should enable a DataTemplateSelection and wait for something to be selected. This enabling is handled by Odin’s EnableIfAttribute, that is hooked to a boolean property that flags whether creation process is in progress.

DataTemplateSelection contains a collection of options and once creation is started, it awaits user selection of one of these to pass it to CreateDataMenuVM. It can be shared between multiple CreateDataMenuVMs.

Now the issues is, that the EnableIf seems to be only reacting to only one of many instances of CreateDataMenuVM. Furthermore it then shows all instances of DataTemplateSelection instead of just the single on owned.

To illustrate it better:

And the code for CreateDataMenuVM and DataTemplateSelection looks as follows:

public class CreateDataMenuVM<TAbstractData> : ICreateDataVMMenu<TAbstractData> where TAbstractData : ICatalogable
    {        
        [SerializeField, EnableIf(nameof(IsCreationInProgress)), PropertyOrder(1), HideLabel, HideReferenceObjectPicker, HideDuplicateReferenceBox]
        private readonly IDataTemplateSelection<TAbstractData> dataTemplateSelection;
        private readonly IDataVMCreator<TAbstractData> dataVMCreator;

        private CancellationTokenSource ctSource;

        public event Action<IDataVM<TAbstractData>> DataCreated;

        protected CreateDataMenuVM(
            IDataTemplateSelection<TAbstractData> dataTemplateSelection,
            IDataVMCreator<TAbstractData> dataVMCreator)
        {
            this.dataTemplateSelection = dataTemplateSelection;
            this.dataVMCreator = dataVMCreator;
        }

        public bool IsCreationInProgress { get; private set; }

        [ShowInInspector]
        public int CreateDataMenuVMHash => GetHashCode();

        [ButtonGroup, Button("Create"), DisableIf(nameof(IsCreationInProgress)), Indent(5)]
        public async void CreateAndAddNewDataAsync()
        {
            using (ctSource = new CancellationTokenSource())
            {
                IsCreationInProgress = true;
                Debug.Log($"Starting creation on {GetHashCode()}");
                try
                {
                    var data = await dataTemplateSelection.SelectDataTemplate(ctSource.Token);

                    CancelCreation();

                    var dataVM = dataVMCreator.Create(data);

                    DataCreated?.Invoke(dataVM);
                }
                catch (OperationCanceledException)
                {
                    Debug.Log($"Creation of {typeof(TAbstractData).Name} cancelled by user.");
                }
                finally
                {
                    IsCreationInProgress = false;
                }
            }
        }

        [ButtonGroup, Button("Cancel"), EnableIf(nameof(IsCreationInProgress))]
        public void CancelCreation()
        {
            try
            { 
                ctSource?.Cancel();
            }
            catch (ObjectDisposedException) { }
        }
    }

    public class DataTemplateSelection<TAbstractData> : IDataTemplateSelection<TAbstractData> where TAbstractData : ICatalogable
    {
        [SerializeField, ListDrawerSettings(Expanded = true, IsReadOnly = false), InlineProperty, HideReferenceObjectPicker]
        private readonly List<IDataSelector<TAbstractData>> dataSelectors = new List<IDataSelector<TAbstractData>>();

        private IEnumerable<IDataValidator<TAbstractData>> dataValidators;

        protected DataTemplateSelection(
            IEnumerable<TAbstractData> dataTemplates,
            IEnumerable<IDataValidator<TAbstractData>> dataValidators)
        {
            if (dataTemplates is null)
            {
                throw new ArgumentNullException(nameof(dataTemplates));
            }

            this.dataValidators = dataValidators
                ?? throw new ArgumentNullException(nameof(dataValidators));

            dataSelectors.AddRange(dataTemplates
                .Where(template => dataValidators.All(validator => validator.Validate(template)))
                .Select(x => new DataSelector<TAbstractData>(x)));
        }

        public IEnumerable<IDataSelector<TAbstractData>> DataSelectors => dataSelectors;

        public async Task<TAbstractData> SelectDataTemplate(CancellationToken ct)
        {
            if (ct.IsCancellationRequested)
            {
                ct.ThrowIfCancellationRequested();
            }

            var dataTasks = DataSelectors.Select(x => x.DataSelectedAsync(ct));

            if (!dataTasks.Any())
            {
                Debug.LogError("CreateDataMenu has no items to create.");
                throw new OperationCanceledException();
            }

            var result = await Task.WhenAny(dataTasks);

            if (ct.IsCancellationRequested)
            {
                ct.ThrowIfCancellationRequested();
            }

            return result.Result;
        }
    }

Problematic member is the very first one of the first class (private readonly IDataTemplateSelection<TAbstractData> dataTemplateSelection;) The EnableIfAttribute acts weirdly as shown above.

These classes are actually being inherited from for different data types that can be created. It is not strictly necessary, but it makes things more readable. The inherited types have no overrides nor any extra members.

My environment looks like this:

Odin: 2.1.8

Unity: 2019.3.7f1

OS: Windows 10

Editor Only mode: disabled

Any help will be appreciated.

Best regards

Knedlo

Comments (4)

  1. Tor Esa Vestergaard

    As I understand it, there is a single value (DataSelector) that is being shared between many different creators that use the selector to select some data to create, or something like that. If I have it right, then this is a bit of a tricky issue, truly - you see, this is somewhat expected behaviour. The data selector is genuinely the same property being drawn several times over in the same frame. Any time after the first time the data selector is “seen”, the remainder of the references to it become just dead-end reference properties that point to the path of the first reference, and the drawer just fetches that first reference and then draws it again. Odin does things this way to protect itself against infinite recursion loops and other cases where it would be very bad to generate new properties for the same values seen in different places; there is never more than a single property for any given reference. Doing things this way is bothersome for rare cases like you have here, but meanwhile it solves a lot of other problems that it would otherwise be extremely difficult and tricky to deal with.

    As such, your [EnableIf] will “globally” act as though it is placed only on the “first reference”, because it happens “down in the shared property”, not “up in the owning class”, despite what it might look like. One solution could be to place your data selector in a unique wrapper instance, or struct, for each thing that holds a reference to it, and you can enable/disable that wrapper, instead of the property that is “shared”. This should allow you to toggle the enabled status independently on each wrapper, while still keeping the reference. Hopefully that tip is helpful.

  2. Jakub Holík reporter

    Hi Tor,

    thanks for your answer and yes, you got it right. I was afraid this might be the case. Wrapping it sounds like an option, but I am not too keen on changing the architecture (even little), just to enable nice to have behaviour of Odin. I’m using it mostly as an effortless way to create temporary ui for viewmodels. I might reconsider, but for now, I guess I’ll just live with that and once I’ll have proper ui this won’t be a problem any more.

    Thanks again.

    K.

  3. Log in to comment