I have pushed topic to https://github.com/kevinwangcanada/Slicer/tree/2919-Add-UseColorNameAsLabel-for-scalarbar-widget

All the changes are limited to the colors module.

Ideally, these changes should be push to VTK and CTK.

Checked it out and building now.
First thought: I'd prefer to see the vtkSlicerScalarBarActor in a VTKWidgets subdirectory in the Colors module, to match the way the classes are separated in the Annotations module. That way it can be part of a separate library that other modules could link against and use just the scalar bar widget.

Could you also add a test for the new class? Something simple to start is fine, I just want to have an indicator on the dashboard that the new class is working as expected.

I'd like to see the GUI label text changed to:
Use Color Names as Labels in Scalar Bar

Adjusting the "Number of labels" spin box in the Scalar Bar GUI widget doesn't seem to be making any difference in the scalar bar VTK widget (tested with the Pelvis Color color node). It does work when you toggle it back to not using the color names, the numbers do adjust.

Testing with the Abdomen Colors shows the limitation more clearly, the text is unreadable because the table goes up to 701 even though it's mostly empty. Generic Anatomy Colors also ends up with really tiny fonts. 50 labels max might be a good upper limit/default, as the 64 color non semantic one you can just about make out the text.

Thanks for all your comments. I will address them in the next submission and will let you know when it is ready.

Hi Nicole,

Actually vtkScalarBarActor is not in vtk widget directory, it is in Rendering directory. Maybe putting vtkSlicerScalarBarActor in VTKRendering or Rendering subdirectory would make more sense, how do you think?

Kevin

For now, I meant to move vtkSlicerScalarBarActor into
Slicer/Modules/Loadable/Colors/VTKWidgets
so that it matches the Slicer convention. Porting it to VTK and updating the CTK widget are separate issues.

Sorry, I did not make it clear. Instead of moving vtkSlicerScalarBarActor into
Slicer/Modules/Loadable/Colors/VTKWidgets, how about moving into Slicer/Modules/Loadable/Colors/VTKRendering? since vtkScalarBarActor is not a widget type class.

Thanks.

I was going by the placement of the widgets and representations in the Annotations module, but you're right, the scalar bar actor seems to be a class that was left behind in the Rendering library during the transition to the widget/representation framework in VTK.
So, yes, Slicer/Modules/Loadable/Colors/VTKRendering should be fine, sorry for the confusion!

Hi Nicole,

I have finished all the improvements you recommended and have pushed to topic at https://github.com/kevinwangcanada/Slicer/tree/2919-Add-UseColorNameAsLabel-for-scalarbar-widget

Let me know what you think about this version.

Thanks,

Kevin

Minor thing: could you change the test name from
TestvtkSlicerScalarBarActor.cxx
to
vtkSlicerScalarBarActorTest1.cxx
to conform to Slicer test naming conventions?

Otherwise looks good to me!

Hi Nicole,

I have changed the test name and pushed the topic at https://github.com/kevinwangcanada/Slicer/tree/2919-Add-UseColorNameAsLabel-for-scalarbar-widget

Thanks,

Kevin

Hi Nicole,

Thanks for your work. I forgot to check the labelformat. I have made changes to allow labelformat to be used with colornames. You can check out the new code at https://github.com/kevinwangcanada/Slicer/tree/2919-Add-UseColorNameAsLabel-for-scalarbar-widget

Regarding the labelformat widget updating issue. As you said it is same no matter usecolornameaslabel is selected or not. I think it is a separate issue and maybe we should create a new ticket for that? also the scalarbarwidget ui is done in CTK so this might have to be fixed in CTK not slicer.

Thanks,

Kevin

For reference, my report that I sent via email instead of via this bug report was:

The colour names are adjusting to the number of labels spin box setting now, looks good.
The Labels property Format string seems to be being ignored when I change it - I wanted to try adding a prefix or post fix string to the colour names to test it out but nothing changed. This works when I don't use the colour names as labels. The cursor also jumps to the end of the format string box after every character.
It also seems to delay in changing the colour and style until I try to change either again. But turning off the user colour names as labels option, it's the same thing, so I think it's an update issue with the widget (it updates when I click in the 3d window).

After this latest update, the label format string is updating on the vtk widget as I change it in the CTK widget. Label color, opacity, font, style all require a click in the 3D window to show the updated setting. The problem could either be in the CTK widget when it updates the setting but doesn't trigger an event, or in the way the VTK widget is hooked up to the 3D window (the dipslayable manager not listening for certain events?). I'd say that's a separate bug report, starting here in Slicer and them moving to either VTK or CTK's bug tracker as needed.

Actually on my machine, label color updates properly. The others (opacity, font and style) are having update problems.

I have created a new ticket http://na-mic.org/Mantis/view.php?id=3027

Do you want to push the code to slicer trunk and change the status of this ticket to resolved?

Thanks

Thanks for creating the new ticket!
Working on pushing the code now...

One file is in conflict with the trunk:
Modules/Loadable/Colors/CMakeLists.txt
I think due to this commit:
http://viewvc.slicer.org/viewvc.cgi/Slicer4?view=revision&revision=21770
Can you update your branch?

Hang on a bit, I realised there are some differnet ways I can try merging the changes that might avoid the conflict...

I've run into too many conflicts, and can't get it merged tonight:
the VTKRendering directory is causing a clash with it existing in the git branch but not in svn
the vtkSlicerScalarBarActor.h needs a newline at the end of the file
Modules/Loadable/Colors/Testing/Cxx/CMakeLists.txt has conflicts with the trunk

Could you please fix these on your branch and I can try again on Monday?

Hi Nicole,

I have updated my branch to the latest Slicer trunk.
https://github.com/kevinwangcanada/Slicer/tree/2919-Add-UseColorNameAsLabel-for-scalarbar-widget

Can you update your branch to check if you still have many conflicts?

Thanks.

I'm still having problems merging it, but think that I'm hitting the limits of my git expertise. I have a couple more things I can try and some experts I can call in, and will give it another shot tomorrow or Monday.

Thanks for your time. Maybe we can wait a bit since we are still seeing build errors on the dashboard and Jc and other experts are probably busy working on those issues.

JC had some comments as I was trying to merge your code:
https://github.com/naucoin/Slicer/commit/c61feb1fba5c83af89d6d2548a22daf6fe4dc5e1#commitcomment-2938065

What's your plan wrt getting this change into VTK? I'm okay with the scalar bar actor class name staying as is for Slicer inclusion, but let me know if you want to change it for ease of integration with VTK.

I have read through the comments on github. I am ok with the name change to vtkSlicerScalarActorWithColorNames.

It would be nice if this get into VTK if VTK people think this is of value, if not I am fine with it staying slicer. However, I would like to minimize the overhead of integration. if we want to get this into VTK eventually, then making it into slicer and remove it later would be a waste of time. On the other side, if we follow Jc's recommendatioin, get this into the VTK fork of slicer but later the VTK people reject to get this into VTK, then we have to remove it from VTK fork and add it into slicer. What do you guys think?

What's the integration status of this feature? Are you still thinking of going the VTK route?
We're looking to sort out the 4.3 release ASAP and I'd like to get this issue updated.

Sorry I did not have enough time to push this to VTK. It will probably take some time to get this into vtk. so is it possible to mark this feature as future instead of 4.3?

Hi Kevin,

There's been some interest in working with color tables again and I wanted to check in with you on the status of putting this feature into VTK. If there's no traction there, should we just put it into Slicer now?

Hi Nicole,

This feature is available in VTK6.0. so when Slicer migrate to VTK6 it can start to use this feature.

for VTK5.10, I have created a patch to backport the VTK6.0 feature to 5.10. however, since this is a feature patch and VTK5.10 branch only takes bug fixes so it can not go into 5.10 branch. so what I did was to submit a pull request to Slicer's VTK clone [1]. it has not been merged into the trunk yet. I guess Jc was busy working on other projects.

let me know if you have more questions.

[1] https://github.com/Slicer/VTK/pull/1

Hi Kevin,

Great news! I'll check in with JC at the winter project week and see if we can get the pull request integrated on the Slicer VTK clone, for now I assigned him so we can get the conversation started.

To be revisited once Slicer switches to VTK6 before the 4.4 release.

VTK6 can be enabled with a CMake flag now, looking at integrating this feature using
#if (VTK_MAJOR_VERSION >= 6)
#endif
conditional blocks.

Dug into this a little bit:
The vtk6 scalar bar actor has the DrawAnnotations flag, if set to true, it will look at the vtkScalarsToColors and use the annotations that have been set there. They're not used for the labels, but are a separate set of text annotations that point to the color bars in the widget, and are not controlled by the interface to set the number of labels - all of them are used (see attached 64-color-nonsemantic-annotated).

Going forward there are some options:

• we can annotate all of Slicer's color look up tables and remove the Names array from the node, or do a local annotation just for when using nodes with the scalar bar actor
• Slicer will have to manage the annotations on the look up tables used for the scalar bar actor to reduce the number of annotations (deep copy, ResetAnnotations, calculate which N names to include)
• otherwise go with Kevin's code that puts the color names in the labels, which can bypass using the LUT annotations entirely (not taking advantage of vtk6) and can also support more customisation for Slicer users
• submit a patch for vtk6 to allow more customisation of the scalar bar actor (number of annotations, as labels or annotations), or to a Slicer version of vtk6 for faster turn around.

VTK bug submitted:
http://vtk.org/Bug/view.php?id=14752

CTK issue submitted:
https://github.com/commontk/CTK/issues/473

Hi Nicole,

I have changed my original vtkSlicerRTScalarBarActor class code to use annotations in VTK6. but instead of using VTK6 drawAnnotation which shows annotation along with labels, I just turn off drawannotation options and instead as a UseAnnotationAsLabel option which shows annotation as labels. this allows to sample the annotation just as we do for labels. You can take a look at the code at : https://www.assembla.com/code/slicerrt/subversion/nodes/1832/trunk/SlicerRt/src/Isodose/Widgets
and maybe give it a try.

Thanks,
-Kevin

Hi Nicole,

a new topic branch was created for the VTK 6 code.

patches has been pushed to this branch: https://github.com/kevinwangcanada/Slicer/tree/2919-Add-UseColorNameAsLabel-for-scalarbar-widget-VTK6

Please review it.

Thanks,
-Kevin

Took a quick look at the code, will compile and test it this afternoon.

The one minor thing that jumped out at me was the use of snprintf, I hadn't noticed it used elsewhere before, but a quick search shows that it's only used in one file:
Modules/CLI/CreateDICOMSeries/CreateDICOMSeries.cxx
which uses this define:
#define snprintf sprintf_s
Would that or an std::string work? I'm mostly bringing it up since OS specific defines tend to be a bit fragile.

Testing results:

• the font of the labels is really small when displaying the Generic Anatomy Colors node.
• it crashes when trying to display the Heat FreeSurfer color node:
  0000004 0x0000000131f0d81b in vtkSlicerScalarBarActor::LayoutTicks (this=0x7fe1b0673a40) at /Users/nicole/Slicer-git/Slicer/Modules/Loadable/Colors/VTKWidgets/vtkSlicerScalarBarActor.cxx:302
  302 SNPRINTF(string, 511, this->LabelFormat, val);
  Current language: auto; currently c++
  (gdb) print val
  $1 = -10
  (gdb) print this->LabelFormat
  $3 = 0x7fe1b067a6f0 " %s"

I'd also like to see a test added, a python self test that does the following:
mainWindow = slicer.util.mainWindow()

open the colors module

mainWindow.moduleSelector().selectModule('Colors')
colorWidget = slicer.modules.colors.widgetRepresentation()
ctkScalarBarWidget = slicer.util.findChildren(colorWidget, name='VTKScalarBar')[0]

show the scalar bar widget

ctkScalarBarWidget.setDisplay(1)
activeColorNodeSelector = slicer.util.findChildren(colorWidget, 'ColorTableComboBox')[0]

iterate over the color nodes and set each one active

numColorNodes = slicer.mrmlScene.GetNumberOfNodesByClass('vtkMRMLColorNode')
for n in range(numColorNodes):
colorNode = slicer.mrmlScene.GetNthNodeByClass(n, 'vtkMRMLColorNode')
activeColorNodeSelector.setCurrentNodeID(colorNode.GetID())

use the delay display here to ensure a render

Since the release is tomorrow, I'm going to retarget this and we can get it ready for the next patch release.

Hi Nicole,

I just finished fixing the crashes and font too small problem. I have also added a python selftest. please review the new code at https://github.com/kevinwangcanada/Slicer/commit/04f6094a8969fce8510b03647d1fb976009e24bb

Thanks,

Kevin

The test is missing the delay display line to make sure that the window renders after switching color tables. This should work (didn't test it yet):
self.delayDisplay('Set Color Node To %s' % colorNode.GetName())

I have added delayDisplay and tested it. it worked. code pushed to github already.

Thanks.

Thanks!
Still building, but didn't see in the code the checkbox to turn on/off the Use*AsLabel option, is it there?

No, it is not there. as the proper place to put that option is the ctk scalar bar widget which would require to change CTK codebase. another way is to add the option in Slicer GUI but it will look a bit strange.

so I took the liberty to not implement it for now. for colorTableNode or FreeSurferColorNode which all have a colorName property, I will display the colorname, for procedurecolornode, just display color value. I hope that is ok.

if people want to know the value, then they can check that using the colorname.

Thanks,

Kevin

True, the CTK widget is missing some options, I'm just leery of turning on a feature like this that people may use as non-developers (ie for pulications) without giving them an option to turn it off. I'll take a look at the user experience and see how it works.

Make sense. you have better understanding with colors module. so please fix wherever you think it fits.

Thanks.

I tried a first pass of adding the check box, got a crash when running the self test, in snprintf again.
Stashing that and testing your base code:
I added a command line test for the python script and fixed the switching into the Colors module.
Running from the self test module: no crash, but it hangs for a long time on the free surfer labels since it's got over 10k entries. Same from the command line. The atlas nodes also take a long time, I think it'll need some optimising.
Looking at the Colors module as a user, even with the scalar bar turned off, when I switched to the FS labels to double check the size of the table (14176), it hung. It's updating something in the background - that has to get fixed.
The lack of being able to switch to the color indices was noticable to me, but I was looking for it.

Todo: isolate the crash, test my logic for switching between names and indices. The FS continuous tables are showing the generated RGB names and need to be a special case.

Crash is due to invalid format string plus number combo with the checkbox changes.

Kevin added support for batch setting the annotations to avoid a performance problem:
https://github.com/kevinwangcanada/Slicer/commit/15476a3a94f279471a1add034a25bc7e378c33fa

I incorporated that in my addition of the check box to toggle between names and values (with simple format string checking):
https://github.com/naucoin/Slicer/commit/01e476598a148b6ce69c21f3a363d1c22afc75b7

I think that branch is ready to test elsewhere and integrate.

pull request:
https://github.com/Slicer/Slicer/pull/164

Reponding to comments on the pull request:

TODO: in the slicer scalar bar actor, make a more strict regular expression to check for strings vs numbers in the label format.

Updated pull request:
https://github.com/Slicer/Slicer/pull/167

Fixed in r23566
See http://viewvc.slicer.org/viewvc.cgi/Slicer4?view=revision&revision=23566

Hi Nicole,

I just updated my slicer and now I can see the checkbox you added above the scalarbar. that is a very smart place to put the checkbox for now.

Do you have a plan to relocate the checkbox into the ctk widget? I think that would be a more logic place to put the checkbox.

Thanks,

Kevin