Skip to content

feat(core): use a real DICOM context to manage DICOM attributes

Description

This is an intermediate step for #862 (closed) and can be summarized as three main changes.

1. Series related modifications

This implies many renames and massive refactoring. SightViewer and SightCalibrator (and of course other activities, samples, tutorials, configuration, ...) have been modified accordinatly, but it is almost certain that some changes need to be done in external repositories.

1.1 Series are for DICOM only

Data objects that have no reason to inherit from sight::data::Series, are no more inheriting from sight::data::Series. This includes sight::data::ActivitySeries (renamed to sight::data::Activity) and sight::data::CameraSeries (renamed to sight::data::CameraSet). There was no meaning to having an activity limited to one DICOM image or to storing a "Study Date" whereas this activity has no link to a DICOM data. Same for a collection of cameras.

1.2 New collection classes for Activity and Camera ...and Series

Since they are no more sight::data::Series, we cannot mix them with regular Series in a sight::data::SeriesDB (renamed sight::data::SeriesSet), and specific collection classes have been created, like for activities, stored in a sight::data::ActivitySet. All "Sets" are like std::set (with the unicity of data guaranteed by design), but also like a std::vector, ordered with random access support. Take a look at sequenced_set in libs/core/data/IContainer.hpp for some insights. This implies that we cannot any more put everything in a magical scumbag (called SeriesDB) to pass everything across application configuration and expect our services to retrieve magically their data, filtering with dynamic_cast everywhere. Now, each container type has the correct contained type. A service which works on activities, will use an sight::data::ActivitySet, another one which works on image series, will use a sight::data::SeriesSet, and so on... This simplifies the code inside the services (no more dynamic_cast, no more check for unicity, etc...).

1.3 sight::data::ImageSeries inherits from sight::data::Image

This allows to skip all sight::data::ImageSeries::getImage() and to use an sight::data::ImageSeries in all services that normally work only on Image, as is.SGetImage is therefore removed, because now unneeded. This simplifies, of course, the code.

Sorry...

Because an sight::data::ImageSeries still inherits from Series, and both sight::data::Series and sight::data::Image also inherit from sight::data::Object, we have now an ugly "diamond" inheritance. Many things have been tried to get rid of that, but it would imply too many changes to remove the Series Object inheritance or to make it a pure virtual class. Unfortunately, the only choice was to use virtual inheritance and deal with the black diamond. If someone has a better clue, feel free to share it ! The only other viable solution would be to simply remove the Image class and only have an ImageSeries. This is definitively something I can live with.

2. "Shallow" and "Deep" Copy revisited

2.1 Inheritance used for copying

Because we needed to have a "Description" for non DICOM classes (Descriptions were stored as DICOM attributes), to allow, for example, to show a meaningful text in selector dialogs (especially in Camera selector), we added it to the base class sight::data::Object as we tough this attribute could be used for many other classes. Unfortunately, this affects serialization, shallow and deep copy, as everything was not done properly concerning inheritance. To implement correctly deep and shallow copy, all classes had to call the object "field" copy method, being aware that a subclass should not do the same elsewhere, and that only fields stored in base class sight::data::Object were covered, nothing was done for possible other inherited attributes.

Now, we must call systematically and explicitly BaseClass::deepCopy(...); or BaseClass::shallowCopy(...); from within the deepCopy() and the shallowCopy() derived implementation. Note that BaseClass is normally automatically defined by the SIGHT_DECLARE_CLASS(...); macro, but you can of course control which super class you use. Doing so, will ensure that "fields" will be correctly copied, but also inherited attributes, even the ones forgotten by the derived class implementer.

2.2 Visibility and signature changed for DeepCopy()

Now it is possible to perform "partial" deep / shallow copy ! Copying only "Series" part from an ImageSeries to an ModelSeries can be done simply by:

auto image = data::ImageSeries::New();
// do things and fill DICOM tags like "PatientName", ...
...

// need to work on a model, but I'm lazy and don't want to fill again DICOM stuff
auto model = data::ModelSeries::New();
model->Series::deepCopy(image); // And voilà !

3. Use GDCM to store DICOM attributes

This is the most important change. Before, all DICOM attributes were simply stored as std::string across sight::data::Patient, sight::data::Study, sight::data::Equipement, sight::data::Series and sight::data::ImageSeries. This had the extreme limitation to only support the defined DICOM attributes and to force us to implement the conformance of these attributes to the DICOM standard. For example, an attribute like Series Number which is a Integer String, should be stored with only numbers, "+" and "-", with a maximum of 12 characters. We could write code like series->setSeriesNumber("youplaboom");, without any problem. As the ultimate goal is to reuse the DICOM context when loading and saving DICOM data, we need to enforce the validity of the data.

3.1 Only sight::data::Series "stores" privately DICOM data

Since the DICOM context cannot be shared easily, as we don't want to "pollute" data library with dependencies from the DICOM engine we use, we choose to use a PIMPL approach with "private" dependency on GDCM. Therefore, the DICOM context (a GDCM dataset) is stored in Series::SeriesImpl class located in libs/core/data/Series.cpp. There is no reference to GDCM outside this file. Since sight::data::Patient, sight::data::Study, sight::data::Equipement only served as DICOM attributes "container", there were removed. sight::data::Series is the only class to know for setting and getting DICOM tag values.

3.2 API

For legacy compatibility and to have something more explicit than direct DICOM tag access, sight::data::Series keep a set of "getters/setters" for various previously used DICOM tag. sight::data::Series will simply forward the call to the internal GDCM instance. For example, sight::data::Series::setSeriesNumber() and sight::data::Series::setSeriesNumber() still exist, but will use the right type (an std::uint32_t) and will use GDCM to read/write it to the GDCM dataset object.

Since all possible "getters/setters" are not implemented, it is also possible to access to DICOM attributes using directly a DICOM tag. This gives access to all possible DICOM attributes, but with some limitations, as we use a std::string as destination/source buffer for raw DICOM data. getByteValue() and setByteValue() will also work on non-string attributes, you should just consider the returned std::string as a raw byte buffer.

For example, the Series Number can be retrieved like:

std::uint32_t seriesNumber = 123456;
auto series = data::Series::New();
series->setByteValue(0x0020, 0x0011, std::to_string(seriesNumber));
CPPUNIT_ASSERT_EQUAL(seriesNumber, *series->getSeriesNumber());
CPPUNIT_ASSERT_EQUAL(std::to_string(seriesNumber), series->getByteValue(0x0020, 0x0011));

Some facilities have also been implemented, like the support of multiple values with auto "" splitting, just take a look at unit tests for examples.

3.3 Why GDCM and not DCMTK

To be honest, GDCM is far from being a perfect DICOM library. Its API is awful, counterintuitive ...and limited. For example, there are no built-ins to check the validity of attributes, even basic checks, like its maximum size. They are no way to verify that "Coded String" attribute like laterality is set to one of the reserved string ('R' or 'L'), everything must be done by "hand". DCMTK seems clearly better, even if far from perfect. It was chosen at the beginning, but, unfortunately, Ubuntu / Debian compiled DCMTK without jpeg200 support and ITK, which can use both DCMTK and GDCM, is only built with GDCM. To sum up, using DCMTK will force us to build it by ourselves, with all the burden it means.

Closes #901 (closed)

How to test it?

  • All unit tests
  • All Applications / Tutorials / Examples
  • Everything....
Edited by Didier WECKMANN

Merge request reports