Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some fixes and new features #9

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

mfandreich
Copy link

Hi! First of all thx for this great tool. And sorry for my bad english =)

I found some bugs (only one to be honnest) and missconception and try fix this in this PR.

  1. Only OnLogic can be working method of states. Its little bit strange, because sometimes (character controller for example) we need use additional methods like OnAnimatorMove or OnCollisionEnter and others. In FSM pattern we usualy use main object as facade and redirect calls to states. In situation when we can use only OnLogic its cant be done in intuitive way. I try solve this with adding OnCommand and SetCommandHandler methods. Its generic methods and we can use any type as "command argument". I hope this change will be useful to you.
  2. I fix minor bug with AddTransitionFromAny. Its produce "Null reference exception" when TStateId is refence type.
  3. I add IEqualityComparrer to state machine constructor for TStateId and TEvent. Its needed in 2 cases. First: custom look up logic. Sometimes its can be very useful. Second: garbage collecting reduce when using struct types as key for dictionaries.

Thats all =)

@Inspiaaa
Copy link
Owner

Thanks a lot for your contribution to UnityHFSM!

Point 2 was a good catch.

For point 1, could you give me a quick example of a use case and show me how your code would be used for this example.

Point 3 also sounds really interesting, especially the GC reduction. Can you share a code example that demonstrates a use case and the advantage for this as well, please.

Thanks again for contributing! 👍 Much appreciated.

@mfandreich
Copy link
Author

mfandreich commented Sep 19, 2021

Hi. No problem. Thats samples:

For first point as example I show how we can process standart unity messages (root motion in this case)

public struct OnAnimatorMoveCommand
{
    public readonly Transform transform;
    public readonly Animator animator;

    public OnAnimatorMoveCommand(Transform transform, Animator animator)
    {
        this.transform = transform;
        this.animator = animator;
    }
}

public class StateCommandSample: MonoBehaviour
{
    private StateMachine fsm;
    private Animator animator;

    private void Start()
    {
        animator = GetComponent<Animator>();
        fsm = new StateMachine();

        var rootMotionState = new State(commands:new CommandBase<string>[]
        {
            new Command<OnAnimatorMoveCommand>((state, command) =>
            {
                command.transform.position += animator.deltaPosition;
            })
        });
        var noRootMotionState = new State(commands:new CommandBase<string>[]
        {
            new Command<OnAnimatorMoveCommand>((state, command) =>
            {
                //do nothing we dont need root motion here or we can not register this command for state at all
            })
        });
        
        //Here we can add some transitions and other fsm stuff
        
        fsm.Init();
    }

    private void Update()
    {
        //Here we process main logic, transitions as usual
        fsm.OnLogic();
    }

    private void OnAnimatorMove()
    {
        //Here we process root motion based on current state
        //its very useful for complex controllers based on fsm 
        fsm.OnCommand(new OnAnimatorMoveCommand(transform, animator));
    }
}

For third all very simple, in code you using this:

if (transition.to.Equals(activeState.name))
	continue;

But TStateId its generic argument. Compiler dont know how work with it and this code can produce boxing (two times) every calling. First - call virtual method Equals on generic arg and box activeState.name because equals get object arg instead of TStateId.
If we using IEqualityComparer we dont have this problem. Other problems for generics dictionaries its using struct as key (rare but possible). Dictionaries with struct key lookup produce garbage if IEqualityComparer not attached.

Thats sample:

public struct CustomKey
{
    private sealed class EqualityComparer : IEqualityComparer<CustomKey>
    {
        public bool Equals(CustomKey x, CustomKey y)
        {
            return x.id == y.id && x.additionalId == y.additionalId && x.somethingWeird.Equals(y.somethingWeird);
        }

        public int GetHashCode(CustomKey obj)
        {
            unchecked
            {
                var hashCode = obj.id;
                hashCode = (hashCode * 397) ^ obj.additionalId.GetHashCode();
                hashCode = (hashCode * 397) ^ obj.somethingWeird.GetHashCode();
                return hashCode;
            }
        }
    }

    public static IEqualityComparer<CustomKey> Comparer { get; } = new EqualityComparer();

    public int id;
    public long additionalId;
    public Guid somethingWeird;
}

//And in code we can use this
var garbageProducer = new StateMachine<CustomKey>();
var garbageFree = new StateMachine<CustomKey>(stateIdComparer:CustomKey.Comparer);

With equality comparers also we can change "equal" behaviour as we need on our logic. For example comapre two objects not by reference, but by some field values.

@JoelCreatesGames
Copy link

This is a very interesting PR

I definitely agree that point 2 and 3 are quite useful (and maybe should be separated out as separate PRs)

I'm still thinking on how 1 could be used. From your example it seems like it is "state machine backed callbacks". So you trigger the command, and the state machine calls the correct one based on the state it is. This seems like a more performant option than in OnLogic checking some state to determine if it should call the function. After writing this out I could see uses for it 🤔 I wonder if Command is the right verbiage here (though can't think of anything better).

@JoelCreatesGames
Copy link

After looking at the recent commits, seems this has been added with OnAction (but yet to be documented) awesome!

@CodeAnthem
Copy link

I am wondering, was parts of this merged? I tried myself to merge it locally, but figured the current version of UnityHFSM is newer and reverted the changes, I wonder if the changes by @mfandreich are worth of taking into account.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants