Page MenuHomePhabricator

Undo functionality on planar figures would be useful.
Closed, WontfixPublic

Description

As the summary states.

Event Timeline

A patch for this has been proposed by rostislav at [1].

However that patch includes several API changes which need to be discussed. A copy of the discussion so far can be found at [2]

[1] https://github.com/MITK/MITK/pull/113

[2]
Our concerns and his answers:

we just had a look at your planar figure pull request. We do see
some issues with using the commit as-is:

  1. It's a very complex commit containing some distinct fixes and

feature implementations which would better be separated into
appropriate commits.

Well, honestly, the majority is related to the undo/redo, so it's
kind of difficult to separate the commits. I'll take a look though.

  1. It contains fixes regarding adding points to the subdivision

planar figure, however, we already fixed this (with a little extra
help today), see T18859.

Yes. I noticed that there was the GetControlPointForPolylinePoint
function, but it is incorrect (probably you fixed it) and horribly
inefficient. Could you perhaps show me the new code for it (I assume
this is the function you changed) - so I could just remove my
substitute?

  1. There is a lot of non-obvious stuff happening like deletion of

public API methods, introduction of new methods to planar figure,
and even renaming of ellipse properties which would cause big
troubles for some of our users like mint medical.

I will describe the rationale for this a bit later (or maybe with
some documentation).

  1. A few formatting issues like trailing whitespaces and missing

empty lines at the end of files (we would need to amend such commits
before
merging)

Trailing whitespaces could be fixed by clang-format - could you give
me your format file for that? I will manually add the newlines where
necessary

  1. Connected to (1): This commit contains more than adding undo/redo

for planar figures

See (1) :)

We'd like to ask you if you could take care of these issues and to
clarify the - for us ;) - obscure stuff?

More comments by Rostislav:

  1. GetPolyLine and GetHelperPolyLine (non-const versions) - I believe that conceptually these are const-functions. The reason why there was a non-const version is that they are evaluated lazily, i.e. only when the polyline is requested. The non-const version used to check the flag if it was dirty and call GeneratePolyLine() if not. The const version would simply return the polyline from the array, meaning that there is a potential to get the polyline that is inconsistent with the current control points, which I think is incorrect behavior. To avoid this, removed the non-const version and made the const version check the flag.. Unfortunately it involved a const-cast, but in this case I preferred this solution because otherwise it would be a huge pain to change all functions related to polyline generation const and all the variables related to that mutable. I also believe that for lazy evaluation, this const cast is not a big design flaw. Additionally, I made the flags themselves mutable and overriden the Modified() method to set all the dirty flags which essentially saves a lot of pain for writing functions modifying the planar figures - no need to remember to set the dirty flags at every point where it is needed in addition to calling Modified().
  1. I removed EvaluateFeatures for the same reason. This function is there only because of lazy evaluation. I don't believe that lazy evaluation should affect the conceptual constness of access methods.

Therefore, I made it so only when the quantity is requested, the features are evaluated (i.e. when the flag shows dirty state).

  1. ResetNumberOfControlPoints in my opinion is a dirty hack. It was there for the figure placement - i.e. to show hoe many points should be filled when the figure is placed. I mean, even the comment in this function shows that something's wrong: "// DO NOT resize the list here, will cause crash!!". The fact that the number of control points and the

vector(!) containing these points can be different is very error-prone and unintuitive. And indeed I had huge troubles with this when implementing undo/redo. I mean, this information is only available when a new planar figure is created and this number is set in a constructor.
I substituted this by adding two virtual methods to the planar figure:
GetPlacementNumberOfControlPoints() and GetPlacementSelectedPointId() - hope the names speak for themselves.

Now, regarding the Is/SetFinalized() methods. To me it is highly strange that the m_Placed is a flag in the planar figure itself, while whether it is finalized or not is a boolean base data property with a specific name. Again, for consistency sake I replaced that with a flag in a planar figure. In case you are worried that it will break your clients'
code, we could remove the flag and make the Is/SetFinalized() methods to consult/set this property - I don't like it, but I completely understand that we don't live in vacuum, even though changes to the code are trivial (find all the places where "initiallyplaced" is mentioned and replace it with a corresponding function call). It doesn't affect the loading of saved figures so it doesn't break loading of old scene files.

Finally, regarding editing the subdivision polygon - and for that matter, bezier curve. I would really like to avoid dynamic_cast to specific planar figure types. For bezier curve, I think we could easily avoid adding new points by just reporting the maximum number of points being current number of points if the bezier curve is finalized. What do you think? And for casting to planar polygon to find the corresponding insertion point - I'd like to find a better solution for that. Perhaps, let the planar figure itself determine the insertion point? I.e. instead of the interactor finding the polyline point by the position of click and then asking for the control point to insert before, just let the planar figure do the search? E.g. replace the
GetControlPointByPolyLinePoint() method with something like FindPolyLineSegment(mitk::Point2D position, double accuracy)? The implementation for most figures should be trivial (or can even throw by default and left non-overriden for non-extendable figures) and implementation for extendable figures can take advantage of knowledge to which figure it belongs to instead of simple point-line-segment distance linear search that is done in the interactor. I think this solution is more flexible and more logical.

Comments by Markus:
Regarding 1) and 2)
Works for him as long as the flags do their job.

He agrees that the ResetNumberOfControlPoints() might not be the most elegant of solutions and synchronization might be a pain. But it is not only used in the c'tor but also in the PlanarCross::ResetOnPointSelect()

Use case:
Draw planarCross (4 control points). Changing one control point will remove the "other line", which has to be added again after the change. That has to work for them and might not do so using only getters.

Is/SetFinalized()
He believes the m_Placed is a very old bool flag, the initially placed property was added later as the reader/writer then knew how to handle them. Could be changed to a real bool flag, but it has to be set in the reader the same way as the property currently is.

Regarding the last point:
If you have to check in AddPoint whether you are allowed to add a control point you have botched your interactor. There are several conditions which should be used to prevent the state machine from reaching AddPoint()

  • CheckFigureFinished
    • CheckPointValidity
    • CheckFigureIsExtendable

The dynamic_cast to planarpolygon is mainly for a performance boost. Switching to getting the index from the figure itself shouldn't be much of an issue.

He can understand your concern regarding GetControlPointByPolyLinePoint(), but considering the interactor has to check the line segments and control points anyway for to determine the hovering state, he might as well return the segment. Changing that would just do the same thing twice, once in the interactor and then again in the planar figure.

Hi Caspar,

well, I had also modified the PlanarCross to just resize the vector and it works fine.

The reader already sets the finalized flag.

Regarding extension of BezierCurve. The old solution was a dyn cast to BezierCurve in AddPoint method which I removed. Now that the BezierCurve returns maximum number of control points being equal to the current number of points when it's finalized (i.e. double-click was used to end the interaction), AddPoint method is not even called.

I made the modification of finding points on the planar figure. It worked out really nice - please take a look at the last commit I made.
Basically, now the planar figure interface also has FindClosestControlPoint and FindClosestPolyLinePoint virtual methods, both take a point and maximum distance in object-space coordinates (it is computed by the interactor using display and planar figure geometries).
This gives the ability for PlanarSubdivisionPolygon to correctly handle the search for the polyline segment. Furthermore it allows more optimizations, e.g. PlanarCircle can test for closeness to polyline by simply comparing distance to point to the circle radius. Circle also returns the closest control point to be 1 when the query control point is near the circle itself, which cleanly handles the change that allows resizing the circle by starting the drag at any point on a circle, while conforming to the interactor's undo support.
This also let me remove any figure-specific processing from the interactor and only leave the significant dyn casts (e.g. the type of interaction event).

Please tell me if you have any other concerns. Otherwise, I will add the formatting commit (with clang-format) and hopefully we're done

Rostislav.

I will take a look at it. Just a quick note beforehand, could you sign off every commit?

I somehow thought that only the last commit needs to be signed off. If every commit needs to be, then I think it will be way easier if I formatted a patch instead of pull request and added the sign off manually to each patch file.

Technically each commit is a self-contained contribution we have to make sure you have the rights to share/you have to declare that you have the rights to share.

If creating a patch file is easier for you that would be fine by me. Could you make sure you do not introduce trailing white-spaces to code files?

clang format removes trailing whitespaces, so once we agree on the final shape of the contribution - I'll just clang-format the files and it won't be a problem.

Sadly it will. We check each commit for trailing whitespace (tws). So if you provide a three commits two of which contain tws and the third of which removes all of them, we still have to edit the two commits.

Ok, I guess the easiest solution would be to create the patch and then use a batch file to remove trailing whitespaces from patch files directly.

That might cause issues with changed .xml files which currently might contain trailing spaces (you can check which files are examined in the .gitattributes).

thanks for the info. Given that there's only one two xml files that I modified, and they are also change in only one of commits, I can handle that manually.

Markus and Daniel will both probably want to check the code as well, however Markus will not be able to take a close look at it before the end of next week.

I get errors compiling the code on linux (might not be too serious, the first one it finds is a unused variable warning elevated to error mitkPlanarFigure.cpp:317:9 int numberOfControlPoints = ...)

I did not fix it though, so there might be more servious compile errors lurking somewhere. Windows compiled fine though.

Some notes:

mitk::DoublePlanarEllipse

has two members:
m_ConstrainCircle
m_ConstrainThickness

which after your changes are always false. They are used in comparisons but they are never set true anywhere in the code.

mitk::PlanarEllipse

FEATURE_ID_MAJOR_AXIS and FEATURE_ID_MINOR_AXIS were renamed to FEATURE_ID_RADIUS1 and FEATURE_ID_RADIUS2 . I think keeping the old nomenclature would be clearer.

in void mitk::PlanarEllipse::EvaluateFeaturesInternal()
I would prefer more descriptive variable names, e.g. instead of p0, p1, p2 use centerPoint, firstControlPoint, secondControlPoint
same goes for radius1/2 etc.

I probably did not catch everything so you might want to hold off on the formatting commit.

I would suggest splitting your contribution in more commits. E.g.

  • one removing the entire non-const getters
  • one doing the lazy evaluation replacement

...

That way we can start integrating changes we all agree on and approve of. At the same time the amount of code we keep discussing gets smaller and easier to review.

Hi Caspar,

I guess fixing gcc's warnings could be left for after the functionality is finalized.

The flags for ellipse and double ellipse were set in the constructor so that during original figure placement they would behave as circles. Once it was placed, the flags were set to false to allow changing the radii separately. Clearly, when undo/redo was introduced this behaviour stopped working because if you undo the placement, the redo will produce wrong results because the internal state is different. Basically, I think that in this case the single responsibility principle is violated, so now the constraints during placement rely on whether the placement is actually happening (which makes sense to me). The flags are still there because they are the part of public interface so perhaps someone is using something like "double-circle". This functionality is maintained, but it is not used anywhere in the Workbench AFAIK.

Regarding the feature names - yes I agree. I think I renamed them because the names did not actually correspond to the values - i.e. major radius could be smaller than minor radius. Setting MAJOR = max(r1, r2), MINOR = min(r1, r2) would be a proper fix. Do you agree with this?

About smaller commits - well, first of all I don't really see how to achieve that without actually doing it manually change by change and in this particular case I think this is not time well spent. Regarding the particular example of getters/lazy evaluation, I think this separation into commits would also be logically wrong because after removing the non-const getters the code would not be correct until the lazy evaluation changes are integrated.

All best,

Rostislav.

One more (tiny) thing I added today is the FinalizedPlanarFigureEvent(). It didn't change any logic inside the PlanarFigure module itself - it's just useful for me (and maybe will be useful for someone else too).

Caspar asked me to take a look. Because it is quite some time since I worked on PlanarFigures, I looked exclusively at the undo/redo part of the changes, so no opinion at all on the other points changes.

I apologize if I missed some things because I just spent a couple of minutes reading the code without testing anything. Anyway, my remarks, hope they help:

  • there are #pragma statements that are Visual Studio specific
  • I found the use of the constants ADD / UNDOADD / CLOSECELL / OPENCELL not really corresponding to the associated operations. Defining new constants could be useful?
  • the following would need discussion, I did not compare to other data structures that support undo/redo. I got the idea when looking at method mitk::PlanarFigureInteractor::FinalizeFigure(): This method was a couple of method calls on the affected PlanarFigure before, and now it is really long, creating a list of operations. Was there a special reason why methods like "RemoveLastControlPoint()" do not create the necessary operations (and undo-operations) themselves? Like this the interactor would perhaps keep more readable. A concern might be that a program that creates many planar figures programmatically (without interaction, and without the need for undo) will flood the undo-stack. A possible counter-action could be to provide a "create undo" flag to all PlanarFigure methods that create operations to implement their action. If the flag was false by default, nothing would change, but we could have undo in the interactor by just providing a true flag to all called methods. _And_ other code could have PlanarFigure undo/redo if required.

Comments point-by-point

It's more efficient and in my opinion much clearer and less error-prone (less copy-pasting). But I understand that it might not adhere to MITK coding standards - this can be easily changed.

  • Yes, defining constants will make the code more clear
  • I designed the interactor following how other interactors are implemented. See for example the mitkPointSetDataInteractor. The fact that the undo/redo functionality is now mostly encapsulated in the interactors and that's probably not the best way to do things I think is a valid argument (e.g. why can we undo the point move resulting from dragging the point, but not when editing a point through a dialog), but I believe that this is a completely different (and big) discussion and is out of scope of this change.
kislinsk claimed this task.
kislinsk added a project: Auto-closed.
kislinsk added a subscriber: kislinsk.

Hi there! 🙂

This task was auto-closed according to our Task Lifecycle Management.
Please follow this link for more information and don't forget that you are encouraged to reasonable re-open tasks to revive them. 🚑

Best wishes,
The MITK devs

kislinsk removed kislinsk as the assignee of this task.May 26 2020, 12:05 PM
kislinsk removed a subscriber: kislinsk.