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

add Clone on triggers if available #8

Merged
merged 2 commits into from
Oct 27, 2023
Merged

add Clone on triggers if available #8

merged 2 commits into from
Oct 27, 2023

Conversation

hyranno
Copy link
Contributor

@hyranno hyranno commented Oct 12, 2023

This PR add Clone and some common traits onto triggers and some values for convenience.
It is good for reusing triggers.

Implementing Clone on StateMachine will be convenient too, but it will conflict with pending PR (#7 ), so I won't implement until it is closed (weather merged or not).

@Seldom-SE
Copy link
Owner

StateMachine used to implement Clone, but I removed it so that triggers don't have to implement Clone. I recommend this pattern:

let state_machine_builder = || {
    StateMachine::default()
        .trans<MyState>(MyTrigger, MyOtherState)
};

commands.spawn((
    MyState,
    state_machine_builder(),
));

commands.spawn((
    MyOtherState,
    state_machine_builder(),
));

Or break it out into a separate function. I'd accept the impls that are here, though. But I'll wait to see what you think first.

@hyranno
Copy link
Contributor Author

hyranno commented Oct 26, 2023

Sounds reasonable.
It is good to allowing things to be not Clone, and it looks hard to implement Clone for StateMachine without forcing its member to be Clone.
I expected these merits to implement Clone for StateMachine:

  • Clone will be faster than repeated initializer calls.
  • It will be easier to implement Clone for impl Bundle which contains StateMachine.

But I think these merits are less important.

@Seldom-SE Seldom-SE merged commit 57fac48 into Seldom-SE:main Oct 27, 2023
4 checks passed
@Seldom-SE
Copy link
Owner

Thanks!

@hyranno hyranno deleted the derive_convinience branch October 28, 2023 00:06
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.

2 participants