Page MenuHomePhabricator

Inconsistent state of mitk::PointSet, points cannot be removed
Closed, ResolvedPublic

Description

MITK point sets contain two point containers, one for ITK, one for VTK points. The size of the two maps can become different after a remove operation.

Cause:

After the removal a new point is selected, but the ID of these new point is chosen incorrectly. The default is the '0' index, the fall back value is (size - 1). However, the points are stored in a map, not in a vector, and possibly no point may belong to the given index. So, when the 'selected' property is set for the chosen element, it can actually overwrite an already deleted item, including its ID.

Secondly, the VTK mapper checked if the point set is not empty and its state is consistent (i.e. the size of the point containers are equal). If these conditions did not fulfill, the function returned.

I had to remove the 'empty' check so that you can remove the last point from a point set from the screen. As I see, in the latest master the visibility is turned off before return, so this change might not be necessary. With 2013.09 it was.

I will send a pull request through github.

Related Objects

Event Timeline

I sent the pull request for the first bug.

https://github.com/MITK/MITK/pull/40

For the second issue, see this:

https://github.com/NifTK/MITK/commit/588ecb3df3b8782b26ea7af5817961c417e2cae3

This might not be necessary for the current master.

The bug is potentially there in the 'else' branch as well, when there is no position and all the selected points have to be removed.

I have not tested that, since we do not have a use case for that. A similar fix might be necessary.

@Tobi: could you have a look, please? It's maybe related to the other PointSet issues you're working on

This bug is indeed partially related to T15264 where according changes have been made with regard to the misuse-case within the point set interactor. The vtk mapper case should be resolved as stated. The else-case seems to be non-problematic as iterators are used to traverse the points container.

Any update on this?

We would like to minimise the differences between our MITK fork and the upstream, so it would be nice if the fix could be merged before the 2014-03 release.

Thanks.

We are waiting for the core change request affirmation and see into merging the fix asap.

Have you got the core change request affirmation?

(In reply to Miklos Espak from comment #7)

Have you got the core change request affirmation?

Some additional changes to the point set interface have been made in the meantime, and we are currently adapting the interface calls throughout the code accordingly. It won't make its way to the current release but will be made available soon after. Hopefully, you don't have too much of an inconvenience from that.

Not at all, thanks for the status update.

The second issue (588ecb3d) is invalid, it has been fixed in the master meanwhile.

The pull request is still valid, though.

User norajitr has pushed new remote branch:

bug-16698-FixPointRemovalInPointSetDataInteractor

Hi Tobias,
thanks for dealing with this issue.
As I see, you just committed a fix for this issue which seems to be almost identical to mine. Would you mind cherry picking my fix instead, so that the authorship is kept? I know that's a very trivial change.
I am happy to update the pull request to add the Signed-off-by clause.
Thanks.

Hey Miklos,

sure, let us do that. Better yet: please apply the same changes to the PointSetDataInteractor as the PointSetInteractor is deprecated (sign-off welcome). I created the new branch just to try the new changes. Seems to be all fine.

Thanks and best regards,
Tobias

Now I see that your commit was for the PointSetDataInteractor, while mine was for PointSetInteractor. Then just keep your commit, and also cherry-pick mine. I added the signed-off-by clause to it.

Can it be that your fix is for 17885 instead?

Note that even if the PointSetInteractor is deprecated, I still need the fix for it. I tried to upgrade to PointSetDataInteractor, but had several issues with it, e.g. 17885 and 17886. So, I am stuck with the old one.

Thanks for your fix, it seems to work fine for both the PointSetInteractor and the PointSetDataInteractor. I will push with the cherry-picked fix then.

Your fix applied to the PointSetDataInteractor solves this bug (no more unpredictable point selections), and apparently so for 17885 (no more point set disappearance) and maybe even for 17886 (last point rendered properly)? Or can you still reproduce these bugs?

[5a8670]: Merge branch 'bug-16698-FixPointRemovalInPointSetDataInteractor'

Merged commits:

2014-08-20 14:25:31 Tobias Norajitra [c51c68]
Comments removed.


2013-12-18 21:01:48 Miklos Espak [846fc2]
First point of a point set set 'selected' after removing point from point set

Signed-off-by: Miklos Espak <m.espak@ucl.ac.uk>
(cherry picked from commit 0bc4755a371e35c8944fcbf33eff41d8dda20f75)


2014-08-13 16:42:18 Tobias Norajitra [8a1ab5]
Fix for non-sequential id point removal.

17885 might be the same, but 17886 is different. It is not a bug but a missing feature, which was provided by the old position event but not by the new one.

[a1597a]: Merge branch 'bug-16698-FixPointRemovalInPointSetDataInteractor'

Merged commits:

2014-09-17 14:51:20 Tobias Norajitra [7243f3]
mitk-data revision tag adjusted.


2014-09-17 14:40:02 Tobias Norajitra [062f77]
Testing data paths adapted to mitk-data file structure.


2014-09-17 14:08:30 Tobias Norajitra [633234]
Geometries for TestPointSet and ReferencePointSet have to be updated before comparison.


2014-09-03 14:06:52 Tobias Norajitra [a61985]
Local interactor instantiation.


2014-08-27 15:07:34 Tobias Norajitra [a315ec]
Added point deletion PointSetDataInteractorTest.

I'm preparing for our next MITK upgrade and try to revert the patches that we have been fixed in the upstream.

As I see, the fix for this is in the master. Can this ticket be closed or are there some issues with it?

Thanks

Everything seems to work, so we can close this bug.