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

Can we merge BehaviourEnabledObserver and RulesMatcher? #321

Open
bddckr opened this issue Mar 10, 2019 · 2 comments
Open

Can we merge BehaviourEnabledObserver and RulesMatcher? #321

bddckr opened this issue Mar 10, 2019 · 2 comments
Labels
enhancement Making something better

Comments

@bddckr
Copy link
Contributor

bddckr commented Mar 10, 2019

BehaviourEnabledObserver definitely should take a Rule instead, at which point it's very similar to RulesMatcher. Perhaps the repeated invoking can be done as a standalone component that repeatedly calls an IProcessable.Process until canceled?

@bddckr bddckr added the enhancement Making something better label Mar 10, 2019
@bddckr
Copy link
Contributor Author

bddckr commented Mar 11, 2019

  • BehaviourEnabledObserver == IProcessable + a new IRule that does a list check for all behaviours being enabled
  • RulesMatcher
  • this new "ProcessCaller"

@fight4dream
Copy link
Contributor

Before
image

After
image

Initial thoughts and decisions involved:

RulesMatcher

RulesMatcher matches a list of rules against the same object (behaviour), whereas BehaviourEnabledObserver matches a single rule (isActiveAndEnabled) against all behaviour in the list. So they are not quite compatible.

a new IRule that does a list check for all behaviours being enabled

IRule interface only Accept(object) a single object. It is incompatible with matching each behaviour in the list. Sounds like a ICollectionRule.Accept(ObservableList).

IsActiveAndEnabledRule

IsActiveAndEnabledRule was the first thing I write. In retrospect, the current BehaviourEnabledObserver2 can use any rule, so it is actually not that specific and more like a BehaviourRuleMatcher. I'm not sure about the difference between RulesMatcher and AllRules but I guess it depends on usage. Anyway, we can keep it specific and check isActiveAndEnabled inside the observer just like the previous version.

bddckr commented that:

The issue now is

  1. Users have to add many more components
  2. All prefabs need to be updated 🙂

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

No branches or pull requests

2 participants