New remote branch pushed: bug-15697-stateMachineConditions

I did a little code review of the published branch.

In general, I like the changes very much. However, I have a few questions / remarks:

1. Is the new StateMachineCondition class supposed to be used programmatically by external users (e.g. adding new conditions at runtime when building / modifying a statemachine manually)? If yes, there should also be a "RemoveCondition" method in the StateMachineTransition class. If no, the class shouldn't be exported and the Add/RemoveCondition methods should be private (and StateMachineContainer a friend).

2. The StateMachineCondition class would also be a good candidate for a POD class having value semantics. Or no class at all, just recording the id (name) and inverted property in the StateMachineTransition class itself.

3. If I understood it correctly, conditions are always tied to specific transitions. Maybe the "condition checking code" could be moved from the EventStateMachine class into the StateMachineTransition class, made accessible via some new method, e.g. bool StateMachineTransition::CheckConditions(InteractionEvent*).

4. When calling the delegators (e.g. the actual "checking functions"), what happens if the function throws an exception? Maybe we should try/catch(...) the Execute() call and consider any caught exception as a return value of "false".

5. There is an "AddConditionFunction" method in EventStateMachine, but no "RemoveConditionFunction".

6. The method mitk::StateMachineState::GetTransition() method was changed to mitk::StateMachineState::GetTransitionList(). Please restore the original method with a sensible implementation and mark it as deprecated (preprocessor macro as well as doxygen macro).

Thanks for working on that stuff!

(In reply to Sascha Zelzer from comment #2)

2. The StateMachineCondition class would also be a good candidate for a POD

class having value semantics. Or no class at all, just recording the id
(name) and inverted property in the StateMachineTransition class itself.

Since we probably want the id as a std::string member, forget about the POD type remark (sorry). The class could still be designed to have value semantics (not inheriting from itk::LightObject at all).

Thank you for the great review! I have taken each point into consideration and tried to adapt the changes accordingly:

1. Is the new StateMachineCondition class supposed to be used

programmatically by external users (e.g. adding new conditions at runtime
when building / modifying a statemachine manually)? If yes, there should
also be a "RemoveCondition" method in the StateMachineTransition class. If
no, the class shouldn't be exported and the Add/RemoveCondition methods
should be private (and StateMachineContainer a friend).

No, the conditions should not be modified by the user. Removal of transitions/actions/conditions is also not supported. The StateMachine is constructed once when initializing the Interactor and will not be modified afterwards. Thus I removed the exporting for the StateMachineActions and StateMachineConditions and made the methods AddAction() and AddCondition() private. The two classes StateMachineFactory and StateMachineContainer are friend classes now.

2. The StateMachineCondition class would also be a good candidate for a POD

class having value semantics. Or no class at all, just recording the id
(name) and inverted property in the StateMachineTransition class itself.

I have slimmed down the StateMachineCondition. It's no longer derived from itk::LightObject. I have replaced all uses of its SmartPointer with 'const StateMachineCondition*'. That's OK because the conditions are not modified after construction.

3. If I understood it correctly, conditions are always tied to specific

transitions. Maybe the "condition checking code" could be moved from the
EventStateMachine class into the StateMachineTransition class, made
accessible via some new method, e.g. bool
StateMachineTransition::CheckConditions(InteractionEvent*).

In fact, conditions can be reused by different transitions. Thus they cannot be moved to the StateMachineTransition. Furthermore, the methods that implement the conditions are implemented in the Interactors that derive from the EventStateMachine. Thus it makes sense to implement the check in this baseclass.

4. When calling the delegators (e.g. the actual "checking functions"), what

happens if the function throws an exception? Maybe we should try/catch(...)
the Execute() call and consider any caught exception as a return value of
"false".

I have added exception handling for the execution of both conditions and actions. In both cases an exception is handled like a 'false'.

5. There is an "AddConditionFunction" method in EventStateMachine, but no

"RemoveConditionFunction".

As I stated above, actions and conditions are constant for an interactor. Thus it makes no sense to disconnect a method that implements a certain condition from its condition in the XML pattern.

6. The method mitk::StateMachineState::GetTransition() method was changed to

mitk::StateMachineState::GetTransitionList(). Please restore the original
method with a sensible implementation and mark it as deprecated
(preprocessor macro as well as doxygen macro).

Sorry, I had forgotten about the deprecated macro. I have restored the original method and re-implemented it. Calling GetTransition() will internally call GetTransitionList() and return the first transition in the list (NULL if list is empty). If there is more than one transition available for the given event, a warning is issued in the log-file.
Thus, it is still possible to implement interactors 'the old way' without the conditions.

I'd appreciate if you could take another look at the changes I have made recently.

Here it comes ;-) Thanks for working on it.

(In reply to Markus Engel from comment #4)

1. Is the new StateMachineCondition class supposed to be used

programmatically by external users (e.g. adding new conditions at runtime
when building / modifying a statemachine manually)? If yes, there should
also be a "RemoveCondition" method in the StateMachineTransition class. If
no, the class shouldn't be exported and the Add/RemoveCondition methods
should be private (and StateMachineContainer a friend).

No, the conditions should not be modified by the user. Removal of
transitions/actions/conditions is also not supported. The StateMachine is
constructed once when initializing the Interactor and will not be modified
afterwards. Thus I removed the exporting for the StateMachineActions and
StateMachineConditions and made the methods AddAction() and AddCondition()
private. The two classes StateMachineFactory and StateMachineContainer are
friend classes now.

In principal, sounds good to me. However, removing the MITK_CORE_EXPORT macro from classes already contained in a previous MITK release will also break third-party code. If the following classes should be removed from the public MITK API, please mark them as deprecated but keep the MITK_CORE_EXPORT macro. We can remove the macro after the next release:

• StateMachineState
• StateMachineAction
• StateMachineTransition

Additionally, if the StateMachineTransition class is not going to be exported, using private methods and friend classes doesn't make so much sense any more (except for protecting ourselves against internal API misuse).

2. The StateMachineCondition class would also be a good candidate for a POD

class having value semantics. Or no class at all, just recording the id
(name) and inverted property in the StateMachineTransition class itself.

I have slimmed down the StateMachineCondition. It's no longer derived from
itk::LightObject. I have replaced all uses of its SmartPointer with 'const
StateMachineCondition*'. That's OK because the conditions are not modified
after construction.

Removing the itk::LightObject looks good to me. Since the StateMachineTransition class takes ownership of all StateMachineCondition instances passed via "AddCondition(const StateMachineCondition*)" (it deletes them in its destructor), I think changing that method to "AddCondition(const StateMachineCondition&) would clarify memory management handling and also avoids expensive "new StateMachineCondition" calls in StateMachineContainer (we would rely on the compiler generated copy constructor and assignment operator). This is what I meant with the class having "value semantics".

3. If I understood it correctly, conditions are always tied to specific

transitions. Maybe the "condition checking code" could be moved from the
EventStateMachine class into the StateMachineTransition class, made
accessible via some new method, e.g. bool
StateMachineTransition::CheckConditions(InteractionEvent*).

In fact, conditions can be reused by different transitions. Thus they cannot
be moved to the StateMachineTransition. Furthermore, the methods that
implement the conditions are implemented in the Interactors that derive from
the EventStateMachine. Thus it makes sense to implement the check in this
baseclass.

I think you are talking about the condition callbacks implemented in the interactor classes and I was talking about StateMachineCondition instances. If StateMachineCondition instances were to be shared among StateMachineTransition instances, we would have a memory ownership problem now (because of the raw pointer usage).

However, I can see now why the code is in EventStateMachine, because I didn't think about the association of the condition name with the condition callback, stored in EventStateMachine::m_ConditionDelegatesMap. I agree that currently the code is placed best in EventStateMachine.

I know that this was originally not in the scope of the planned changes, but if you like the following idea, I think it leads to a cleaner and simpler design: The EventStateMachine::AddConditionFunction(...) (and similarly the AddActionFunction method) could call a new StateMachineTransition::AddConditionFunction(...) method, such that each transition contains its own "m_conditionDelegatesMap" map - which would just be a vector then - (the EventStateMachine::AddConditionFunction() could easily look up the transition instance by name), instead of storing all delegates in EventStateMachine. This would allow to move the "condition checking" code also into the mitk::StateMachineTransition class and lead to a better "separation of concerns" and less EventStateMachine code (in my opinion). Also, the StateMachineTransition::GetConditions() method wouldn't be needed any more.

4. When calling the delegators (e.g. the actual "checking functions"), what

happens if the function throws an exception? Maybe we should try/catch(...)
the Execute() call and consider any caught exception as a return value of
"false".

I have added exception handling for the execution of both conditions and
actions. In both cases an exception is handled like a 'false'.

Great. But you actually almost always would want to catch by const reference, not by value (catching by value leads to object slicing and stripping off possibly inherited functionality). Since we don't do anything with the thrown object, you should catch *everything* by using "catch with ellipsis", i.e. catch(...)

7. There is a copy & paste error in the warning message in StateMachineContainer::StartElement() in the CONDITION branch. It referes to an "action", but should refer to a "condition".

(In reply to Sascha Zelzer from comment #5)

Here it comes ;-) Thanks for working on it.

(In reply to Markus Engel from comment #4)

1. Is the new StateMachineCondition class supposed to be used

programmatically by external users (e.g. adding new conditions at runtime

...

private. The two classes StateMachineFactory and StateMachineContainer are
friend classes now.

In principal, sounds good to me. However, removing the MITK_CORE_EXPORT
macro from classes already contained in a previous MITK release will also
break third-party code. If the following classes should be removed from the
public MITK API, please mark them as deprecated but keep the
MITK_CORE_EXPORT macro. We can remove the macro after the next release:

• StateMachineState
• StateMachineAction
• StateMachineTransition

Additionally, if the StateMachineTransition class is not going to be
exported, using private methods and friend classes doesn't make so much
sense any more (except for protecting ourselves against internal API misuse).

OK, I had forgotten about that. I will add the Export-Macros for those classes, although I'm pretty sure that no user will directly use those classes.
I'm not sure about marking the classes as deprecated (as they are not).
Can you explain what you had in mind here?

2. The StateMachineCondition class would also be a good candidate for a POD

class having value semantics. Or no class at all, just recording the id
(name) and inverted property in the StateMachineTransition class itself.

I have slimmed down the StateMachineCondition. It's no longer derived from
itk::LightObject. I have replaced all uses of its SmartPointer with 'const
StateMachineCondition*'. That's OK because the conditions are not modified
after construction.

Removing the itk::LightObject looks good to me. Since the
StateMachineTransition class takes ownership of all StateMachineCondition
instances passed via "AddCondition(const StateMachineCondition*)" (it
deletes them in its destructor), I think changing that method to
"AddCondition(const StateMachineCondition&) would clarify memory management
handling and also avoids expensive "new StateMachineCondition" calls in
StateMachineContainer (we would rely on the compiler generated copy
constructor and assignment operator). This is what I meant with the class
having "value semantics".

Good point, I will change this. I had misunderstood your initial comment on this part.

3. If I understood it correctly, conditions are always tied to specific

transitions. Maybe the "condition checking code" could be moved from the

...

I think you are talking about the condition callbacks implemented in the
interactor classes and I was talking about StateMachineCondition instances.
If StateMachineCondition instances were to be shared among
StateMachineTransition instances, we would have a memory ownership problem
now (because of the raw pointer usage).

You are right, we talked about two different things here. The transitions share the callbacks, not the instances of the StateMachineConditions.

However, I can see now why the code is in EventStateMachine, because I
didn't think about the association of the condition name with the condition
callback, stored in EventStateMachine::m_ConditionDelegatesMap. I agree that
currently the code is placed best in EventStateMachine.

I know that this was originally not in the scope of the planned changes, but
if you like the following idea, I think it leads to a cleaner and simpler
design: The EventStateMachine::AddConditionFunction(...) (and similarly the
AddActionFunction method) could call a new
StateMachineTransition::AddConditionFunction(...) method, such that each
transition contains its own "m_conditionDelegatesMap" map - which would just
be a vector then - (the EventStateMachine::AddConditionFunction() could
easily look up the transition instance by name), instead of storing all
delegates in EventStateMachine. This would allow to move the "condition
checking" code also into the mitk::StateMachineTransition class and lead to
a better "separation of concerns" and less EventStateMachine code (in my
opinion). Also, the StateMachineTransition::GetConditions() method wouldn't
be needed any more.

This does look good, but there is the problem that the EventStateMachine does know which transition to give the conditionDelegate to. The transitions have no name or any kind of ID.
One possibility (that I do not find very nice) is that the EventStateMachine gives all transitions all conditionDelegates. Each instance of transition could check if it has a condition that matches the name of the conditionDelegate and store it. As I said, I do not like that approach.

4. When calling the delegators (e.g. the actual "checking functions"), what

happens if the function throws an exception? Maybe we should try/catch(...)
the Execute() call and consider any caught exception as a return value of
"false".

I have added exception handling for the execution of both conditions and
actions. In both cases an exception is handled like a 'false'.

Great. But you actually almost always would want to catch by const
reference, not by value (catching by value leads to object slicing and
stripping off possibly inherited functionality). Since we don't do anything
with the thrown object, you should catch *everything* by using "catch with
ellipsis", i.e. catch(...)

consider it done...

7. There is a copy & paste error in the warning message in

StateMachineContainer::StartElement() in the CONDITION branch. It referes to
an "action", but should refer to a "condition".

fixed..

OK, I had forgotten about that. I will add the Export-Macros for those classes,
although I'm pretty sure that no user will directly use those classes.
I'm not sure about marking the classes as deprecated (as they are not).
Can you explain what you had in mind here?

Removing them from the public API by removing the export macro effectively
means "deprecating" the classes (for external users). What we do internally
with them, is of course completely up to us. So in my opinion, it is okay to
mark them deprecated, even in this case. And you can never be sure if
someone is using these classes or not. Besides, it is just good practice
this way ;-)

However, I can see now why the code is in EventStateMachine, because I
didn't think about the association of the condition name with the condition
callback, stored in EventStateMachine::m_ConditionDelegatesMap. I agree that
currently the code is placed best in EventStateMachine.

I know that this was originally not in the scope of the planned changes, but
if you like the following idea, I think it leads to a cleaner and simpler
design: The EventStateMachine::AddConditionFunction(...) (and similarly the
AddActionFunction method) could call a new
StateMachineTransition::AddConditionFunction(...) method, such that each
transition contains its own "m_conditionDelegatesMap" map - which would just
be a vector then - (the EventStateMachine::AddConditionFunction() could
easily look up the transition instance by name), instead of storing all
delegates in EventStateMachine. This would allow to move the "condition
checking" code also into the mitk::StateMachineTransition class and lead to
a better "separation of concerns" and less EventStateMachine code (in my
opinion). Also, the StateMachineTransition::GetConditions() method wouldn't
be needed any more.

This does look good, but there is the problem that the EventStateMachine does
know which transition to give the conditionDelegate to. The transitions have no
name or any kind of ID.
One possibility (that I do not find very nice) is that the EventStateMachine
gives all transitions all conditionDelegates. Each instance of transition could
check if it has a condition that matches the name of the conditionDelegate and
store it. As I said, I do not like that approach.

Hm, maybe we need to talk about this in person. As I tried to explain, each
transition would fill its own list of condition delegates when the (new)
StateMachineTransition::AddConditionFunction() method is called by the event
state machine (implicitly via the macro calls). So each
transition instance would hold its associated condition delegates (the event
state machine would no longer have to manage the association of transition
instances to condition delegates). As the event state machine knows which
transition currently is to be evaluated, it could then just call the (new)
StateMachineTransition::CheckConditions(...) method.

All that said, I am happy to see the other changes merged into master,
without any changes related to the things just discussed above. We could
talk it over F2F later.

OK, I have changed the code according to your review.

I have added the export macros again to maintain the compatibility for third-party code.
Conditions are now given to the transition via const references.

We can talk about the other two issues when we see each other again some other time..

Reopen to push some changes that were made after review with Daniel.

I have further improved the code and extended the documentation.

The checking of conditions has been moved to a distinct method to improve readability and reduce complexity of the HandleEvent().

This method iterates over all possible transitions (possible for an incoming event) and checks all conditions. As it is possible that all transitions share conditions, I have added a caching for conditions that have already been evaluated, thus saving us some time when many transitions with similar conditions are involved.

I have tested the conditions quite extensively during the portation of the PlanarFigureInteractor (see T15546). So far, everthing works like a charm.

The changes have been reviewed by both Sascha and Daniel. Most of their concerns have already been adressed.

I have added two new bugs:

• T15797: issue that Sascha brought up in comment #5
• T15796: add missing consistency check for statemachine patterns

[8f7e44]: Merge branch 'bug-15697-stateMachineConditions'

Merged commits:

2013-08-07 14:17:57 Markus Engel [b77739]
moved checking of conditions to new method, results are cached for potential following checks

2013-08-07 11:36:15 Markus Engel [f157b9]
removed default param in c'tor, improved docu

2013-08-05 14:38:38 Markus Engel [0ab842]
using const condition& instead of pointers

2013-08-05 14:37:59 Markus Engel [67ab6b]
added export macros again to maintain compartibility

2013-07-30 16:33:31 Markus Engel [0feaac]
added new condition to DisplayInteractor

2013-07-30 16:32:39 Markus Engel [3e9436]
added try-catch for execution of conditions and actions

2013-07-30 16:32:09 Markus Engel [c06e9c]
using const StateMachineCondition* instead of SmartPointer

2013-07-30 16:29:57 Markus Engel [55ec82]
no longer exporting internal classes

2013-07-30 16:28:27 Markus Engel [e74613]
slimmed down StateMachineCondition, it's no longer a itk::LightObject, no longer exported

2013-07-30 16:27:36 Markus Engel [7aec42]
restored method GetTransition() as deprecated method

2013-07-24 16:09:11 Markus Engel [5dbe70]
removed unnecessary includes from DataInteractor

2013-07-24 15:51:24 Markus Engel [c9662e]
now considering conditions in the execution of transitions

2013-07-24 15:32:31 Markus Engel [d04e2e]
States can have a list of transitions for one event

2013-07-24 15:20:13 Markus Engel [3b4190]
stateMachineContainer now constructs StateMachineConditions from XML

2013-07-24 15:18:40 Markus Engel [323ca4]
transitions also hold a list of necessary conditions

2013-07-24 15:13:46 Markus Engel [5b621f]
added new class mitkStateMachineCondition

[cde4c2]: COMP: Merge branch 'bug-15697-stateMachineConditions'

Merged commits:

2013-08-07 16:36:36 Markus Engel [e7b41b]
removed const in std::vector to satisfy gcc

The implementation of the conditions is finished, Dashboard is green.