User:Barre/Slicer

A few notes about Slicer 3.0

• I updated the CMakeLists.txt to match the changes in KWWidgets
• Slicer3 would not compile for Windows in shared mode, for the dll export symbol was missing in the headers in Base/GUI classes. I noticed there was a VTK_SLICER_BASE_LOGIC_EXPORT inside Base/Logic, and a VTK_SLICER_BASE_EXPORT in the parent dir, so I introduced a new one in Base/GUI (VTK_SLICER_BASE_GUI_EXPORT). You may want to make sure you need a unique one for each sub-libs in Base/ (this is the case in VTK for each of its packages for example, say VTK_HYBRID_EXPORT for the Hybrid/ subdir), or just a global one (which was the case some time ago in VTK, with a unique VTK_EXPORT for the whole tree). Also, you do not need to prefix the export symbol with VTK_ anymore, SLICER_BASE_EXPORT is just fine, I updated the wrappers some time ago to support it :) It's OK to disrespect VTK that way :)
• A set of useful setup paths scripts is now automatically created using KWWidgets macros: check your bin/ directory for Slicer3SetupPaths.bat (or .sh, .csh), and source anyone of them to update your environment variables so that you can run Slicer3 executable on Win32, especially if it is built in shared lib mode. Without this script, it becomes quickly impossible to setup the PATH, TCLLIBPATH, LD_LIBRARY_PATH (which names can change depending on the OS btw) manually each time. This is especially true since it seems the libraries in Slicer3 are in stored lib/ instead of the usual bin/, where the executables are stored; this works fine on Unix, but Windows will choke trying to find the DLL the executables depends on. Anyway, this set of setup scripts will do all that for you, point to the right ITK, VTK and KWWidgets lib and bin dirs, set the PATH, TCLLIBPATH, etc. Ultimately you probably won't need that if you include all third-party libs in the repository (as links to their CVS repositories counterparts I assume).
• I found no less than 3 different instances of #define SLICER_VTK5 and two of #define VTKSLICER_STATIC. This could probably be unified on top.
• We can talk about Tcl wrapping tomorrow, but there is actually a macro in KWWidgets (KWWidgets_WRAP_TCL) that sits on top of the Tcl wrapping macros in VTK and try to cope for various VTK versioning issues (I mentioned it in the main CMakeList.txt inside comments, and used it now in Base/GUI).
• I ran Slicer3.exe, and a lot of warnings are issued because the pipeline is not setup correctly before you load something.
• Slicer crashed on exit, I fixed one crash-point, and this is something interesting actually: the Slicer app was listening to the ModifiedEvent of one of its UI components in order to update itself; the issue to keep in mind here is that nothing prevents a class destructor to do something like this:

 this->SetFooBar(NULL);

which deallocates the FooBar string if it was defined using the very common pattern:

 vtkSetStringMacro(FooBar);
 vtkGetStringMacro(FooBar); 

in the class header. Now remember that SetFooBar() *will* trigger a ModifiedEvent. What this means is that you may be called back during an object destruction! Now as long as you carefully check that the object still exists (which is what I fixed, and that should be a good practice anyway), you should be OK. I would recommend listening to more specific events though, and/or ask me to add them for you in KWWidgets.

• Slicer3.exe is still crashing in ~vtkSlicerApplicationGUI because this->KWApplication is probably deleted multiple times. The whole KWApplication attribute is very confusing to me in the current design, since it is found, declared and probably duplicated as an attribute to different classes: remember that any vtkKWObject already have a pointer to a vtkKWApplication object (this->Application), i.e. the design is either duplicating it, or using two at the same time, and if not synced properly, bad things will happen :) In our case here, this has probably something to do with ref counting, etc. I would strongly recommend you use the one already in KWWidgets's vtkKWObject, which can be set using the vtkKWObject::SetApplication() method, and which is ref-counted properly. I can help you with that.

=> OK, I have not much time to fix more crashes, but here are few more things I noticed. I'm sure you guys are aware of them, but I'm just trying to give some constructive feedback regarding the source tree:

• Comments. Comments. Comments. My personal opinion is that it is really important that the code is commented *as soon as possible*. Right now a large number of methods in Base are undocumented, making it really tricky to follow; I think somebody should be able to get a good grasp of the architecture from the header files, which I was unable to do at the moment. I would definitely recommend grouping and commenting methods together to avoid blocks of unrelated and undocumented methods like:

   // GUI FUNCTIONS:
   virtual int BuildGUI ( vtkSlicerApplicationGUI *app ) { return 1; }
   virtual void SetWindow ( vtkKWWindowBase *win );
   vtkKWWindowBase *GetWindow ( );
   virtual void SetSlicerApplication ( vtkSlicerApplicationGUI *app );
   vtkSlicerApplicationGUI *GetSlicerApplication ( );
   virtual void SetKWApplication ( vtkKWApplication *app ) ;
   vtkKWApplication *GetKWApplication ( );

Also remember that grouping is paramount if you are planning to create documentation automatically using Doxygen.

• Style. Style. Style. Another thing I've noticed in my years at Kitware, it is probably equally important to adopt a consistent coding style. I'll let you guys decide which one, but since most classes inherit from VTK, I would strongly recommend using the VTK style, i.e. 2 spaces indentation, newline before curly braces, avoid one-liners (pretty dangerous), etc. I.e. this code:

   if ( this->LogicCommand )
       this->LogicCommand->Delete ( );

is in VTK:

 if (this->LogicCommand) 
   {
   this->LogicCommand->Delete();
   }

We have emacs and vi modes to do that automatically for you, if you are interested.

• Forward declaration. I think it's actually a very good practice to include as few as possible dependent headers in a class declaration, i.e., if you do not explicitly refer to the vtkCollection API in your class header for example, you really want to use:

  class vtkCollection;

instead of:

  #include "vtkCollection.h"

A counterpart to this is to try to avoid inlining code in the header file as much as possible (unless it's only one line of code, really). Inlining in declaration files makes them so much longer and distract from a clean API; you should probably only be exposed to the public interface of a class, not what is going on under the hood. Inlining also creates a lot of dependencies. For example, in vtkSlicerApplicationLogic.h, you probably want to avoid:

 void Connect (const char *URL) {
   if (this->MRMLScene)
     {
     this->MRMLScene->SetURL(URL);
     this->MRMLScene->Connect();
     }
 };

Now believe me, this is not just fancy :) We actually cleaned up VTK 4.0 and remove all extraneous #include that way, and this sped up the compilation process *tremendously* as well as enabled us to avoid a lot of dependencies checking. This is going to be *very* true in a project that uses a lot of large external third-party libs like VTK, ITK and KWWidgets (if you are wondering what CMake is doing each time you compile, well it's checking the dependencies). As you probably know, moving the above lines to the definition file (.cxx) will not harm your performance; virtual functions are used so much in VTK that inlining is actually not used very often.

• I would recommend using STL instead of the (aging) vtkCollection for all your internal container needs :)

Thanks !