Verification Exception. Help D:

I've never seen this error before. But then, i am trying to do something i've never done before.

Code (CSharp):

1. VerificationException: Error verifying Wolf/<Action1>c__Iterator3:MoveNext (): Cannot call a non-final virtual method from an objet diferent thant the this pointer at 0x0028
2. UnityEngine.MonoBehaviour:StartCoroutine(String)
3. CreaturePlayerControl:ActionButtonInput(Int32) (at Assets/Scripts/Core/CreaturePlayerControl.cs:80)
4. InputManager:Update() (at Assets/Scripts/Core/InputManager.cs:69)
5.  

Some context: I'm playing around with inheritance. I have a base class, Creature: http://www.pasteall.org/63712/c

And then i have another class, Wolf, which inherits from creature. It's pretty simple, i don't need to post it here.
The part that's causing trouble is this:

Code (CSharp):

1. public override IEnumerator Action1()
2.     {
3.         base.GenericAction(1);
4.         yield return null;
5.     }

I'm overriding a coroutine in the base class (Action1)
And from the overridden version, i'm calling another function in the base class, GenericAction

This is of course, completely redundant in its current form. But this sytem allows me to customise how things work on a per-creature-type basis, so it has merit.

Anyways, why does that code snippet throw the above error? Is it related to anything in my code or something else? I notice a lot of other scripts being named that aren't directly related to this.

 

Not sure if it's the cause, but your virtual Action# coroutines are odd. They just start another coroutine without yielding it and then return null. I'm not sure what Unity does with a coroutine that returns a null IEnumerator.

 

It might be worth a try. You can try either:

Code (CSharp):

1. public override IEnumerator Action1()
2.     {
3.         StartCoroutine( base.GenericAction(1))
4.         yield return null;
5.     }
6.  
7. //OR
8.  
9. public override IEnumerator Action1()
10.     {
11.         base.Action1();
12.         yield return null;
13.     }

Your base.GenericAction(1) returns IEnumerator, but you didn't call StartCoroutine(). Other than that, I'm sorry, never encountered the error before.

 

I've never liked doing nested Coroutines like that anyway. I know they're supposed to work but I always feel like I end up with weird behavior. I would try just using the IEnumerator directly.

Code (csharp):

1.  
2. IEnumerator Action1()
3. {
4.     IEnumerator b = base.GenericAction(1);
5.     while (b.MoveNext()) yield return b.Current;
6. }
7.  

 

This
sounded like the right solution, but it doesn't seem to work. i'm not sure if the error is different now:

Code (CSharp):

1. VerificationException: Error verifying Wolf/<Action1>c__Iterator3:MoveNext (): Cannot call a non-final virtual method from an objet diferent thant the this pointer at 0x002e
2. UnityEngine.MonoBehaviour:StartCoroutine(String)
3. CreaturePlayerControl:ActionButtonInput(Int32) (at Assets/Scripts/Core/CreaturePlayerControl.cs:80)
4. InputManager:Update() (at Assets/Scripts/Core/InputManager.cs:69)

 

What is this? I don't even understand what you're doing here, and it looks like a pain to remember. I was trying to simplify this system

I never really thought about the fact that i'm nesting coroutines, that does sound messy. But i was kind of thinking that it might copy the code of the called function inline, rather than invoking it. I was doing that manually before

Also, calling base.Action1 won't work because i'm planning to convert those to calling the same genericAction coroutine once i get this working. My whole point in writing GenericAction was to not have the same code duplicated eight times in the creature class.

 

Manually enumerating the IEnumerator directly in a single coroutine instead of letting Unity try to do it in multiple, nested coroutines.

I'd again point out that your base virtual methods are returning null IEnumerators and I don't know what Unity does with that.

 

It seems other people had the same problem:
http://forum.unity3d.com/threads/verificationexception.60589/

More insight might be found here on a reddit post:
https://www.reddit.com/r/Unity3D/comments/23l2nd/overridden_ienumerator_methods_and_webplayer/

 

hmm, this is a bit of a complex problem

I should mention, i don't actually know what an IEnumerator is. I know these as coroutines from the unity tutorials, and i was taught that "yield return null" is the way to yield control. I wasn't aware it actually returned something, or that that could cause problems

Is there any other way to do coroutines that won't have those issues?

 

trying to read through this, some of it is a little advanced for me. The main solutions seem to be either unselecting webplayer as a compile target (not possible, my project is specifically only FOR web) and helper functions, which i don't quite understand

It occurs to me that i don't actually need GenericAction to be virtual and thus overrideable. I'm sure if i ever need to create a class specific version of that, i can recreate it as a local function. but generally any alternative functionality i need is done before or after the call to that.
might making it non-virtual fix this issue?

(although i am still overriding a coroutine in the base class (Action1)), so maybe not?

 

Your virtual methods are using return null not yield return null so you're giving the caller back a null IEnumerator, not waiting one frame.

So - the way coroutines work when you call StartCoroutine is that they execute the IEnumerator method until it encounters some kind of yield instruction. It then saves that point of execution and returns to the caller. Where it can get weird is in nested coroutines.

Code (csharp):

1.  
2. void Start()
3. {
4.     StartCoroutine(One());
5. }
6.  
7. IEnumerator One()
8. {
9.     Debug.Log("1");
10.     StartCoroutine(Two());
11.     Debug.Log("3");
12.     yield return null;
13. }
14.  
15. IEnumerator Two()
16. {
17.     Debug.Log("2");
18.     yield return null;
19.     Debug.Log("4");
20. }
21.  

This will give you
1
2
3
4

But if you change One() to

Code (csharp):

1.  
2. IEnumerator One()
3. {
4.     Debug.Log("1");
5.     yield return StartCoroutine(Two());
6. }
7.  

You'll get
1
2
4
3

In the first example you actually have two "concurrent" coroutines running despite the fact that one called the other. This makes it very easy to run into odd situations where a stopped coroutine will keep executing code from its caller.

EDIT:
Reading the other thread - it would appear that this is an issue with inherited methods being used as Coroutines on web platform.

Double Edit:
I would be curious if yielding the IEnumerator directly via my previous post would solve the issue.

 

This special return type causes the compiler to build a state machine around your code so that you can execute some code, return a value, then resume at the same position in code at a later time. It's the perfect tool for a coroutine, so Unity has taken advantage.

I've had to read between the lines for this next bit -- my reasoning could be off

The issue is that you are calling a base virtual function from within the state machine. I believe the state machine makes use of "anonymous" functions which don't belong to a real class. When the code gets wrapped up in these methods, the type safety mechanism built into C# isn't able to determine that "base" is going to refer to a static class (it could refer to an abstract class).

Normally, this isn't an issue but apparently when running in a PartialTrust environment, all calls from anonymous methods must be type safe. Turns out the web player falls into this category.

Personally, I think this is a limitation of the compiler because it should be possible to implicitly trace back to the original class that was compiled into an enumerator state machine. Anyways, the solution is to show the compiler how to trace back explicitly using a "helper method". BTW, a helper method is just a regular method that "helps" you do something..

Code (csharp):

1. public override IEnumerator Action1()
2. {
3.   CallBaseGenericAction(1);
4.   yield return null;
5. }
6. private CallBaseGenericAction(int x)
7. {
8.   base.GenericAction(x);
9. }

 

ahh yes, now i remember. it wouldn't compile because it complained i hadn't returned something, so i just used return null. i assumed that was a safe way to do things. Again, didn't quite know what i was doing there.

As far as i can see, it's only the base Action1...8 functions that have that problem, but Action1 is overridden by the wolf class anyways, so i can't see why it would be relevant. I've fixed it now as you described, and there's no difference, error still occurs

 

Right. I wasn't sure what Unity would do if you told it to start a coroutine with a null IEnumerator. Obviously that isn't the issue and it appears to be a compiler thing.

It's also worth mentioning that those methods don't even need to be IEnumerators because all you're doing is starting another coroutine inside them.

 

Shouldn't you yield the base coroutine?

Code (CSharp):

1. public override IEnumerator Action1()
2.     {
3.         yield return StartCoroutine(base.GenericAction(1));
4.         yield return null;
5.     }

If the intent is "First run the coroutine base.GenericAction(1), and then wait an additional frame".

If you just want StartCoroutine(Action1()) from the subclass to be equivalent to StartCoroutine(base.GenericAction(1)), the correct implementation is:

Code (CSharp):

1. public override IEnumerator Action1()
2.     {
3.         yield return StartCoroutine(base.GenericAction(1));
4.     }

If you're doing the string-based startcoroutine, it might be that this doesn't work - that's based on reflection, and I have no idea where the reflection backend will start looking for the matching IEnumerator.

 

Oops, I was wrong. The second code example doesn't need to have it's own StartCoroutine call. It just needs to return the base's IEnumerator, since StartCoroutine is called on it:

Code (CSharp):

1. public override IEnumerator Action1()
2.     {
3.         return base.GenericAction(1);
4.     }

You're aware that the web player is about to be very dead, right?

 

won't this end the function there? i want to call GenericAction and then possibly do some more stuff within my new Action1 before ending it

well its for web, generally. unity web will still work right? how do i compile to that.
this project has to go on a website somehow

 

Can i do this?

Code (CSharp):

1. public override IEnumerator Action1()
2.     {
3.         yield return base.GenericAction(1);
4.         MoreCodeHere();
5.     }

i'm feeling a little aimless right now, and not really sure of how to proceed with this problem. i've been offered a hundred solutions and suggestions, and seen many of them withdrawn or changed, it's left me a bit lost. i could really do with more cncrete ideas.
otherwise, ill make a start on testing these various ideas tomorrow and hope for the best

 

Nanako, I tried reproducing the error you are getting based on the reading I had done and the answer I gave. Turns out, my suggestion must be wrong because the code seems to compile and work as (un)expected.

It would be useful if you could provide a simple but complete script that causes the error and explain when the error appears.

This might also start a discussion about how you could restructure the code to avoid the problem entirely. Of course, you know well, those discussions often end up on unhelpful tangents.. Anyways, I'm at a bit of a loss. I can't really make heads or tails of this error without some more context.

 

I was able to reproduce the error by making a more complex coroutine -- namely, one which calls another coroutine and waits for it to complete., which I think is what you were asking about above.

I found that my suggestion of using a helper method to explicitly show the compiler how to navigate back to the base class does in fact work, so i think it should help you too.

For the record, here is my code showing the problem and solution.

Code (csharp):

1. using UnityEngine;
2. using System.Collections;
3. using System;
4.  
5. public class CoroutineTest : MonoBehaviour
6. {
7.     protected virtual IEnumerator GenericAction(int x)
8.     {
9.         while (true)
10.         {
11.             gameObject.transform.Rotate(new Vector3(0, 1), Mathf.PI / 10);
12.             yield return null;
13.         }
14.     }
15.  
16.     protected virtual IEnumerator Action1()
17.     {
18.         yield break;
19.     }
20. }

Code (csharp):

1. using UnityEngine;
2. using System.Collections;
3.  
4. public class DerivedTest : CoroutineTest
5. {
6.     void Start()
7.     {
8.         StartCoroutine(Action1());
9.     }
10.  
11.     protected override IEnumerator Action1()
12.     {
13.         yield return StartCoroutine(base.GenericAction(1)); // call like this to see the error (web player only)
14.         // yield return StartCoroutine(this.CallBaseGeneric(1)); // call like this instead to fix the problem
15.         yield break;
16.     }
17.  
18.     IEnumerator CallBaseGeneric(int x)
19.     {
20.         return base.GenericAction(x);
21.     }
22. }

Basically, add DerivedTest to a gameobject such as a cube. When running in any mode, the object will begin to rotate. However, on a webplayer, the error will occur and the rotation will not start. Swap the two lines indicated in the DerivedTest to make the problem go away.

Of course there are some other solutions... such as not calling a base method at all. Instead, you could make the call to this.GenericAction(1) since DerivedClass inherits this function and does not override it. This avoids the call to a virtual base method from an anonymous method and works fine in the Web Player.

I do think the code to get into this position is quite odd, so, if you're up to the conversation, I encourage you to post your code and I'm sure someone will have ideas on how it could be tidied up a bit.

 

I'll check when I get to work, but I think you'll need to yield a StartCoroutine call to have the inner coroutine be finished first.

EDIT: Nope! Your code will work. Shouldn't be surprising at all, as you don't have to StartCoroutine a WaitForSeconds. Not sure what I was thinking.

I haven't done it myself (pc/console developer), but the idea is to compile to WebGL. Of course, WebGL is a very young technology, and apparently still not as fast as the old webplayer. The big upside is that your users don't have to download a plugin, and you can run on Google Chrome, which is the first of many to stop supporting plugins.

To read about why the Web Player is getting killed off, read the first paragraph here.

 

I'm not clear on why that fixes it.
The thing you're calling, which calls genericAction is also a coroutine, so that's nesting THREE levels of coroutines instead of two, i'm surprised that doesn't make the problem worse.

Is it because your helper function isn't virtual? In that case, could my original code work if i made GenericAction also non virtual? Making it that way was kind of an oversight in the first place, and i've realised i'll never need

And i'm kicking myself now. IT seems i completely forgot how inheritance works, and that the inherited class has it's own perfectly functional copy of GenericAction. There was no reason in the first place to call base.anything

Sure, although i've only made one tiny chance since posting Creature.cs in the OP
http://www.pasteall.org/63738/c Creature.cs
http://www.pasteall.org/63739/c Wolf.cs, which inherits from creature. Very simple at the moment, all it really does is set some config values. But i'm planning to make some unique behaviour for it's overridden Action2 once i get this problem sorted out

 

To be perfectly honest, I'm not 100% clear on it either. This is quite an arcane little gotcha you've found.

From what I can tell, the issue is calling a virtual base method from within an anonymous method. It's clear from your code that you are calling a base method that is marked virtual, what's less clear is that you're calling it from an anonymous method. It doesn't look like an anonymous method but because it is an IEnumerator, it is getting wrapped up into one behind the scenes.

Yes. It's also not on the base class, so it could technically be virtual so long as it was this.CallBaseGeneric you are calling instead of base.CallBaseGeneric (which obviously doesn't make any sense, it's just to illustrate a point).

Yes, I believe it would work. As would calling this.GenericAction instead of base.GenericAction.

It is the combination of these four things that got you into trouble:

1. calling a base method
2. calling a virtual method
3. calling from an anonymous method (or in this case, an IEnumerator that got turned into an anonymous method)
4. running in a PartialTrust environment (Web Player)

Yes, that is correct -- I assumed you might have overridden this.GenericAction and wanted to bypass directly to the base version's of GenericAction. This is actually one of the reasons the pattern seems so strange to me.

Just to clear up some of the other posts on this topic, you OP seems to have two independent problems.

Code (CSharp):

1. public override IEnumerator Action1()
2.     {
3.         base.GenericAction(1);
4.         yield return null;
5.     }

First is the base virtual call not working in the Web build, which I've already covered..
Second is the way you are calling your nested coroutine. From what I can tell, there are two legitimate ways to nest coroutines:

1. StartCoroutine(GenericAction(1));
2. yield return StartCoroutine(GenericAction(1));

If you use the first option, the coroutines will actually run in parallel -- the first coroutine will initiate the second and then immediately continue on.
If you use the second option, the coroutines will run in serial -- the first coroutine will initiate the second and then wait for it to complete before continuing on.

This is what KelsoMRK was explaining in his post: http://forum.unity3d.com/threads/verification-exception-help-d.381016/#post-2474638

Simply calling GenericAction(1), without wrapping in StartCoroutine will probably not do what you expect. I'll try to explain..

StartCoroutine can be thought of as a registration call. You must register a coroutine by passing this method an IEnumerator. The IEnumerator is not a coroutine on its own, it is just a state machine that knows how to return an item, yield control of the computer, and then resume from where it left off later. When you call GenericAction(1) without wrapping it with StartCoroutine, you are creating the IEnumerator state machine and then throwing it away without registering it.

The implication is kind of interesting. You could create one of these state machines for tuning into a coroutine and save it in a local variable for use later. If you do this, you can also make use of the strongly typed StopCoroutine method.

Code (CSharp):

1. public override IEnumerator Action1()
2.     {
3.         var myEnumerator = this.GenericAction(1);
4.         StartCoroutine(myEnumerator); // start coroutine immediately
5.         yield return new WaitForSeconds(1); // wait one second
6.         StopCoroutine(myEnumerator); // stop coroutine
7.         yield return WaitForSeconds(1); // wait another second
8.         StartCoroutine(myEnumerator); // start coroutine again _from where it left off_
9.     }

I'm going to take a quick look over your code and see if anything comes to mind.

 

eisenpony you're amazing as always. I swear every time you reply to something, i learn a thousand things.
rereading and digesting this post, i ought to be able to solve things from here <3

 

Wow .. thanks for the kind words. I don't mind at all as I really enjoy teaching and watching others get better.

I finally got a chance to skim over your code. I won't pretend to know exactly what is going on in your script -- there is quite a lot to it.

You logic is obviously very strong, there is a lot of complex branching and state management. However, I think you would benefit from a different perspective. You're smart to put as much common code as possible in a place that can be shared amongst all of your animal types. However, you have fallen into the trap of only using inheritance to share code. There is another way to share code which will allow you to break down your script into bite size pieces. This technique is often called "the Unity Way" on this forum but that is just nonsense -- composition, as it's known to the rest of the programming world, has been around a lot longer than Unity and I'm certain will outlast it.

I've seen others struggle with this idea, I think partially because inheritance provides abstraction by default which is another idolized technique. So let's talk about what composition is for just a moment. Let me point out some places you are using composition already. Under the "references" comment you have local variables for the components: Animator, CharacterController, GameObject, AudioSource, NavMeshAgent. You have referenced these components because you need their functionality to accomplish something. I don't think you would dream of inheriting from Animator just so you can call SetBool or SetTrigger etc.. In fact, you probably spot the trouble right away -- you can't inherit from all of the external components you need to use. You are using composition just by creating a new object that has some useful functionality within it.

You might think these things are bad because they are references to external objects and are causing your code to become coupled. But the truth is that they are breaking your code up. Imagine if you needed to copy and paste the entire contents of Animator, CharacterContorller, etc.. into your scripts whenever you wanted to use them. I think the reason references get a bad rep is because we often end up with too many of them to keep straight.

So, why do we end up with so many? Usually because the code that we are working on is doing too many things. Look at your RecalculateVelocity method. It does not care about CharacterController, or GameObject, or AudioSource, etc.. but it is forced to be coupled with these things because it is a part of the Creature class which has other methods that need those components. If we break your methods into related chunks we may find that one of these chunks has no dependencies on the components that the other chunks have. If we move this chunk into a new class, its methods can drop their knowledge of those components completely.

Based on my very short analysis, I see 5 chunks inside of your Creature class which would make good components:

1. Moving
2. Jumping
3. Actions
4. Recruiting
5. Threatening

I wrote a whole pile more but after rereading it, I found it was too abstract to be of any value.. Instead, I'm going to try to show you how I might refactor your code if I came across it and was asked to add features to it. To be clear, I'm a big proponent of "don't fix what ain't broke" and I do not believe your code is broken. However, when asked to extend features or fix a bug I always start by refactoring the code I will be working on a bit to make it more "bite sized".

The process of refactoring, itself, is not difficult nor does it take a very long time compared to how much it will save down the road. However, explaining as I go will be quite a bit more time consuming, so I won't tackle the whole problem all at once. I may come back and do more refactorings later as time permits.

 

Wrap Up
I wrote this section last but realized it would be helpful to be at the top of the following wall of text, just to give some context before setting off..

Whew.. So that took quite a bit longer to explain than I anticipated. Still it was a fun experiment. I'm hoping I get a chance to come back and talk about how I would refactor the Movable component out of Creature, and after that how I might change some of your code to be more object oriented. Unfortunately, I don't think I can explain the benefits of doing all this work until you see the components separated nicely. Regardless, I hope it was at least a little interesting to see how you can slowly modify existing code towards a new design.

Just a few things I want to emphasize before signing off. First, this refactoring isn't meant to fix any thing. In fact, my only real goal was to not break anything. It is an accumulation of several refactorings that will make this code easier to maintain by breaking it into distinct chunks each with a single focus. Second, It's hard to see the purpose of all this without reaching the end goal. I wouldn't suggest following along too carefully until I get closer to the finished product. There's simply no guarantee I will have time to complete this project and as it is, it's more of an interest work than a practical help. Thirdly, I had to read between the lines and fudge a little bit because I don't have an actual project to test against to a) make sure I didn't break anything and b) to actually get a successful compile.

If you have any questions about how or why I did something, just ask and I'll try to explain.

Intro
So for the first example, let's say I have been asked to introduce a feature to the jumping of a creature. Let's not try to implement a specific feature because I think the refactor will be enough for this post..

First, I'm going to try to tease the jumping code out of Creature and add it as a component.

The Work
To begin, I added Creature to a namespace and created a new set of folders for my creature components. Creature is in the base namespace Assets and I created a Jumpable class in the Assets.CreatureComponents namespace.

I started by looking over your fields and finding items that are probably related. I found these:

Code (csharp):

1. public float baseJumpSpeed = 8f;
2. public float jumpTakeoffDelay = 0.43f;
3. float jumpTakeoffTimer = 0f;
4. private int _jumpState = 0;
5. public int jumpState

I moved these fields out of Creature and into my new class "Jumpable". Obviously this broke some things but let's keep moving..

scanning over the methods, these ones stood out to me:

Code (csharp):

1. public bool isOnGround()
2. { ... }
3. void CreatureGroundDebug(string input)
4. { ... }

There was also quite a bit of jumping related code in the FixedUpdate method. It looked to me that this code was determining if a jump had been scheduled and if so, would take the required actions to make the jump occur. I wanted this code in my Jumpable class, so I created a new method "TryJump" which I could call from Creature.FixedUpdate.

So here is my Jumpable class so far.

Spoiler: Jumpable.cs

Code (csharp):

1. using UnityEngine;
2.  
3. public class Jumpable
4. {
5.     public float baseJumpSpeed = 8f;
6.     public float jumpTakeoffDelay = 0.43f;
7.     float jumpTakeoffTimer = 0f;
8.  
9.  
10.     //Data
11.     private int _jumpState = 0;//0 = not jumping. 1 = Jump queued for a grounded frame 2= Jump started, but takeoff not achieved yet. 3 = liftoff
12.     public int jumpState
13.     {
14.         get
15.         {
16.             return _jumpState;
17.         }
18.         set
19.         {
20.             Debug.Log("Jumpstate being set to " + value);
21.             if (value == 1)
22.             {
23.                 if (_jumpState == 0)
24.                 {
25.                     _jumpState = value;
26.                 }
27.             }
28.             else
29.             {
30.                 _jumpState = value;
31.             }
32.         }
33.     }
34.  
35.     public bool isOnGround()
36.     {
37.         if (jumpState == 2)
38.         {
39.             return false;
40.         }
41.  
42.         RaycastHit hit;
43.         Physics.SphereCast(spherecastStartOffset + transform.position, spherecastRadius, spherecastDirection.normalized, out hit, spherecastDirection.magnitude);
44.  
45.         if (hit.collider != null)
46.         {
47.             if (jumpState != 3)
48.             {
49.                 CreatureGroundDebug("Onground because jumpstate != 3");
50.                 return true;
51.             }
52.             else if (controller.velocity.y <= 0)
53.             {
54.                 CreatureGroundDebug("Onground because yvel <=0");
55.                 return true;
56.             }
57.             else
58.             {
59.                 return false;
60.             }
61.         }
62.         else
63.         {
64.             return false;
65.         }
66.     }
67.  
68.     void CreatureGroundDebug(string input)
69.     {
70.         if (jumpState >= 2) Debug.Log(input);
71.     }
72.  
73.     public void TryJump()
74.     {
75.         if (jumpState == 0)
76.         {
77.             //If we are not jumping, or if a jump has just finished
78.             //controller.SimpleMove(moveDirection*Time.fixedDeltaTime);
79.             //controller.SimpleMove(moveVelocity);
80.             Move(0);
81.         }
82.         else if (jumpState == 1)//Jump state will be set to 1 by the external controller
83.         {
84.             //if (isOnGround == true)
85.             if (isOnGround() == true)
86.             {
87.                 //We start a jump
88.                 Debug.Log("Starting a jump");
89.                 anim.SetTrigger("Jump");
90.                 jumpState = 2;
91.                 jumpTakeoffTimer = 0f;
92.             }
93.             else
94.             {
95.                 Debug.Log("Trying to jump, but we're not grounded");
96.                 //controller.SimpleMove(moveVelocity);
97.                 Move(0);
98.             }
99.         }
100.         else if (jumpState == 2)
101.         {
102.             if (jumpTakeoffTimer >= jumpTakeoffDelay)
103.             {
104.                 flightVelocity.x = controller.velocity.x;
105.                 flightVelocity.z = controller.velocity.z;
106.                 flightVelocity.y = baseJumpSpeed;
107.                 jumpState = 3;
108.             }
109.             else
110.             {
111.                 jumpTakeoffTimer += Time.fixedDeltaTime;
112.                 //controller.SimpleMove(moveVelocity);
113.                 Move(0);
114.             }
115.         }
116.  
117.         if (jumpState == 3)//Once here, then we have lifted off from the ground
118.         {
119.             //controller.Move(flightVelocity * Time.fixedDeltaTime);
120.             Move(1, flightVelocity * Time.fixedDeltaTime);
121.  
122.             if (isOnGround() == false)
123.             {
124.                 //We are flying through the air. Gravity gradually applies more
125.                 flightVelocity.y -= Time.fixedDeltaTime * Mathf.Abs(Physics.gravity.y);
126.             }
127.             else if (controller.velocity.y < 0f)
128.             {
129.                 //We have touched down. jumping is finished/
130.                 Debug.Log("Touchdown vel: " + controller.velocity);
131.                 anim.SetBool("grounded", true);
132.                 jumpState = 0;
133.             }
134.         }
135.     }
136. }

A couple of things I'll draw your attention to. first, this class does not inherit from MonoBhevaiour. I'm not 100% sure this is a good idea, since inheriting would allow us access to some useful functions and would also allow this component to be found using Unity's "FindComponent" command. Anyways, it's something to think about in future..

Second, notice that this script is very broken. There are a lot of things in Creature that this code was taking advantage of and we no longer have access to them from here. That's okay though, there are always going to be dependencies in our code. We just need to make sure the items we need are available as we need them. I'm going to start changing the methods to take parameters to the data and objects it needs.

First of all, let's look at isOnGround(). There are a number of fields in here that I didn't anticipate needing:
spherecastStartOffset
transform
spherecastRadius
spherecastDirection

Now I don't really understand what these guys do, but that's okay. I'm not going to try to clean that up right now. All I care about is getting this data available for my method. So let's move them over from Creature.
Up near the top, I start copying these fields over.

Code (csharp):

1. public float baseJumpSpeed = 8f;
2. public float jumpTakeoffDelay = 0.43f;
3. float jumpTakeoffTimer = 0f;
4. private int _jumpState = 0;
5. public int jumpState
6.  
7. float spherecastRadius;
8. Vector3 spherecastStartOffset;
9. Vector3 spherecastEndOffset;
10. Vector3 spherecastDirection;

Before I move them out of Creature I use the Find All References command to see if the field is still being used in Creature. Turns out, all of these values are used in the Boot() method but this seems to be just for initialization, so I think it is safe to move them over so long as I move the initialization over as well.

Instead of replicating the boot mechanism, I'm going to use a constructor which automatically gets run anytime you "new up" an object. This is the primary way of creating objects outside of Unity and since my Jumpable class is not inheriting MonoBehaviour (for now), it is the only way to create a new object of this type.

Code (csharp):

1. public Jumpable()
2. {
3.   spherecastEndOffset = c.center;
4.   spherecastStartOffset = GetComponent<CharacterController>().center;
5.   spherecastDirection = spherecastEndOffset - spherecastStartOffset;
6.   spherecastRadius = c.radius;
7. }

Not quite good enough, because the initilaizers use the object "c" and the GetComponent command to grab the "CharacterController". Since I don't have the GetComponent command, I will pass both of these objects as paramaters

From Creature.Boot,

Code (csharp):

1. new Jumpable(c, controller);

and in Jumpable

Code (csharp):

1. public Jumpable(SphereCollider c, CharacterController controller)
2. {
3.   spherecastEndOffset = c.center;
4.   spherecastStartOffset = controller.center;
5.   spherecastDirection = spherecastEndOffset - spherecastStartOffset;
6.   spherecastRadius = c.radius;
7. }

I'm not trying really hard to clean things up. My main concern is to componentize the jumping logic, so I'm willing to pass all sorts of objects around for now. In a later refactor, I might take the time to think more carefully about what information I really need here -- maybe I could calculate the direction and radius from Creature and just pass those in, but for now .. let's stay focused.

At this point, isOnGround is looking a lot better. However, there are still some missing pieces. This code relies on transform and controller. Since I'm not a monobhevaiour, I can't access transform directly, so I'll pass it in as a parameter. For the controller object, it seems I only need one tiny piece of information -- the y velocity, so I'll just have this piece of information passed in for now. I'll change my isOnGround method to be like this:

Code (csharp):

1. public bool isOnGround(Transform transform, float yVelocity)
2. {
3.     if (jumpState == 2)
4.     {
5.         return false;
6.     }
7.  
8.     RaycastHit hit;
9.     Physics.SphereCast(spherecastStartOffset + transform.position, spherecastRadius, spherecastDirection.normalized, out hit, spherecastDirection.magnitude);
10.  
11.     if (hit.collider != null)
12.     {
13.         if (jumpState != 3)
14.         {
15.             CreatureGroundDebug("Onground because jumpstate != 3");
16.             return true;
17.         }
18.         else if (yVelocity <= 0)
19.         {
20.             CreatureGroundDebug("Onground because yvel <=0");
21.             return true;
22.         }
23.         else
24.         {
25.             return false;
26.         }
27.     }
28.     else
29.     {
30.         return false;
31.     }
32. }

isOnGround is compiling in Jumpable now but I haven't totally finished. Notice that isOnGround is a public method, which means there might be other components which rely on Creture having this method available to them. That means I can't rip it out completely from Creature, but I also don't want to leave the code duplicated. Instead, I'm now going to add the Jumpable class as a component to Creature.

Code (csharp):

1. //References
2. public Animator anim;
3. public CharacterController controller;
4. public GameObject body;
5. public AudioSource voice;
6. public NavMeshAgent nav;//This will only be found on an NPC
7. private Jumpable jumper;

Note that I made the component private. I always default to private unless I find a reason to increase the visibility of the component.
Don't forget to fix our initialization of this component in the Boot method.

Code (csharp):

1. void Boot()
2. {
3.   ArticulateSelf();
4.   angularSpeedPerFixedUpdate = angularSpeed * Time.fixedDeltaTime * Mathf.Deg2Rad;
5.   ConfigValues();
6.   PrefillLists();
7.   //Here we prefill lists with random values
8.   SphereCollider c = GetComponent<SphereCollider>();
9.   jumper = new Jumpable(c, controller);
10.   Destroy(c);
11. }

Now I can change the isOnGround method of Creature to simply delegate to the Jumpable component

Code (csharp):

1. public bool isOnGround()
2. {
3.   return jumper.isOnGround(transform, controller.velocity.y);
4. }

Okay, one down..

CreatureGroundDebug is a private method and using the Find All References command, I found it is only used within isOnGround, so I pulled it out of Creature completely and added it to Jumpabale. That was easy.

Next let's tackle the TryJump method I created. My goal here is to remove the jumping complexity from Creature, so I copied the code from the FixedUpdate and replaced it with:

Code (csharp):

1. void FixedUpdate()
2. {
3.   velocity = transform.position - previousPosition;
4.   previousPosition = transform.position;
5.   //Shortcuts.ClearConsole();
6.   //Debug.Log("Vel: " + controller.velocity);
7.   //Debug.Log("OnGround: " + isOnGround());
8.   //if (jumpState != 0) Debug.Log("jumpState: " + jumpState);
9.   anim.SetBool("grounded", isOnGround());
10.  
11.   jumper.TryJump();
12.  
13.   //Code for rotating towards velocity
14.   RotateTowardsVelocity();
15. }

Now looking at the TryJump() method in Jumpable, I can tell this is going to be a bit awkward. There are a lot of crisscrossing dependencies here. The jump code seems to need knowledge of the animator, the controller and makes heavy use of the move command. This is probably a good target for more refactoring, but, again, I just want to focus on teasing out the jumping component. So, let's just figure out what references we need to pass over to the TryJump command to get this working.

Intelisense is doing a lot of heavy lifting for me, and highlights these problems:
Move(0);
isOnGround()
anim.SetTrigger("Jump");
flightVelocity.x = controller.velocity.x;

There are a number of others, but they appear to be similiar. Basically, we need access to the Move command, more info for the isOnGround command, access to the anim component, another local variable: flightVelocity, and access to the controller component.

I'm going to skip the Move command for now because this will be a bit tricky to explain.

Let's look at isOnGround. The method is now local but it needs more information: namely transform and the controller's y velocity. Since I need quite a bit more from the controller in this method, I will add both transform and controller as arguments.

Code (csharp):

1. public void TryJump(Transform transform, CharacterController controller)

I'll also need the animator component, so let's ask for that as well

Code (csharp):

1. public void TryJump(Transform transform, CharacterController controller, Animator anim)

Finally, flightVelocity is another local field that I didn't anticipate needing. I flip back to the Creature class and do a Find All References on this field and discover that it is no longer being used, so it's safe to move to Jumpable.

Okay, let's deal with Move. This is a tough one because we are heading towards componentization but we are sill a long ways off. I would love to pass in the Movable component and be done with it, but I haven't extracted that component yet..

This bit will likely be abit difficult to follow but don't worry if you don't understand right away. What we need to recognize is that the Creature class is an almagamation of components which we are trying to tease apart. One of the implications is that Creature is the Movable component, in addition to a number of other components, such as Actionable, Recruitable, etc .. So how can we use that to our advantage? We need to pass in the Movable component, and since Creature is a Movable component, we wil just pass in Creature.

Okay, hang on a sec. You are probably going balistic here.. How can passing around the very object we are trying to decouple ourselves from be any better than just belonging to that object?? You are totally correct. This is not a pretty solution but you must remember that Refactoring is not an instant fix. It will take time and a few more refactors before we're able to totally componentize Jumpable. That said, it is still ludicrus to pass in the entire Creature object just so we can use the Move funtionality. So instead, I'm going to introduce an abstraction. This is a bit premature since I haven't had the oportunity to explain the benefits of abstraction yet but just try to keep up for now.

Eventually, I will refactor the moving logic out of Creature into a Movable class. I don't know exactly how it will look except that it will have a Move function on it that will probably, at least initially, look like this:

Code (csharp):

1. void Move(int type, Nullable<Vector3> velocity = null)

Since I know what the method is going to look like, I can create an abstraction for the Movable component without actually creating it. This is what an Interface is for. Let's create a new interface called IMovable (convention is to start all C# interfaces with a capital I).

Code (csharp):

1. using System;
2. using UnityEngine;
3.  
4. namespace Assets.CreatureComponents
5. {
6.     interface IMovable
7.     {
8.         void Move(int type, Nullable<Vector3> velocity = null);
9.     }
10. }

As I mentioned earlier, Creature is the Movable component but the compiler doesn't know that yet. I will add this information to the declaration of Creature class.

Code (csharp):

1. public class Creature : MonoBehaviour, CreatureComponents.IMovable

This tells the compiler that this class is a MonoBehaviour and an IMovable. If this is confusing, it's probably because you've heard C# doesn't support multiple inheritance. This isn't really true though, it's just a misnomer caused by sliding definitions.. I won't try to explain that here but suffice it to say we are free to inherit from one class and as many interfaces as we like. If you want to get a better understanding of that, I think this old conversation might be useful. http://forum.unity3d.com/threads/interfaces-vs-inheritance-help.373237/

Okay, so we're getting close but we need to fix something related to this component. As is, the compiler will complain that Creature cannot implement IMovable.Move because its method is not marked as public. This seems a bit counterintuitive -- we are making a private method public jsut for the sake of the refactoring. But don't forget that our goal is to completely componentize Movable later on, so we are going to need this method to be public anyways. It wouldn't make sense to add a Movable component which cannot actually be asked to move the thing it was attached to. Let's change the Move method in Creature to be public.

Now, we add this component to the TryJump method

Code (csharp):

1. public void TryJump(Transform transform, CharacterController controller, Animator anim, IMovable mover)

We have all the information we need, so let's get to fixing the compile errors.
First, any calls to Move need to be changed to mover.Move.
Second, isOnGround will need the transform and controller.velocity.y
Everything else has resolved itself (based on moving flightVelocity and passing in the animator and controller components using their original names)
Here is my fixed TryJump method.

Code (csharp):

1. public void TryJump(Transform transform, CharacterController controller, Animator anim, IMovable mover)
2. {
3.     if (jumpState == 0)
4.     {
5.         //If we are not jumping, or if a jump has just finished
6.         //controller.SimpleMove(moveDirection*Time.fixedDeltaTime);
7.         //controller.SimpleMove(moveVelocity);
8.         mover.Move(0);
9.     }
10.     else if (jumpState == 1)//Jump state will be set to 1 by the external controller
11.     {
12.         //if (isOnGround == true)
13.         if (isOnGround(transform, controller.velocity.y) == true)
14.         {
15.             //We start a jump
16.             Debug.Log("Starting a jump");
17.             anim.SetTrigger("Jump");
18.             jumpState = 2;
19.             jumpTakeoffTimer = 0f;
20.         }
21.         else
22.         {
23.             Debug.Log("Trying to jump, but we're not grounded");
24.             //controller.SimpleMove(moveVelocity);
25.             mover.Move(0);
26.         }
27.     }
28.     else if (jumpState == 2)
29.     {
30.         if (jumpTakeoffTimer >= jumpTakeoffDelay)
31.         {
32.             flightVelocity.x = controller.velocity.x;
33.             flightVelocity.z = controller.velocity.z;
34.             flightVelocity.y = baseJumpSpeed;
35.             jumpState = 3;
36.         }
37.         else
38.         {
39.             jumpTakeoffTimer += Time.fixedDeltaTime;
40.             //controller.SimpleMove(moveVelocity);
41.             mover.Move(0);
42.         }
43.     }
44.  
45.     if (jumpState == 3)//Once here, then we have lifted off from the ground
46.     {
47.         //controller.Move(flightVelocity * Time.fixedDeltaTime);
48.         mover.Move(1, flightVelocity * Time.fixedDeltaTime);
49.  
50.         if (isOnGround(transform, controller.velocity.y) == false)
51.         {
52.             //We are flying through the air. Gravity gradually applies more
53.             flightVelocity.y -= Time.fixedDeltaTime * Mathf.Abs(Physics.gravity.y);
54.         }
55.         else if (controller.velocity.y < 0f)
56.         {
57.             //We have touched down. jumping is finished/
58.             Debug.Log("Touchdown vel: " + controller.velocity);
59.             anim.SetBool("grounded", true);
60.             jumpState = 0;
61.         }
62.     }
63. }

Now, we need to go back to Creature.FixedUpdate and fix our call.

Code (csharp):

1. jumper.TryJump(transform, controller, anim, this);

This should be fairly straightforward except maybe the use of "this". Remember that in Creature.FixedUpdate, this refers to the Creature object. Also remember that, for now, the Creature object is the Movable component. So passing in "this" is just how we get TryJump the Movable component.

Okay, we're nearly done for today but there was one more piece of code I stole from Creature: jumpState.

While I was working through TryJump, I found a comment that makes me feel very nervous.

Code (csharp):

1. else if (jumpState == 1)//Jump state will be set to 1 by the external controller

I usualy never like the idea of an external controller changing my local variables, and certainly not to do something that could have been accomplished with a simple method call. I would prefer to make jump state compleley invisible to other components but I'm not sure what else might rely on reading this information. Based on the comment I'm going to assume that only the controller component would try to change the value so I will make this variable readonly with the caveat that I would need to modify the controller component to initiate the jump in another way.

That's not totally accurate because, without any changes, the controller component is going to be operating on the Creature class and we will introduce a hold-over here to allow the controller to continue working as normal .. for now.

First, let's lock down the jumpState in Jumpable. I will make this property readonly by changing the set accessor to be private. Also, since no other code appears to be setting this property, I will get rid of most of the logic.

Code (csharp):

1. public int jumpState //0 = not jumping. 1 = Jump queued for a grounded frame 2= Jump started, but takeoff not achieved yet. 3 = liftoff
2. { get; private set; }

Now, I'll add the function which permits another component to initiate a jump

Code (csharp):

1. public void InitiateJump()
2. {
3.     if (jumpState == 0)
4.         jumpState = 1;
5. }

And finally, add the hold-over to Creature.

Code (csharp):

1. public int jumpState
2. {
3.     get
4.     {
5.         return jumper.jumpState;
6.     }
7.     set
8.     {
9.         Debug.Log("Jumpstate being set to " + value);
10.         if (value == 1)
11.             jumper.InitiateJump();
12.     }
13. }

This was probably the most dangerous change I made because I don't know how all the components in your project interact. It's possible that another component relied on being able to set the jumpState to an arbitray value. If this is the case, I think there is a more fundamental problem and it would require special analysis. For now, I'm happy with the changes.

Okay, so let's take a final look at the classes as they are now, and wrap up.

Spoiler: Creature.cs

Code (csharp):

1. using UnityEngine;
2. using System;
3. using System.Collections;
4. using System.Collections.Generic;
5.  
6. namespace Assets
7. {
8.     public class Creature : MonoBehaviour, CreatureComponents.IMovable
9.     {
10.         //Config
11.         public float baseSpeed = 1.75f;//The maximum velocity in metres per second, that the creature can jog at. When sprinting or walking, it moves at some multiplier of this value
12.         public float angularSpeed = 180f;//The number of degrees we can turn in one second. This will be converted to radians
13.         public float walkMult = 0.4f;
14.         public float sprintMult = 2.5f;
15.         public List<float> ActionDelays = new List<float>(8);
16.         public List<float> ActionDurations = new List<float>(8);
17.         public List<AudioClip> ActionSounds = new List<AudioClip>(8);
18.         public List<string> ActionAnimationTriggers = new List<string>(8);
19.         public float recruitMinRadius = 5f;//radiuses for herd recruitment
20.         public float recruitMaxRadius = 20f;
21.  
22.         public float threat = 1f;//How much threat this creature applies to the nearest angle of the threat matrix of a creature
23.         public float threatProjection = 0.1f;//The multiplier of threat that this creature applies to the FARTHEST angle of a creature's threat matrix.
24.         public float threatFalloff = 0.01f;//How much of the threat is lost per metre distance from the target
25.                                            //The amount applied to intermediary angles linearly interpolates between threat, and threat*threatProjection
26.  
27.  
28.         public int type = -1;//Which type of creature this is. -1 means undefined
29.                              //0 = wolf
30.  
31.         //References
32.         public Animator anim;
33.         public CharacterController controller;
34.         public GameObject body;
35.         public AudioSource voice;
36.         public NavMeshAgent nav;//This will only be found on an NPC
37.         private CreatureComponents.Jumpable jumper;
38.  
39.         public int jumpState
40.         {
41.             get
42.             {
43.                 return jumper.jumpState;
44.             }
45.             set
46.             {
47.                 Debug.Log("Jumpstate being set to " + value);
48.                 if (value == 1)
49.                     jumper.InitiateJump();
50.             }
51.         }
52.  
53.  
54.         List<string> boolsSet = new List<string>();
55.         public float speed;//magnitude of our current velocity
56.         public float maxSpeed;//calculated as basespeed * sprintmult
57.         float angularSpeedPerFixedUpdate;
58.  
59.         public float AISpeedMult = 1.0f;
60.         private bool _sprinting = false;
61.         public bool sprinting
62.         {
63.             get { return _sprinting; }
64.             set
65.             {
66.                 _sprinting = value;
67.                 RecalculateVelocity();
68.             }
69.         }
70.         public bool moveAllowed = true;//When false, the creature will not call Move or SimpleMove, thus holding it in place. All other functions and calculations will run as normal, including setting of animation bools. Resulting in the creature animating in-place.
71.         public bool rotateAllowed = true;//When false, the creature will not rotate towards its velocity, thus locking it at the same world-relative orientation.
72.  
73.  
74.         public List<int> actionStatuses = new List<int>();//0 = ready, 1 = in progress, 2 = cooling
75.         public bool AIControlled = false;
76.  
77.         public Vector3 velocity = Vector3.zero;
78.  
79.         //Operations
80.         private int controlMode = 0;
81.         float speedMult = 1.0f;//What we multiply the speed by when moving. This is largely determined by walking or sprinting
82.         private bool isLocomoting = false;
83.         Vector3 moveDirection;
84.         Vector3 moveVelocity;
85.         Quaternion targetTurningRotation;//The rotation we are currently turning to face
86.         int targetTurningDirection;//-1 = left, 1 = right, 0 = not turning.
87.         float turningAngleToTarget;//how many degrees til we reach the target
88.         Vector3 previousPosition;
89.  
90.  
91.         //Constants
92.  
93.         //Creature control modes
94.         int CONTROL_MODE_NONE = 0;//The control mode hasn't been figured out yet. IT cannot do anything.
95.         int CONTROL_MODE_AI = 1;//The creature is controlled by an AI script
96.         int CONTROL_MODE_ACTIVE = 2;//The creature is controlled by keyboard input right now
97.         int CONTROL_MODE_INACTIVE = 3;//The creature is normally controlled by input, but it is not active right now, and so it is idle.
98.         int CONTROL_MODE_INACTIVE_AI = 4;//The creature is normally controlled by input, but not right now, it is temporarily controlled by an AI instead
99.  
100.         public void RecalculateVelocity()
101.         {
102.             speedMult = 1f;
103.             if (sprinting == true)
104.             {
105.                 speedMult *= sprintMult;
106.             }
107.             else
108.             {
109.                 speedMult *= AISpeedMult;
110.             }
111.             moveVelocity = moveDirection * baseSpeed * speedMult;
112.             anim.SetFloat("Speed", moveVelocity.magnitude);
113.         }
114.  
115.         public void Locomote(Vector3 direction)
116.         {
117.             if (direction != Vector3.zero)
118.             {
119.                 moveDirection = new Vector3(direction.x, 0, direction.z);
120.                 RecalculateVelocity();
121.                 isLocomoting = true;
122.             }
123.             else
124.             {
125.                 moveDirection = direction;
126.                 RecalculateVelocity();
127.                 isLocomoting = false;
128.             }
129.             Vector3 drawFrom = transform.position + new Vector3(0f, 1f, 0f);
130.             Debug.DrawLine(drawFrom, VectorTools.Flatten(moveDirection) + drawFrom, new Color(0.0f, 1.0f, 0.0f), 10.0f);
131.             Debug.DrawLine(drawFrom, transform.position + new Vector3(UnityEngine.Random.value, UnityEngine.Random.value, UnityEngine.Random.value));
132.         }
133.  
134.         public void Move(int type, Nullable<Vector3> velocity = null)
135.         {
136.             //type: 0 = simpleMove, 1 = Move
137.  
138.             if (AIControlled == true)
139.             {
140.                 nav.Move(moveVelocity * Time.deltaTime);
141.             }
142.             else if (type == 0)
143.             {
144.                 controller.SimpleMove(moveVelocity);
145.             }
146.             else if (type == 1)
147.             {
148.                 if (velocity != null)
149.                 {
150.                     controller.Move((Vector3)velocity);
151.                 }
152.                 else
153.                 {
154.                     controller.Move(moveVelocity * Time.fixedDeltaTime);
155.                 }
156.             }
157.         }
158.  
159.         void Boot()
160.         {
161.             ArticulateSelf();
162.             angularSpeedPerFixedUpdate = angularSpeed * Time.fixedDeltaTime * Mathf.Deg2Rad;
163.             ConfigValues();
164.             PrefillLists();
165.             //Here we prefill lists with random values
166.             SphereCollider c = GetComponent<SphereCollider>();
167.             jumper = new CreatureComponents.Jumpable(c, controller);
168.             Destroy(c);
169.         }
170.  
171.         public virtual void ConfigValues()
172.         {
173.             Debug.Log("Setting maxspeed: " + baseSpeed + " " + sprintMult);
174.             maxSpeed = baseSpeed * sprintMult;
175.         }
176.  
177.         void PrefillLists()
178.         {
179.             while (ActionDelays.Count < 8)
180.             {
181.                 ActionDelays.Add(0f);
182.             }
183.  
184.             while (ActionSounds.Count < 8)
185.             {
186.                 ActionSounds.Add(null);
187.             }
188.  
189.             while (ActionAnimationTriggers.Count < 8)
190.             {
191.                 ActionAnimationTriggers.Add("");
192.             }
193.  
194.             while (actionStatuses.Count < 8)
195.             {
196.                 actionStatuses.Add(0);
197.             }
198.  
199.             while (ActionDurations.Count < 8)
200.             {
201.                 ActionDurations.Add(0f);
202.             }
203.         }
204.  
205.         void ArticulateSelf()
206.         {
207.             anim = GetComponent<Animator>();
208.             controller = GetComponent<CharacterController>();
209.             body = gameObject;
210.             voice = GetComponentInChildren<AudioSource>();
211.  
212.             /*nav = GetComponent<NavMeshAgent>();
213.             if (nav != null)
214.             {
215.                 nav.Stop();
216.                 nav.updatePosition = false;
217.                 nav.updateRotation = false;
218.             }
219.              */ //we will do navmeshagent through the AI script, since it is only relevant there
220.             Debug.Log(gameObject.name + ": " + anim + "     " + controller + "     " + body + "     " + voice);
221.         }
222.  
223.         void Command(string command)
224.         {
225.             anim.SetBool(command, true);
226.         }
227.  
228.         void StopCommand(string command)
229.         {
230.             anim.SetBool(command, false);
231.         }
232.  
233.         public bool isOnGround()
234.         {
235.             return jumper.isOnGround(transform, controller.velocity.y);
236.         }
237.  
238.         // Use this for initialization
239.         void Awake()
240.         {
241.             Boot();
242.         }
243.  
244.         // Update is called once per frame
245.         void FixedUpdate()
246.         {
247.             velocity = transform.position - previousPosition;
248.             previousPosition = transform.position;
249.             //Shortcuts.ClearConsole();
250.             //Debug.Log("Vel: " + controller.velocity);
251.             //Debug.Log("OnGround: " + isOnGround());
252.             //if (jumpState != 0) Debug.Log("jumpState: " + jumpState);
253.             anim.SetBool("grounded", isOnGround());
254.  
255.             jumper.TryJump(transform, controller, anim, this);
256.  
257.             //Code for rotating towards velocity
258.             RotateTowardsVelocity();
259.         }
260.  
261.         void RotateTowardsVelocity()
262.         {
263.             Vector3 from = VectorTools.Flatten(transform.forward);
264.             Vector3 to;
265.             if (AIControlled == true)
266.             {
267.  
268.                 to = this.velocity.normalized;
269.             }
270.             else
271.             {
272.                 to = controller.velocity.normalized;
273.             }
274.             transform.forward = Vector3.RotateTowards(from, to, angularSpeedPerFixedUpdate, 0f);
275.         }
276.  
277.  
278.  
279.         public virtual IEnumerator Action1()
280.         {
281.             StartCoroutine(GenericAction(1));
282.             yield return null;
283.         }
284.         public virtual IEnumerator Action2()
285.         {
286.             StartCoroutine(GenericAction(2));
287.             yield return null;
288.         }
289.         public virtual IEnumerator Action3()
290.         {
291.             StartCoroutine(GenericAction(3));
292.             yield return null;
293.         }
294.         public virtual IEnumerator Action4()
295.         {
296.             StartCoroutine(GenericAction(4));
297.             yield return null;
298.         }
299.         public virtual IEnumerator Action5()
300.         {
301.             StartCoroutine(GenericAction(5));
302.             yield return null;
303.         }
304.         public virtual IEnumerator Action6()
305.         {
306.             StartCoroutine(GenericAction(6));
307.             yield return null;
308.         }
309.         public virtual IEnumerator Action7()
310.         {
311.             StartCoroutine(GenericAction(7));
312.             yield return null;
313.         }
314.         public virtual IEnumerator Action8()
315.         {
316.             StartCoroutine(GenericAction(8));
317.             yield return null;
318.         }
319.  
320.         //Common Action Functions
321.         public virtual IEnumerator GenericAction(int type)
322.         {
323.             int a = type;
324.             actionStatuses[a - 1] = 1;
325.             bool delay = false;
326.             float timePassed = 0f;
327.             float delayTime = (float)ActionDelays[a - 1];
328.             //Is there a delay
329.             if (delayTime > 0f)
330.             {
331.                 delay = true;
332.             }
333.  
334.  
335.             if (delay)
336.             {
337.                 while (timePassed < delayTime)
338.                 {
339.                     timePassed += Time.deltaTime;
340.                     yield return null;
341.                 }
342.             }
343.  
344.  
345.             AudioClip sound = ActionSounds[a - 1];
346.             if (sound)
347.             {
348.                 Debug.Log("Voice: " + voice);
349.                 voice.clip = sound;
350.                 voice.Play();
351.             }
352.  
353.             string trigger = ActionAnimationTriggers[a - 1];
354.             if (trigger != "")
355.             {
356.                 //Debug.Log("Setting " + trigger);
357.                 anim.SetTrigger(trigger);
358.             }
359.             else
360.             {
361.                 //Debug.Log("Setting doAction");
362.                 anim.SetTrigger("doAction");
363.             }
364.  
365.             float duration = ActionDurations[a - 1];
366.             while (timePassed < duration)
367.             {
368.                 timePassed += Time.deltaTime;
369.                 yield return null;
370.             }
371.  
372.             actionStatuses[a - 1] = 0;
373.         }
374.  
375.         public virtual void Recruit(int type)
376.         {
377.             //This function takes a type of creature
378.  
379.             //It will then find all NPCs of that creature type, and recruit them based on their distance, and the min/max recruit distances
380.             List<Creature> creatures = new List<Creature>();
381.  
382.             //1. Find all creatures of matching type in scene
383.             GameObject[] npcs = GameObject.FindGameObjectsWithTag("NPC");
384.             foreach (GameObject npc in npcs)
385.             {
386.                 MonoBehaviour[] scripts = npc.GetComponents<MonoBehaviour>();
387.                 foreach (MonoBehaviour s in scripts)
388.                 {
389.                     if (s is Creature)
390.                     {
391.                         Creature c = (Creature)s;
392.                         if (c.type == this.type)
393.                         {
394.                             creatures.Add(c);
395.                             break;
396.                         }
397.                     }
398.                 }
399.  
400.             }
401.  
402.             //2. Check the distance to each one, and if within range, decide random odds for a recruitment to work
403.         }
404.  
405.         public virtual void Threaten(float range, float activeThreat = Mathf.Infinity)
406.         {
407.             Vector3 ourpos = transform.position;
408.             if (activeThreat == Mathf.Infinity)
409.             {
410.                 //Activethreat allows us to pass in a specific value to override our creature's base threat value. if none is passed, then the base is used
411.                 activeThreat = threat;
412.             }
413.             float sqrRange = range * range;//We square the range so we can compare it against square magnitudes
414.  
415.  
416.             Creature[] allCreatures = GameManager.CreatureList();
417.             foreach (Creature c in allCreatures)
418.             {
419.                 AI cAI = c.gameObject.GetComponent<AI>();
420.                 if (cAI != null) //we can only threaten things that have an AI script, so skip it if not.
421.                 {
422.                     Vector3 offset = (c.transform.position - ourpos);
423.                     float sqrDist = offset.sqrMagnitude;
424.                     if (sqrDist <= sqrRange)
425.                     {
426.                         //the creature is within range
427.                         float distfactor = offset.magnitude / range;
428.                         float attenuatedThreat = threat * distfactor;//The threat is reduced linearly by the range
429.                         ThreatProfile tp = new ThreatProfile();
430.                         tp.source = this;
431.  
432.                     }
433.                 }
434.             }
435.         }
436.  
437.     }
438.  
439. }

Spoiler: Jumpable.cs

Code (csharp):

1. using UnityEngine;
2.  
3. namespace Assets.CreatureComponents
4. {
5.     class Jumpable
6.     {
7.         public float baseJumpSpeed = 8f;
8.         public float jumpTakeoffDelay = 0.43f;
9.         float jumpTakeoffTimer = 0f;
10.         Vector3 flightVelocity;
11.  
12.         float spherecastRadius;
13.         Vector3 spherecastStartOffset;
14.         Vector3 spherecastEndOffset;
15.         Vector3 spherecastDirection;
16.  
17.         public Jumpable(SphereCollider c, CharacterController controller)
18.         {
19.             spherecastEndOffset = c.center;
20.             spherecastStartOffset = controller.center;
21.             spherecastDirection = spherecastEndOffset - spherecastStartOffset;
22.             spherecastRadius = c.radius;
23.         }
24.  
25.      
26.         public int jumpState //0 = not jumping. 1 = Jump queued for a grounded frame 2= Jump started, but takeoff not achieved yet. 3 = liftoff
27.         { get; private set; }
28.  
29.         public void InitiateJump()
30.         {
31.             if (jumpState == 0)
32.                 jumpState = 1;
33.         }
34.  
35.         public bool isOnGround(Transform transform, float yVelocity)
36.         {
37.             if (jumpState == 2)
38.             {
39.                 return false;
40.             }
41.  
42.             RaycastHit hit;
43.             Physics.SphereCast(spherecastStartOffset + transform.position, spherecastRadius, spherecastDirection.normalized, out hit, spherecastDirection.magnitude);
44.  
45.             if (hit.collider != null)
46.             {
47.                 if (jumpState != 3)
48.                 {
49.                     CreatureGroundDebug("Onground because jumpstate != 3");
50.                     return true;
51.                 }
52.                 else if (yVelocity <= 0)
53.                 {
54.                     CreatureGroundDebug("Onground because yvel <=0");
55.                     return true;
56.                 }
57.                 else
58.                 {
59.                     return false;
60.                 }
61.             }
62.             else
63.             {
64.                 return false;
65.             }
66.         }
67.  
68.         void CreatureGroundDebug(string input)
69.         {
70.             if (jumpState >= 2) Debug.Log(input);
71.         }
72.  
73.         public void TryJump(Transform transform, CharacterController controller, Animator anim, IMovable mover)
74.         {
75.             if (jumpState == 0)
76.             {
77.                 //If we are not jumping, or if a jump has just finished
78.                 //controller.SimpleMove(moveDirection*Time.fixedDeltaTime);
79.                 //controller.SimpleMove(moveVelocity);
80.                 mover.Move(0);
81.             }
82.             else if (jumpState == 1)//Jump state will be set to 1 by the external controller
83.             {
84.                 //if (isOnGround == true)
85.                 if (isOnGround(transform, controller.velocity.y) == true)
86.                 {
87.                     //We start a jump
88.                     Debug.Log("Starting a jump");
89.                     anim.SetTrigger("Jump");
90.                     jumpState = 2;
91.                     jumpTakeoffTimer = 0f;
92.                 }
93.                 else
94.                 {
95.                     Debug.Log("Trying to jump, but we're not grounded");
96.                     //controller.SimpleMove(moveVelocity);
97.                     mover.Move(0);
98.                 }
99.             }
100.             else if (jumpState == 2)
101.             {
102.                 if (jumpTakeoffTimer >= jumpTakeoffDelay)
103.                 {
104.                     flightVelocity.x = controller.velocity.x;
105.                     flightVelocity.z = controller.velocity.z;
106.                     flightVelocity.y = baseJumpSpeed;
107.                     jumpState = 3;
108.                 }
109.                 else
110.                 {
111.                     jumpTakeoffTimer += Time.fixedDeltaTime;
112.                     //controller.SimpleMove(moveVelocity);
113.                     mover.Move(0);
114.                 }
115.             }
116.  
117.             if (jumpState == 3)//Once here, then we have lifted off from the ground
118.             {
119.                 //controller.Move(flightVelocity * Time.fixedDeltaTime);
120.                 mover.Move(1, flightVelocity * Time.fixedDeltaTime);
121.  
122.                 if (isOnGround(transform, controller.velocity.y) == false)
123.                 {
124.                     //We are flying through the air. Gravity gradually applies more
125.                     flightVelocity.y -= Time.fixedDeltaTime * Mathf.Abs(Physics.gravity.y);
126.                 }
127.                 else if (controller.velocity.y < 0f)
128.                 {
129.                     //We have touched down. jumping is finished/
130.                     Debug.Log("Touchdown vel: " + controller.velocity);
131.                     anim.SetBool("grounded", true);
132.                     jumpState = 0;
133.                 }
134.             }
135.         }
136.     }
137. }

Spoiler: IMovable.cs

Code (csharp):

1. using System;
2. using UnityEngine;
3.  
4. namespace Assets.CreatureComponents
5. {
6.     interface IMovable
7.     {
8.         void Move(int type, Nullable<Vector3> velocity = null);
9.     }
10. }

 

Great post! I noticed two things:

1: JumpState should really be an enum. That'll make the code a lot easier to read:

Code (csharp):

1. if(jumpState == 1) { } //Have to put a comment here explaining what "1" is
2.  
3. vs
4.  
5. if(jumpState == JumpState.jumpQueued) { } //No comment needed

2: Implementations of interface methods does not have to be public. You can implement them privately, if you put the interface name in front of it. You can still access the method outside of the class, but only if you're using the object in the context of being a part of the interface. This is an explicit interface implementation:

Spoiler: Private interface?

Code (csharp):

1. public interface ISomeInterface {
2.     void SomeMethod();
3. }
4.  
5. public class BastesTestScript : MonoBehaviour, ISomeInterface {
6.  
7.     void ISomeInterface.SomeMethod() {}
8. }
9.  
10. public static class WHAT {
11.  
12.     public static void _WHAT_(BastesTestScript bts) {
13.         bts.SomeMethod(); //Does not compile!
14.         (bts as ISomeInterface).SomeMethod(); //Compiles!
15.     }
16. }

This is a bit insane, but still makes sense for your Move method; you want to be able to tell an IMovable to move, but not a creature. Even if the Move interface will be refactored away, implementing the Move method in this way will guard you from trying to move a creature from places it shouldn't be moved, which would make refactoring harder.

 

Thanks for the feedback Baste! You make two very good points.

Regarding the enum, I had it in my head that I wanted to, eventually, eliminate jumpState entirely so I was mostly ignoring it for now. But I actually have no idea how I might achieve that, so the enum is a really cheap way to improve the readability of the code.

Regarding the private interface implementation. I actually did not know this but it explains why explicit interface implementations do not always show up in meta data! This seems like a really bizarre way of thinking about systems because Interfaces are supposed to guarantee that an operation is supported. However, in this case I think it is a perfect fit because I don't really need to expose the Move method unless I am explicitly treating the Creature class as the movement component. Anyways, this would just be a stop-gap since my end goal is to decouple the movement component completely and at that point I would be okay with the method being public.

There are a bunch of other things I might do to tidy up here but I'm mainly going for componentization so I'm going to mostly ignore everything else until I achieve that. The point of the refactoring is to show how you can keep old investments and make incremental changes towards new design goals.