Zinnia.Unity icon indicating copy to clipboard operation
Zinnia.Unity copied to clipboard

feat(Data): UnityEvents with reorderable list of listeners

Open JGroxz opened this issue 5 years ago • 5 comments

Implementation of UnityEvents with reorderable lists of listeners for Zinnia.

Replaced CollapsibleUnityEventDrawer from “Zinnia/Editor/Data/Type” with new ReorderableUnityEventDrawer (which is also collapsible). The code is heavily based on EasyEventEditor repo. Although, I have restructured it and documented most of the stuff.

Features which migrated from above-mentioned EasyEventEditor project:

  • UnityEvent listeners can now be reordered by dragging.
  • Listeners can now be edited using standard keyboard shortcuts (cut, copy, paste, delete). One can also copy listeners between different UnityEvents.
  • (*) There is an option to display an “Invoke” button on the event in Inspector to invoke it manually.
  • (*) All event drawing options can be managed in “Project Settings/Zinnia/Reorderable Unity Events”. Also unnecessary, but good for debugging for now.

Features marked with (*) are legacy from EasyEventEditor, although I personally think they may not work well for Zinnia architecture and should be stripped out.

NOTE: If this implementation proves itself to be useful, I would like to add the same functionality to array and list fields in Zinnia (reordering + keyboard shortcuts).

JGroxz avatar Aug 25 '20 16:08 JGroxz

i think the main issue with reordering unity events is the order they appear is in no way a guarantee of the order they will execute in, so it's potentially misleading

https://forum.unity.com/threads/the-order-in-which-unityevents-are-invoked.286496/

thestonefox avatar Aug 25 '20 16:08 thestonefox

Got the point, though still suggest testing it. It may turn out that UnityEvents initialize on game start and don't update execution order if it's changed in Play Mode. If this is the case, we can just restrict reordering in Play Mode while safely allowing this convenience in Edit Mode. Do you have specific requirements on how should I test it? UPD: also found these two threads. It really seems the order is undefined. That's sad.

On the other hand, if order actually is undefined and reordering events does not make sense, I would suggest leaving copy/paste/duplicate shortcuts functionality. It makes the workflow of setting up the events faster, without having to click the +/- buttons and rewiring each field manually. For example, there are situations where one has to add similar listeners with different arguments - these cases would get workflow speed boost. Thoughts?

Finally, what do you think of reorderable arrays? It is good for ObservableLists, especially in MomentProcessors - as they actually call Process() on list elements in defined order.

JGroxz avatar Aug 25 '20 16:08 JGroxz

maybe @bddckr can remember more about the unity events order thing (if he has time). but i'm pretty sure Unity don't allow reordering of events is because there's no guarantee what order they will be done in.

That being said though, having them reorderable doesn't make things worse per-say if there is no concrete order anyway.

I think re-orderable lists may be a good idea but i think only at edit time, i think it may cause issues at runtime.

thestonefox avatar Aug 25 '20 18:08 thestonefox

maybe @bddckr can remember more about the unity events order thing (if he has time). but i'm pretty sure Unity don't allow reordering of events is because there's no guarantee what order they will be done in.

I only remember that one, too - no guarantees on order by them, which is a dumb thing to begin with. Almost nothing in game dev is order-independent 🙄

I think re-orderable lists may be a good idea but i think only at edit time, i think it may cause issues at runtime.

As a user that just changed the order of some collection elements I would expect that re-ordering operation to be picked up and reacted to in any codepath that depends on the order. I think we would get that for free as you are ultimately reassigning the whole array probably, but even then that will be seen as a huge "remove everything and re-add new things" operation, which will be reacted to automatically then (as we made sure that our setters get run etc.), but it's probably kicking off a lot of edge case scenarios that are unhandled. However, this sounds like it's on the specific implementation that uses a collection to handle it, and goes back to the same point you just raised:

That being said though, having them reorderable doesn't make things worse per-say if there is no concrete order anyway.

I.e. one could argue the user can just do it, they will learn what does and doesn't work and can raise PRs to improve specific implementations where it's adding enough friction for them.

bddckr avatar Aug 26 '20 05:08 bddckr

So, shall I implement the arrays and lists to be reorderable, too?

JGroxz avatar Aug 27 '20 19:08 JGroxz