Page MenuHomePhabricator

Rethink/CleanUp PropertyRelation connection levels and RII properties used in SourceImageRelationRule
Closed, ResolvedPublic


With a bit distance I think the PropertyRelationRule is a bit overdesigned on its connection levels. Questions is, if we realy need to distinct between Connected_Data and Implicit_Data? In pracitce it makes no sense to have the posibility to store a connection in RII-properties in addition to normal properties (which would be a implicit_data connection). Either you have a ID and can make ID based connection (in additon to optional data connection) or not. But if you can only make a data based connection it makes no sense to try to additionally make it via RII-properties and a handbaked identifier. Either you can use the ID or you have it already in your data. Looking e.g. at DICOM relations, the ID connect and the "implicit" data connect are enough and serve there respective purpose. It makes no sense to have a "connected_data" and it only introduce additional complexity in the SourceImageRelationRule to manage this.

If no argument pro "Connexted_Data" comes up, till I take care of this issue, I would propose the following:

  1. remove the Connected_Data category and redefine the rest. Refactor the classes accordingly. The new categories are:
    • None = 0 (as before)
    • Data = 1 (was implicit_data, meaning that we just have connection on the data level)
    • ID = 2 (meaning that we just have it on the ID level, but it is explicit)
    • Complete = 3 (meaning that we have both data and id)
  2. Rework the code of PropertyRelationRuleBase in such a way that the disconnect on data layer does not depend on the finding of instanceID via RII-properties. A disconnect_datalayer should always be invoked if there is a Data connetion. It is implementation detail of the derived rule if the realy want to disconnect on that level. I think it would make sense to extend the call signature of disconnect, this would making thinks clearer and controlable by the caller what to disconnect (default = RelationType::Complete = everything):
void Disconnect(IPropertyOwner *source, const IPropertyProvider *destination, RelationType layer = RelationType::Complete ) const;
 /**Disconnect the source from the passed relationUID (usefull for "zombie relations").
 All RII-properties in the source for the passed relationUID will be removed.
 If the relationUID is not part of the source. Nothing will be changed.
 @pre source must be a valid instance.*/
 void Disconnect(IPropertyOwner *source, RelationUIDType relationUID, RelationType layer = RelationType::Complete) const;
  1. Rework SourceImageRelationRule. The RII-property SourceImageSequenceItem is not needed any more. Its now purely about either IDs or things encoded in DICOM tags. Makes life/code much easier for this rule.
  1. Connect() should work (and not fail) if one layer (ID or data) does not work. Only if both does not work, it may return an error/exception (if this is the currently documented and implemented error behavior, check). E.G. SourceImageRelation should not produce an error or so if it cannot connect at the data layer (due to missing dicom tags) but can connect on the ID layer).
  1. A function in the base interface would be usefull to query all properties that belong to a certain relation
/**Returns the list of PropertyKeyPaths of all properties that are relevant for a given relation.
@param source Pointer to the Source instance that containes the potential properties.
@param relationUID UID of the relation that is relevant for the requested properties.
@param layer Indicates which layer is requested. ID: returns all RII properties that belong to the relation. Data: returns all properties that are relevant/belong to the data layer of the relation. Complete: returns all properties (ID+Data)
@pre source must be a valid instance.
@pre relationUID must identify a relation of the passed source and rule. (This must be in the return of
this->GetExistingRelations(source). */
std::vector<PropertyKeyPath> GetReleationPropertyPaths(const IPropertyProvider *source,
                                                       RelationUIDType relationUID, RelationType layer = RelationType::Data) const;

Revisions and Commits

Restricted Differential Revision
Restricted Differential Revision
Restricted Differential Revision
Restricted Differential Revision

Event Timeline

floca triaged this task as High priority.Nov 23 2019, 11:11 PM
floca created this task.
floca updated the task description. (Show Details)
floca removed floca as the assignee of this task.Mar 18 2020, 10:38 AM
floca added a project: Restricted Project.
floca edited projects, added MITK (2020); removed MITK.
floca moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.
floca added a revision: Restricted Differential Revision.May 5 2020, 9:46 PM
floca moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.May 10 2020, 2:44 PM