Search Unity

  1. Looking for a job or to hire someone for a project? Check out the re-opened job forums.
    Dismiss Notice
  2. Unity 2020 LTS & Unity 2021.1 have been released.
    Dismiss Notice
  3. Good news ✨ We have more Unite Now videos available for you to watch on-demand! Come check them out and ask our experts any questions!
    Dismiss Notice

Official State Machine

Discussion in 'Open Projects' started by Dustin_00, Sep 29, 2020.

  1. Harsh-099

    Harsh-099

    Joined:
    May 1, 2020
    Posts:
    186
    @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)

    AlreadyAddedException.jpg

    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):

    NullReferenceException.jpg

    Please Help...
     
    kirbygc00 likes this.
  2. kirbygc00

    kirbygc00

    Joined:
    Apr 4, 2016
    Posts:
    46
    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.
     
    Last edited: Nov 5, 2020
  3. deivsky

    deivsky

    Joined:
    Nov 3, 2019
    Posts:
    86
    Hey!

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

    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:
    upload_2020-11-5_15-40-18.png

    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:
    upload_2020-11-5_15-43-22.png

    @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? :p

    Edit: Issue and PR submitted.
     
    Last edited: Nov 5, 2020
    Harsh-099 likes this.
  4. cirocontinisio

    cirocontinisio

    Unity Technologies

    Joined:
    Jun 20, 2016
    Posts:
    742
    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?
     
    kirbygc00 likes this.
  5. deivsky

    deivsky

    Joined:
    Nov 3, 2019
    Posts:
    86
    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 :D.

    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 :).
     
    kirbygc00 likes this.
  6. WagnerGFX

    WagnerGFX

    Joined:
    Aug 26, 2013
    Posts:
    27
    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.
     
    kirbygc00 and deivsky like this.
  7. kirbygc00

    kirbygc00

    Joined:
    Apr 4, 2016
    Posts:
    46
    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!
     
  8. cirocontinisio

    cirocontinisio

    Unity Technologies

    Joined:
    Jun 20, 2016
    Posts:
    742
    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?
     
    deivsky likes this.
  9. Neonage

    Neonage

    Joined:
    May 22, 2020
    Posts:
    203
    It should've been in a separate branch from the start :p
     
  10. Harsh-099

    Harsh-099

    Joined:
    May 1, 2020
    Posts:
    186
    Oh, I thought it was happening because of something mine mistake. I will try it after the pull request gets merged.
     
  11. WagnerGFX

    WagnerGFX

    Joined:
    Aug 26, 2013
    Posts:
    27
    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? :D
    At least I'm out of ideas.
     
  12. deivsky

    deivsky

    Joined:
    Nov 3, 2019
    Posts:
    86
    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.
     
  13. cirocontinisio

    cirocontinisio

    Unity Technologies

    Joined:
    Jun 20, 2016
    Posts:
    742
    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!
     
  14. WagnerGFX

    WagnerGFX

    Joined:
    Aug 26, 2013
    Posts:
    27
    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.
     
    deivsky likes this.
  15. Harsh-099

    Harsh-099

    Joined:
    May 1, 2020
    Posts:
    186
    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?
     
  16. cirocontinisio

    cirocontinisio

    Unity Technologies

    Joined:
    Jun 20, 2016
    Posts:
    742
    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.
     
  17. WagnerGFX

    WagnerGFX

    Joined:
    Aug 26, 2013
    Posts:
    27
    Issue time! :D

    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?
     
  18. deivsky

    deivsky

    Joined:
    Nov 3, 2019
    Posts:
    86
    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.
     
    WagnerGFX likes this.
  19. WagnerGFX

    WagnerGFX

    Joined:
    Aug 26, 2013
    Posts:
    27
    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
    :D

    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.
     
    deivsky likes this.
  20. Harsh-099

    Harsh-099

    Joined:
    May 1, 2020
    Posts:
    186
    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.
     
    Last edited: Nov 10, 2020
  21. WagnerGFX

    WagnerGFX

    Joined:
    Aug 26, 2013
    Posts:
    27
    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? :D
     
  22. Harsh-099

    Harsh-099

    Joined:
    May 1, 2020
    Posts:
    186
    So should i delete the "feature-enemy" branch (after moving changes from it) or not?
     
  23. WagnerGFX

    WagnerGFX

    Joined:
    Aug 26, 2013
    Posts:
    27
    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.
     
  24. deivsky

    deivsky

    Joined:
    Nov 3, 2019
    Posts:
    86
    Yes, please!!! :p
     
  25. javimtib92

    javimtib92

    Joined:
    Aug 15, 2020
    Posts:
    7
    @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.
     
  26. Harsh-099

    Harsh-099

    Joined:
    May 1, 2020
    Posts:
    186
    What is meant by "cherry pick"?
     
  27. shuttle127

    shuttle127

    Joined:
    Oct 1, 2020
    Posts:
    180
  28. cirocontinisio

    cirocontinisio

    Unity Technologies

    Joined:
    Jun 20, 2016
    Posts:
    742
    Just merged the two PRs from @WagnerGFX - regarding caching and Editor assembly.
     
    deivsky and WagnerGFX like this.
  29. cirocontinisio

    cirocontinisio

    Unity Technologies

    Joined:
    Jun 20, 2016
    Posts:
    742
    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.

    upload_2020-11-15_21-6-36.png

    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
    upload_2020-11-15_21-5-42.png
     
  30. cirocontinisio

    cirocontinisio

    Unity Technologies

    Joined:
    Jun 20, 2016
    Posts:
    742
    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.
     
  31. WagnerGFX

    WagnerGFX

    Joined:
    Aug 26, 2013
    Posts:
    27
    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.
     
    cirocontinisio likes this.
  32. cirocontinisio

    cirocontinisio

    Unity Technologies

    Joined:
    Jun 20, 2016
    Posts:
    742
    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... :D
     
  33. WagnerGFX

    WagnerGFX

    Joined:
    Aug 26, 2013
    Posts:
    27
    (Butting in :p) 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.
     
    cirocontinisio likes this.
  34. deivsky

    deivsky

    Joined:
    Nov 3, 2019
    Posts:
    86
    Hi, sorry I haven't been so active, I'm moving out this week so it's been super busy lately :oops:.

    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!
     
    Last edited: Nov 16, 2020
    WagnerGFX likes this.
  35. deivsky

    deivsky

    Joined:
    Nov 3, 2019
    Posts:
    86
    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.
     
    cirocontinisio and WagnerGFX like this.
  36. cirocontinisio

    cirocontinisio

    Unity Technologies

    Joined:
    Jun 20, 2016
    Posts:
    742
    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.
     
    deivsky likes this.
  37. cirocontinisio

    cirocontinisio

    Unity Technologies

    Joined:
    Jun 20, 2016
    Posts:
    742
    Hey all, I just pushed some refactoring to the state-machine-improvements branch. Just not to push it to main :D
    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)
     
  38. deivsky

    deivsky

    Joined:
    Nov 3, 2019
    Posts:
    86
    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.
     
  39. treivize

    treivize

    Joined:
    Jan 27, 2016
    Posts:
    135
    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.
     
  40. deivsky

    deivsky

    Joined:
    Nov 3, 2019
    Posts:
    86
    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.
     
    WagnerGFX and treivize like this.
  41. cirocontinisio

    cirocontinisio

    Unity Technologies

    Joined:
    Jun 20, 2016
    Posts:
    742
    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.
     
    deivsky likes this.
  42. deivsky

    deivsky

    Joined:
    Nov 3, 2019
    Posts:
    86
    :eek: I thought it was enough with the onChangedCallback!
    Code (CSharp):
    1. reorderableList.onChangedCallback += list => list.serializedProperty.serializedObject.ApplyModifiedProperties();
     
  43. cirocontinisio

    cirocontinisio

    Unity Technologies

    Joined:
    Jun 20, 2016
    Posts:
    742
    No, I think that callback is only if "the list" changed, not what is contained in it :)
    (this page seems to confirm)
     
    deivsky likes this.
  44. treivize

    treivize

    Joined:
    Jan 27, 2016
    Posts:
    135
    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?
     
  45. deivsky

    deivsky

    Joined:
    Nov 3, 2019
    Posts:
    86
    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.
     
  46. cirocontinisio

    cirocontinisio

    Unity Technologies

    Joined:
    Jun 20, 2016
    Posts:
    742
    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!
     
  47. deivsky

    deivsky

    Joined:
    Nov 3, 2019
    Posts:
    86
    It's never enough! :p

    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.
     
  48. cirocontinisio

    cirocontinisio

    Unity Technologies

    Joined:
    Jun 20, 2016
    Posts:
    742
    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!
     
  49. cirocontinisio

    cirocontinisio

    Unity Technologies

    Joined:
    Jun 20, 2016
    Posts:
    742
    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?! :D

    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
     
  50. Neonage

    Neonage

    Joined:
    May 22, 2020
    Posts:
    203
    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?
    upload_2020-11-23_8-25-31.png
    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
     
    Last edited: Nov 23, 2020
unityunity