Hi Csaba. The issue is the color. İf you proceed with no modifier state and assign this term to segment, you will get gray color (wrong) as opposed to yellow (correct one). İt is. About hard to see in the screenshot with the modifier drop down obscuring the actual color at the bottom, but try and you will see.
Probably what happened is that in the first implementation selection of a modifier was enforced. Later this limitation was relaxed and the no “No type mofidier” option was added, but the change was not consistently made throughout the code. In summary, I think this is a bug. When no modifier is chosen then the basic, non-modified properties should be used.
2 Likes
I was just about to add the bug in the issue tracker with all the details, but when looking for the color specified in the json file, I found that no color is specified for the non-modified type. So this is a problem with the json terminology, and not the code (at least considering this example).
See here
No recommendedDisplayRGBValue tag exists for the non-modified type, only its Modified children.
That’s what I was afraid. We tried decorating the Json with recommendedDisplayRGBValue for unmodified state (so just before the modifier group), but that didn’t have an effect either…
You’re right. I just tried it too so it seems to be a real bug. I added an issue
But even if we fix this, the colors will need to be added to the JSONs. Unless we define some heuristic behavior, like use the color of the first modifier if not specified in the basic type.
I tested a version that does this and think this behavior makes sense. @muratmaga would you agree?
I noticed that if the color selector is used to pick a new color, that color persists when you move to other modifiers/types. Is this expected behavior?
For our use cases, this is definitely an acceptable behavior. Alternatively, since we will not be using Slicer’s DICOM terminology and will be generating our custom JSON, decorating the no modifier state with proper RGB codes is also an option for us.
I think that’s a bug too. Can’t imagine that being a default behavior.
This color of the first modifier is also what is currently displayed in the property types viewer.
I have created a pull request with this change here: https://github.com/Slicer/Slicer/pull/6973/commits/94e80a3d304c39176660ad55ade12b509c909f50
1 Like
Yes, this is a feature. See attribute ColorAutoGenerated (https://github.com/Slicer/Slicer/blob/main/Modules/Loadable/Terminologies/Widgets/qSlicerTerminologyNavigatorWidget.cxx#L1739-L1741), and same concept for manually edited name (so that once the user spends time manually specifying a property it is not lost when you change the terminology type).
@lassoan please confirm.
But that’s not how most other properties work in Slicer (like color tables, volume property), in my experience. If you want to make a change, you either make a copy or save it to the disk.
Anyways, I think the button on lower right reverts the state to original color, so it is not a big issue, but I found the behavior a bit awkward first.
Yes, the small reset buttons also reset this property, and the manually fixed color will again change to the one defined by the current type. I don’t remember the details anymore, but I do remember that we struggled implementing this in a way that it was robust. We wanted to allow the user to set custom color/name, and also to be able to reset the color/name to the one defined by the type.
I wonder if we change behavior so that the *AutoGenerated properties are reset on changing the type (or modifier), would it cause any issues or not. As I understand this is the main issue right? For especially the name, it probably does not make sense to keep the user-defined one if the user realized they chose a wrong type and change the selection.
@lassoan do you have anything to add?
It would be more appropriate to show default/gray color than copying the color of the first modifier. It should be no problem to add the missing colors to the json files.
If you manually pick a non-default color then it could be very frustrating to lose it. Manually set color and segment name is kept until you reset to default. We could make the behavior more obvious by displaying an “override” checkbox but then it would mean that the users need to do one more click to modify the name or color.
Regarding the segment renaming issue (Rename segments via Terminologies window · Issue #6975 · Slicer/Slicer · GitHub), it seems that @muratmaga suggests that we should open the terminology selector when double-clicking the name column. I think this could make sense, but then we would need to make it easier to add a new term (and probably to allow searching for terms in all the terminologies, to avoid re-adding a term just because it is defined in another terminology). Implementation might not be trivial, because we allow renaming in the subject hierarchy tree, too (we would need to define a mechanism that would control when name must be set via terminology).
I am not sure I am following this. While I agree it would be nice to add new terms/colors to custom terminologies for people to reuse their own terms, I don’t think this is strictly necessary for renaming behavior I mentioned. Currently, if I click the color button next to the segment name, in the pop up terminology button, name comes automatically editable. All it is necessary is perhaps focus on the name field and highlight it.
As for custom terminology, perhaps a button like “add to custom terminology”, and confirm name color selection and have them define file to write?
Highlighting the name field should not be a problem. Showing the terminology picker when renaming would be a major change and I’m sure many people wouldn’t like it (including myself, I very often rename nodes and having to go through the terminology picker would be very heavy for a simple rename).
Should we maybe discuss this tomorrow at the developer hangout? Unless the agenda is full or anything, in which case we can set another meeting.
İ will try to make the call, but that was just a suggestion to build awareness for terminology module and encourage its usage.
Exactly. That’s why I wrote that if we do something like this then we would need to make it much easier to add a new name (it should not require more clicks than by simply typing a new name). It would be great if we could pull it off, because then terminologies would be used much more frequently. However, I don’t know how we would name temporary segments, identify specific segments (e.g., tumor 1, tumor 2,…).