Tweak ctkVTKConnection - Fixes Slicer issue #3361 and also optimize connection creattion/removal #476
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In case the debug statement is used in the destructor, this commit will prevent the application from crashing.
By using ctkCallback instead of QTimer, we can associate a function that increment a counter each it is invoked. This allows to check that the Qt slot or VTK callback have been invoked. This commit also compute the stats associated with the connection5.
…ndently This is particularly useful when instrumenting the code and running callgrind on each test case independently.
This can be useful for both providing a custom implementation and for testing.
* Use vtkWeakPointer to track the VTK object. The observation of the DeleteEvent is now required only to disconnect the Qt slots. Using a weak pointer also ensure that the pointer to VTK object will be set to zero with/without observing deletion. * Simplify ctkVTKConnection: There is not need to explicitly disconnect slots when object is deleted. Indeed, according to Qt documentation, "all signals to and from the object are automatically disconnected" there is no need to explicitly disconnect them." [1] http://qt-project.org/doc/qt-4.8/qobject.html#dtor.QObject By also removing the call to 'disconnectSlots' in the 'disconnect()' function, it resolves Slicer issue #3361 [2] where it was reported "No such slot" error. Since the disconnect happened in the connection destructor and the connection were destroyed in the base class but the slot belonged to the derived class, this explained the error message. [2] http://www.na-mic.org/Bug/view.php?id=3361 * Simplify ctkVTKConnection: There is no need to explicit remove VTK observers where connection is deleted. Indeed, observers of a VTK object are automatically removed when an object is deleted.
Now the VTK pointer is set to zero independently of the DeleteEvent observation, the observation is only needed to explicitly disconnect the Qt slots. Since keeping the connected slots around doesn't impact performance, this commit disabled the "deletion" observation. If needed, a future additional improvement would be the pruning of connection there are associated with deleted VTK object. This could be done in the same loop taking care of shadow connection. Results of ctkVTKObjectTest1: [ObserveDeletion = true] 148: Create 10000 connections... 148: elapsed time: 3.06955 seconds 148: Remove 10000 connections... 148: elapsed time: 3.44588 seconds [ObserveDeletion = false] 148: Create 10000 connections... 148: elapsed time: 0.775543 seconds 148: Remove 10000 connections... 148: elapsed time: 0.760212 seconds
As discussed in [1], the test should check that the code works as expected and not set a bar that we cannot yet attain. [1] commontk#378 (comment)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.