Official State Machine

@deivsky and others, I tried to create new states, actions, conditions and others for enemy, as @cirocontinisio said me to do for my Simple Enemy System, to adapt the currently used statemachine. I did so today, but at last ended up with some errors.

When I finished added some states (Idle, Move and Attack), actions (Moving, Attacking), Conditions and Transitions and added respective scripts to it, there were no errors in console.

I played the game, went near the enemy but there were no expected actions. When I looked at the console there was one error: (Image below)



And the State Machine script on enemy was disabled (but It was enabled before playing the game)
When I manually enabled the script, the console was showing another error, NullObjectReference repeatedly again and again (Image Below):



Please Help...

 

Hey @Harsh-099,

from the photo you attached it seems like you added the same transition twice (enemy to moving) which is throwing the error. i would make sure you don't have duplicates and see how it goes.

If you haven't gotten any help by tonight I'll boot up the project and troubleshoot this for you.

 

Hey!

I managed to replicate the error, and I'm not sure how it didn't come up earlier! .

It happens during initialization, when a Transition that's being created chains up to a State that hasn't been created holding the same Transition.

Examples:
StateA => TransitionA => StateB => TransitionA => Exception. (why would you do this?)
StateA => TransitionA => StateB => TransitionB => StateC => TransitionA => Exception. (this seems more likely)

Where all the States and Transitions are being instantiated for the first time.

This is because the Transition is added to the Dictionary after getting its target State:


The solution would be doing the same as what I did with States where I add an empty State to the Dictionary and then fill it:


@Harsh-099 from your stack trace it seems like the transition that's causing you trouble is EnemyToMoving. For a quick solution, make copies of it for each state that uses it.

@cirocontinisio I'm gonna make a PR fixing this, but this wouldn't be happening if we had TransitionTables, any idea if/when that'll get merged?

Edit: Issue and PR submitted.

 

Sorry, @deivsky, I've been busy with other sides of the game and I saw the conversation here was still open and on fire as usual.

What is the current state of things? Can somebody do me a short recap? Like, should I try and pull in the fix first, or the TransitionTable PR?

 

It's okay, I know you've been busy, a lot of merges lately! And I know this isn't the easiest thread to follow, soon we'll catch up in posts with the Choose the game title thread .

So to recap:

• The current state of the StateMachine is that the whole workflow of state/transition/condition/action is so convoluted that it makes you not want to use the StateMachine at all.
• The TransitionTable PR, which comes with a custom editor, is currently the only proposal that addresses this issue. The one thing I'm afraid is I'm not much of an Editor scripting expert so, even though I've made my own testing and everything seems to work fine, more testing might be needed. But what better testing than merging and having people use it?
• There was a talk about GraphView but I'm not sure whether anyone is actually moving forward with it.
• Besides that we have the improved StateMachine debugger, by @WagnerGFX, which provides better logging of the transitions between states, including what condition is making the transition happen, proving really useful for spotting possible errors. Further iteration on this is still needed but I think it should get merged, and we can continue to add on top later.
• There's another draft PR by @WagnerGFX that would mainly improve performance on conditions that are repeated in one State by caching the result. It's draft because it depends on the debugger PR, but the change is minimal so I think it can be merged as well.

And about the fix:
Applying the fix would just be a temporary solution if the TransitionTable PR gets merged, because TransitionSOs (which are the cause of the issue) are entirely removed in it. Seeing that there are no other proposals to improve the workflow, I'd push the TransitionTable PR.

I don't think I'm missing anything, we had a couple of discussions going on but I think they came to proper conclusions. Anyone can correct me if I missed something and/or disagrees .

 

My suggestion is to also start merging with the TransitionTable, then go to the debugger and finally the condition caching.
Specially since the TransitionTable is a great improvement and the other 2 PRs will need some small changes due to merging conflicts.

There was a talk about improving the condition logic system, but we still don't have any conclusion and the condition caching is independent of that.

 

Solid plan, sounds good to me.

RE condition system logic:
IMO this is the lowest priority. these editor improvements may do the same job (from a ux PoV). I think we're mostly in agreement that we should hold off on condition system changes until we know we need them!

 

OK, thanks you all for the recap!
Since we're getting closer to a new livestream and I'd like to have something working to show and don't have some nasty last minute surprises on the now-working Character Controller, what about:
- I merge the fix PR on main
- I merge the other two (Transition table and Debugger) into another branch? And we keep iterating the state machine in there?

 

It should've been in a separate branch from the start

 

Oh, I thought it was happening because of something mine mistake. I will try it after the pull request gets merged.

 

It's a good idea, but we should also put a timer or another type of limit on the changes since this is a core system and could impact the rest of the development if we keep changing too much and for too long.

Is anyone cooking any other changes for the FSM?
At least I'm out of ideas.

 

It would be good to have a separate branch to continue development, but I think, after the livestream, we should merge what we currently have to main, that way we can start using it for the development of other systems. I believe most of what might be missing is editor stuff, so there shouldn't be any breaking changes that require rebuilding every state machine we have (like replacing StateTransitionSO with TransitionTableSO), and it should be safe to move forward with these other systems.

 

Merged the TransitionTable by @deivsky and the debugger by @WagnerGFX. Good job, guys!

I have merged them both on a new branch called 'state-machine-improvements', where you can experiment a little more and refine the state machine UX. It spins off from main so it contains all of the latest improvements and features in it.

I have left the caching one by @WagnerGFX unmerged since it's still a Draft.

However, as you said, let's aim to wrap this one up and make something usable: we need to start building things!

 

Since the other two PRs have been merged I will now be able to review the conflicts and make it a better PR.

@deivsky, have you seen this recent video about the Editor? The side list example at the end might actually be very useful to make a single window where every Transition Table is listed and able to edit.

 

That means I can work on enemy state machine from this 'states machines improvements' branch. But I have created my feature-enemy branch from main branch, how will I add the new branch changes to it?

 

That's not a huge issue, you can take your PR and point it to a different branch (by pressing Edit on the PR's web page itself), and in any case, your changes are just three scripts so it's probably faster to just save them locally, pull the latest changes on the state-machine-improvements branch, and put them back in or something.

 

Issue time!

When using an Assembly Definition, any Editor sub-folder loses its powers and everything is set to the same assembly.
That means any attempt to compile will give an error due to the use of Editor scripts.

The default solution is to add another assembly definition for that folder:
- The FSM and editor scripts will stay in the same place (good, minimal changes)
- The editor scripts will be isolated from the rest of the code (good practice)
- Another project inside the solution on the visual studio hierarchy (bothersome, but good)
- Seem to be the default recommendation on the Unity Manual (good)

There are other possible changes, but they have their own issues with good practices:
- Move the scripts to the existing Assets>Scripts>Editor
- Have a single UOP1.Editor assembly definition and add multiple references when needed.

Should I go with the default or there are any better suggestions?

 

Ahh I'd go with the default one. Usually I do but I saw everyone was using Editor folders, didn't know they don't work with assemblies.

 

I didn't either, but I opened Visual Studio and saw the editor code inside UOP1.StateMachine instead of Assembly-CSharp-Editor.

At that moment my Programmer Senses™ issued a warning

And now we have two issues, since there is another script using editor references. But that one is also in the main branch so I opened an issue. For this one in the State Machine I opened a PR with the fix.

 

Can I not pull the 'state-machine-changes' branch into my 'feature-enemy' branch and from there continue working?

And change the pull request pointing 'main' branch to 'state-machine-improvements' branch because i can't seem to edit my side branch in the pull request.

 

If you try to merge the changes from "state-machine-improvements" into your "feature-enemy" then the commits will fall out of sync. The best thing to do is just create a new updated branch and move manually the changes to it. I had to do the same with my changes for Condition Caching.

You can't change your side of the PR. I actually just had this issue with my changes and had to make a new PR with the new branch.

On another note.
@cirocontinisio, should we change the namespace of the FSM from UOP1.StateMachine to ChopChop.StateMachine?

 

So should i delete the "feature-enemy" branch (after moving changes from it) or not?

 

If the branch is unused and any PRs have been merged or cancelled, then I think there should be no problem. I usually follow this logic when deleting my branches.

 

Yes, please!!!

 

@Harsh-099 Assuming you've been commiting the changes into your branch you could create a new branch from the new state machine branch and then cherry pick the commits from yours to this new up-to-date branch and this way you wouldn't have to move any changes manually, but you would still need to check that your cherry picked changes doesn't change or use any non-existent files or code that isn't there anymore. And for those who didn't commit the changes yet they can stash their work and apply the stash into the updated branch.

 

What is meant by "cherry pick"?

 

Basically picking your commits one-by-one from your original branch and putting them into the up-to-date branch. Check out these examples:

• https://git-scm.com/docs/git-cherry-pick
  
• https://devconnected.com/how-to-cherry-pick-git-commits/
  
• https://www.git-tower.com/learn/git/faq/cherry-pick
  
• https://mattstauffer.com/blog/how-to-merge-only-specific-commits-from-a-pull-request/

 

Just merged the two PRs from @WagnerGFX - regarding caching and Editor assembly.

 

Hey all, I've made a few improvements to the styling of the custom inspector for the Transition Table:

• First, I fixed the colours when the Inspector is seen with the Professional skin (it was thought to work with Personal only).
• I added an explanation message at the top
• I resized the Object picker for the Condition SO. This allows to pick the object using the little target, but also to select and locate it in the Project window by clicking on the little SO icon to the left. Super useful in my opinion!!
• I made it so if the Condition is the last (or the only one), they don't show the logic criteria (And or Or). It was superfluous, since it's not being used in the logic check.

On this Inspector there's still a layout issue when it gets too narrow, but we can fix it later. I'll add it as an Issue soon.

On the Actions editor:

• Added an explanation message
• Same thing with the SO picker, I made it bigger.
• Added spacing here and there

 

So, do you guys think we can merge these improvements into main?
We really need to start setting up other animations, I've been meaning to do it but was waiting for these improvements.

Obviously you can still work on improvements for it, as long as they are not breaking changes.

 

This is not superfluous at all. Reading the conditions was hard even when knowing how it works.

@deivsky is still working in a full editor window for the transitions, but I don't know if the changes are close to finish.

 

Do we need this full editor? What is it going to bring? (question for @deivsky)
I feel like this Inspector is already much better than we had before.

Also we really need to put a hard deadline on this...

 

(Butting in ) The full editor window also has a list of every transition table in the project to select and edit faster instead of relying in finding them in the inspector. But its true that it is creating two places to edit the same thing.

If speed is important, it might be better to add later (since there are no changes in the core) or review the idea so it won't be a duplicated window. An alternative could be to make the window a dedicated listing (with no editing) of every SO related to the FSM so it can be selected or dragged to the inspector.

From what I can see there are no more changes to the core of the FSM unless some big issue appear. Performance and flexibility are good and the implementation is still simple enough.

 

Hi, sorry I haven't been so active, I'm moving out this week so it's been super busy lately .

These are great additions!
If by layout issue you mean with the add transition box, I've already addressed that in my branch with the editor window. I refactored it to use EditorGUI instead of EditorGUILayout. I can move it to a separate branch if we decide we don't want the window.

@WagnerGFX already made the point on why it's useful. The editor window is basically just for easier accessibility to get to the transition tables, then it simply draws the inspector of the selected one.

I'll be pushing a PR soon!

 

Okay, I've opened a new PR.

@cirocontinisio I fixed the layout issue you actually meant (found the TODO in the code).
@WagnerGFX I changed the add transition button to make it clearer, and moved the menu item as suggested.

 

Perfect, I merged it.
Honestly at this point, I'd just go ahead and start making more states and transitions, also to test this solution. But I would probably want to merge it to main. They are mostly Inspector/Editor scripts anyway, so I don't see much harm in doing so.

 

Hey all, I just pushed some refactoring to the state-machine-improvements branch. Just not to push it to main
It's not reeeally StateMachine stuff, but related. I'd like your feedback.

You will notice it has the jump animation now wired in. That was easy.

So two things:

- As I was adding a new action to the Pig (talking) I've realised that I'd have to add a listener to the input event on the Protagonist script, then add a boolean to the Character script so that the StateMachine would pick it up... in the end, I've merged the two scripts and deleted the Character script entirely, now it's only Protagonist.
This leads to a slightly longer Protagonist script, but this amount of steps Input Actions > InputReaderSO > Protagonist MB > Character MB > StateMachine > Animator seemed a bit redundant for a simple boolean, so I killed one step. Makes sense?

- There is some work to do on the input processing. For the jump button, there are two events to listen to (when you press the button and when you release) so we can know how long the button was pressed. For other buttons, I'd like to avoid all of that. But if we only get the event for when the action is performed, then the boolean inside the Protagonist MB is only set to true. Who sets it to false? At the moment, it's the Condition on the State Machine which, upon checking its value, "consumes" the input by setting it to false. So even if the player holds the button, the action is performed only once. How does it sound?

I also think I found a bug on the StateMachine editor, but I'll log it in later. (reminder)

 

Definitely makes sense. I didn't know why we had two MB that had basically the same purpose (control the character).

It's a little weird that the condition is the one consuming it, but I guess an action just to set it to false is also redundant. What you're doing is the equivalent of:

Code (CSharp):

1. if (boolean)
2. {
3.     boolean = false;
4.     // Do stuff...
5. }

Which is rather common, so I'd say it's fine.

 

Hi ChopChop's brains!

I am starting to look at using the state machine for enemies like the plant critter and I was wondering what was the targeted folder structure for state machine SO?

First, is it too early? Should I wait for the merge of state-machine-improvements branch?

Next, what is the targeted folder structure for all these SO?
There is this folder: ScriptableObjects/Protagonist, that seems quite generic, but I have the feeling that if we start to add SO for each NPC/Enemy here SO will soon be undiscoverable...

Also, the same state will not have the same conditions/actions depending of the protagonist. For instance Idle state for the playable character is not transitioning the same as an enemy.

Maybe the response is obvious, sorry I am not fully familiar with the big picture around the usage of our SM yet.

 

Hey, totally, I think it's time that we start making more use of the state machine. Main is already up to date with our branch, except for some editor stuff, so I'd say it's safe to move forward.

As for the folder structure, there just isn't one for enemies yet, but you can add it yourself. Following the same structure of the protagonist, you could make it ScriptableObjects/Enemies.

 

Hey all, and especially @deivsky: I've fixed what I think was a pretty nasty bug in the custom inspector, which would have led to plenty of mistakes and headaches: there was a missing

_serializedObject.ApplySerializedProperties();

and so the Inspector was not saving the changes... in some cases. Very confusing!
https://github.com/UnityTechnologies/open-project-1/commit/df934a4e425f62c881457e1e0161051354c43e15

@deivsky let me know if I saw it wrong and it shouldn't be fixed like this. Anyway, the fix is on main.

 

I thought it was enough with the onChangedCallback!

Code (CSharp):

1. reorderableList.onChangedCallback += list => list.serializedProperty.serializedObject.ApplyModifiedProperties();

 

No, I think that callback is only if "the list" changed, not what is contained in it
(this page seems to confirm)

 

Hi there,
we had a quick usage review of the state machine yesterday with a new comer and we found that it is difficult to understand that the current state rows in the transition table are expandable sections, despite the note at the top, that seems to be just ignored by users
Actually, my understanding is that the little arrow before the label has the same color as the background so it is completely invisible.

In term of possible solutions, the following could solve this issue:
- Expand the states by default when the table is opened
- Change the background color to make the arrow visible
- Change the arrow color (I did not find how to achieve this)

What do you think about it?

 

Hey, nice feedback!

I actually don't remember how I got rid of the arrow, I don't think it's hiding with the background, I think it's just not there because we're using a custom style. Anyway, I removed it because I didn't like it, but I do see it being a little confusing without it.

States are currently only expandable one at a time. I did this because I thought it would look very convoluted and become harder to navigate if we could have all open, and there's usually no need to look at more than one state at a time. We could have the initial state expanded by default though, that should make a good enough hint that you can expand any of the rest.

 

Yeah, I don't think they should be expanded by default. Any of them.
But having the arrow should be enough.

It's... interesting though that the first words of the notice message at the top of the inspector are "Click on any state..." but that was not enough!

 

It's never enough!

I get, though, that it's also a bit more confusing because at the same time there's a toolbar right on the foldout header. Maybe that whole thing is something that could be reviewed.

 

I think the little triangle to the left of the text would help a lot. It's a Unity standard. Obviously pointing down when it opens!

 

I know it's a bit late to talk about this, but as I was creating yet another StateAction class and having to write a lot of boilerplate to pass 3 variables from the SO to the actual Action, I was thinking about this...

... and I thought: can't they be SOs?!

Let me explain. I think the whole reason this separation exists is to not touch the SOs that we use to configure actions (because they might be shared by multiple

StateMachines

), right?

So why don't we simply instantiate new runtime SOs from the asset ones? This was every

 StateMachine

would own its copy of the data, could overwrite values and whatnot.

But this would also avoid A TON of boilerplate code.

Thoughts? cc @deivsky

 

The initial idea of separating SOs from runtime data was about shared vs. non-shared data.

For ex. Enemy SO can have a lot of shared properties, but only a few of them would be unique per-instance at runtime.

I agree that these "Authoring" SOs created a lot of boilerplate code, and all they do is just create runtime class for the instance, passing the same values..

So I'm wondering - why StateAction/StateCondition is not an interface for the struct?

That way, we can just paste struct data modified in the inspector, without boilerplate constructors.

Edit: in the example, parameters should actually be in the SO as a shared data, instead of being unique for every instance, wasting memory and "playmode editability". You get the idea