leapmotion / LeapMotionCoreAssets

Unity Assets for Leap Motion v2 Skeletal Tracking
Apache License 2.0
166 stars 81 forks source link

Execution Order attributes #119

Closed Amarcolina closed 9 years ago

Amarcolina commented 9 years ago

This PR adds the ExecuteAfter and ExecuteBefore attributes, as well as a solver which applies these attributes to Unity's Script Execution Order. This solver is invoked automatically upon each script reload, and so there should be no effort on the developers side (apart from adding the attribute to a class).

The ExecuteAfter attribute enforces that the tagged behavior executes after another behavior. This affects the order in which many unity callbacks, such as Awake / Start / Update / LateUpdate are called during each frame. The ExecuteBefore attribute works in the same way.

@RandomOutput @GabrielHare @yuwilbur Testing is a bit annoying, as it requires the manual creation of new scripts and placing attributes onto them to inspect the behavior.

For each case verify that the ScriptExecutionOrder tab shows an order that satisfies the attributes. Valid orderings

Invalid orderings. Verify that an error dialog pops up and it reports the correct cycle.

NOTE: I have not deleted EnableUpdateOrdering nor have I used the Execute attributes on any existing class. I think that should be done in a later PR.

GabrielHare commented 9 years ago

This looks great! If I have a chance I’ll merge this and then start clearing out the EnableUpdateOrdering use.

On 14 Aug, 2015, at 11:11 AM, Amarcolina notifications@github.com wrote:

This PR adds the ExecuteAfter and ExecuteBefore attributes, as well as a solver which applies these attributes to Unity's Script Execution Order. This solver is invoked automatically upon each script reload, and so there should be no effort on the developers side (apart from adding the attribute to a class).

The ExecuteAfter attribute enforces that the tagged behavior executes after another behavior. This affects the order in which many unity callbacks, such as Awake / Start / Update / LateUpdate are called during each frame. The ExecuteBefore attribute works in the same way.

@RandomOutput https://github.com/RandomOutput @GabrielHare https://github.com/GabrielHare @yuwilbur https://github.com/yuwilbur Testing is a bit annoying, as it requires the manual creation of new scripts and placing attributes onto them to inspect the behavior.

For each case verify that the ScriptExecutionOrder tab shows an order that satisfies the attributes. Valid orderings

Script1 executes before Script2 Script1 executes after Script2 Script1 executes before Script2 executes before Script3 executes before Script4 Script1 executes after Script2 executes after Script3 executes after Script4 Script1 executes before Script2. Script3 executes after Script4. Script4 executes before Script1. Invalid orderings. Verify that an error dialog pops up and it reports the correct cycle.

Script1 executes before Script1 Script1 executes before Script2 executes before Script1 Script1 executes before Script2. Script1 executes after Script2. Script1 executes after Script2 executes after Script3 executes after Script4 executes after Script1. NOTE: I have not deleted EnableUpdateOrdering nor have I used the Execute attributes on any existing class. I think that should be done in a later PR.

You can view, comment on, or merge this pull request online at:

https://github.com/leapmotion-examples/LeapMotionCoreAssets/pull/119 https://github.com/leapmotion-examples/LeapMotionCoreAssets/pull/119 Commit Summary

Importing execution order util from home Cleaned up some commends on the Node class Added additional comments Improved the cycle error message Added large doc at the top, added Editor defines Added logic for the event system Improved cycle reporting File Changes

A Assets/LeapMotion/Scripts/Utils/ExecutionOrder.cs https://github.com/leapmotion-examples/LeapMotionCoreAssets/pull/119/files#diff-0 (552) A Assets/LeapMotion/Scripts/Utils/ExecutionOrder.cs.meta https://github.com/leapmotion-examples/LeapMotionCoreAssets/pull/119/files#diff-1 (12) Patch Links:

https://github.com/leapmotion-examples/LeapMotionCoreAssets/pull/119.patch https://github.com/leapmotion-examples/LeapMotionCoreAssets/pull/119.patch https://github.com/leapmotion-examples/LeapMotionCoreAssets/pull/119.diff https://github.com/leapmotion-examples/LeapMotionCoreAssets/pull/119.diff — Reply to this email directly or view it on GitHub https://github.com/leapmotion-examples/LeapMotionCoreAssets/pull/119.

GabrielHare commented 9 years ago

Coped branch locally, and will begin testing...

GabrielHare commented 9 years ago

Execute before self is caught. Execute after self is caught.

GabrielHare commented 9 years ago

Adding a node with inconsistent order relative to a valid order is caught. Example: A before B B after A then add C with C before A and C after B

GabrielHare commented 9 years ago

Multi-stage loop was caught. However, the error message did not describe the loop correctly.

The problem is that the findCycle method includes to path leading TO the cycle. The solution is to drop all visited nodes preceding on the recurring one.

My guess for a solution would be something like the code below... but I am not sure how to compare nodes.

private static bool findCycle(Dictionary<Node, List> edges, Node visitingNode, Stack visitedNodes) { if (visitedNodes.Contains(visitingNode)) { // INSERTED CYCLE TRIM HERE: visitingNode = visitinNode.SkipWhile(node => node != visitingNode); return true; } ...

Amarcolina commented 9 years ago

Found another issue, it is actually not possible to set the execution order of built-in behaviours such as UnityEngine.UI.Button, but this method will occasionally (depending on requirements) try to change their execution index, resulting in a warning and no effect. Working on a fix right now.

Amarcolina commented 9 years ago

The script has been updated, there were two issues that existed:

The following changes have been made:

GabrielHare commented 9 years ago

Really good changes. There is just one more issue that I found. To reproduce it update to the force-pushed head of test-execution-order-attributes and open the TESTExecutionOrder.unity scene.

(1) Apply the execution ordering. (2) Play, and examine the "Call Order" fields of the TextNode objects to see that the call ordering is correct. (3) Open the "Script Execution Ordering" panel and break the ordering. (There is only one acceptable ordering - try moving BridgeFrom12To21 above Side1Node2) then run. (4) You will get a message stating "Unapplied Execution Order" and can choose "Revert" or "Apply". (5) If you choose "Revert" everything works. (6) If you choose "Accept" then there will be warnings about a missing behavior and the call ordering can be seen to be incorrect. My guess is that an attempt to resolve the call ordering has hung.

Amarcolina commented 9 years ago

I do not see any warnings following the same steps. After pressing 'Accept' the scene begins as normal, and a recompile occurs to apply the execution order. Once the recompile finishes, the ExecutionOrder callback is triggered and I see the second recompile, after which I see the execution order become correct in the execution order window.

I do notice that the call-order for all of the nodes is set to zero, but I think that has more to do with a recompile happening during play mode, triggering a hot-reload. The hot-reload fails to serialize the isCalled delegate, so all of the nodes have their onUpdate set to null, resulting in the callOrder to be set to zero on the next Update cycle.

What I did was simply have each OrderedNode Debug.Log(this) in the Update method. Then I pressed the collapse button on the console. The order in which the logs are output is the same as the order they are executed, so it makes for a nice and easy way to verify the callback order. (You just have to clear the console every time you want to check). Using this method, I verified that after the second recompile finished, the scripts executed in the correct order.