Furthermore, preview control points are shown in figures with a fixed number of control points (e.g. line, circle). Those preview control points should be visible on mitkPlanarPolygon and mitkPlanarSubdivisionPolygon only.

New remote branch pushed: bug-15856-planar-circle-figure-fix

First of all, thank you for working on this bug. I admit that I have not tried to move a whole PlanarCircle by moving the central point.

REVIEW:

• Please change the commit message to something a bit more meaningful
• The code you added to IsPositionOverFigure() to check if the cursor is above a control-point
  • is inside the for-loop over all PolyLines. Thus, you check all points several times although a single time would be enough!
  • returns the id of the first control-point ('0') even if it's not the first control-point the cursor is currently hovering above
  • is more or less a copy of the method IsPositionInsideMarker() that is located just below IsPositionOverFigure().

It should be possible to use the method IsPositionInsideMarker() (in addition to IsPositionOverFigure()) where IsPositionOverFigure() is called. Calling both methods shoudl result in the same logic as your additional code without adding additional code =)

Just for your information:
It does no longer matter if an action (the method that actually implements the action) returns true or false. If all conditions of a transition are fulfilled, the statemachine will move on into the next state. I just say it because you changed some 'return false;' to 'return true;'.

The rest looks fine to me.

New remote branch pushed: bug-15856-planar-circle-fix-v2

ok, thanks for the review: it was a cut and paste error inside an existing loop

bug-15856-planar-circle-fix-v2 has the latest changes (made a mistake with git and have to start a new branch)

will merge it into master

regards,
sebastian

I just had a look at the new branch.

You fixed the error with the wrong control-point ID and moved the new code outside of the for-loop.

But it is still duplicated code! The exact same code already exists in the method IsPositionInsideMarker().

Please check where IsPositionOverFigure() called and use the additional method IsPositionInsideMarker():

e.g. if ( IsPositionOverFigure(...) && IsPositionInsideMarker(...) )
{

do_something...

}

If I remember correctly, IsPositionOverFigure() is called quite often and I do not think that you have to check for point-hovering in all those cases.

Please check in which cases it is really needed to keep the additional checking for point-hovering to a minimum.

[4790cb]: Merge branch 'bug-15856-planar-circle-fix-v2'

Merged commits:

2013-08-14 17:00:52 Sebastian Ordas [824d2f]
allowed for selecting the central control point in planar
circle figures

do not show preview control points in figures that are not extendible

sorry, this time i will look into it more carefully

sorry, busy elsewhere ...

how about now? (bug-15856-planar-circle-fix-v2)

OK, I think now I understand what you are trying to do here.

Your latest changes look better but I think there is a more elegant way by implementing the additional logic in the statemachine XML definition.

You see, there are two separate states in the statemachine for hovering:

• HoveringOverFigure: the cursor hovers above the lines of the figure
• HoveringOverPoint: the cursor hovers above a control-point

By adding your additional check into 'HoveringOverFigure', you merge the two states. I think the problem here was that there was no transition that lead from the state 'EditFigure' (no hovering at all) to the state 'HoveringOverPoint' (which you have to be in to move the point). You first had to hover above the line, which was not possible in case of the center point.

I think the most elegant way to fix this is to add an additional transition to the state 'EditFigure' that leads to 'HoveringOverPoint'. I have made the changes in 'bug-15856-planar-circle-fix-v2'. Could you take a look at it and tell me if this fixes your problem?

yes, that would be the right way! please go ahead and merge it.

something else: I think we should show the control points and helper lines ONLY when the user hovers on the figure? In that way, figures would be cleaner when they are not selected. what do you think? I can quickly push a fix for that.

wait, don´t merge. Circle dragging is again not possible for me. Let me have a look ....

You are right, I had forgotten to add a missing action. I pushed it, now it should work.

Concerning the hiding of control-points when we are not hovering:
This can actually be done right now. You can achieve this by setting the color/opacity settings for control-points. There is a logic that does not try to render in case of opacity == 0, so the performance-overhead will be minimal. I'll check which properties you have to set ...

ok. Now I got your change and pushed another fix. It is working for me. same for you?

the "return false" that I removed from CheckControlPointHovering in my last push was not being accessed anyway.

it is working "almost" fine for me now.

I will have a look at why sometimes a control point is not effectively selected on the first click on it. It is indeed on the second try.

Sorry for the late reply, I checked everything again. Seems to work fine for me now.

Feel free to merge the changes into master as soon as you have fixed your point-selection problems.

[b5032b]: Merge branch 'bug-15856-planar-circle-fix-v2'

Merged commits:

2013-08-16 15:11:57 Sebastian Ordas [fb7274]
more elegant approach

2013-08-15 11:25:02 Sebastian Ordas [af2347]
it was always returning false
fixed it

2013-08-15 11:02:14 Markus Engel [118fd8]
added missing action to select point

2013-08-15 09:41:55 Markus Engel [ab6e1c]
fixed issue by adding new transition to statemachine xml

2013-08-15 09:41:27 Markus Engel [fdcbdc]
restored old version of PlaanrFigureInteractor

2013-08-14 18:32:00 Sebastian Ordas [c59b11]
avoid code duplication and unnecessary checks for point-hovering

I will close this bug because planar circle is working fine.

Remaining issues in planar figure interactor will be fixed in T16077