educelab / volume-cartographer

Volumetric processing toolkit and C++ libraries for the recovery and restoration of damaged cultural materials
GNU General Public License v3.0
63 stars 22 forks source link

Refactor Segmentation pointset changes #89

Closed csparker247 closed 3 months ago

csparker247 commented 3 months ago

Revises some changes from #88. My impression of that PR was that, somewhere downstream, people were attempting to use getPointSet() (or some 3rd party equivalent) on segmentations where that field was set to an empty string, which is the default value in VC when a segmentation is first created. To keep people from doing this, the previous PR effectively ignored any segmentations with an empty string value for the vcps field.

Upon reflection, the problem with this solution is that Segmentation::hasPointSet() relies upon this emptiness property to tell us whether a point set has already been assigned to the segmentation. This subsequently enables the VC GUI to check whether we can draw a new segmentation curve in the UI.

An empty string is, admittedly, not a great solution, but needs to be supported for legacy reasons. This PR makes a number of changes:

Here are conforming configurations of a segmentation metadata file. Third party software is encouraged to use the first of these.

  1. vcps and/or volume fields are null (preferred)

    { "type": "seg", "uuid": "abcd0123", "name": "Foo", "vcps": null, "volume": null }
  2. vcps and/or volume fields are absent

    { "type": "seg", "uuid": "abcd0123", "name": "Foo" }
  3. vcps and/or volume fields are empty strings

    { "type": "seg", "uuid": "abcd0123", "name": "Foo", "vcps": "", "volume": "" }

In all of these configurations, the API will behave as follows:

auto seg = Segmentation("abcd0123");  // no except
seg.hasPointSet();                    // false
seg.getPointSet();                    // throws std::runtime_error
seg.hasVolumeID();                    // false
seg.getVolumeID();                    // throws std::runtime_error
csparker247 commented 3 months ago

@stephenrparsons Mostly want your input on the change to the API behavior. Does the proposal fix the situation you were encountering?

stephenrparsons commented 3 months ago

My understanding of these changes are that they look good and are compatible with the intention of #88. That was made to accommodate the case when a path directory is added to paths/ by some means other than being generated by VC. Said path may either not have a meta.json, or might have one that has no pointset.vcps to point to. In these cases, it is convenient to allow VC to skip these and still load the valid paths, rather than to exit. It looks like that behavior is preserved here.

csparker247 commented 3 months ago

Perfect. I just added a check to VolumePkg when loading segmentations, renders, and volumes which ignores all directories that 1. don't contain a meta.json file and 2. throw an exception during construction (what you had already added for segmentations). So this should be quite a bit more flexible to people playing around with the VolumePkg format at their own peril.

Thanks!

stephenrparsons commented 3 months ago

Fantastic, thank you @csparker247!