search

InsightSoftwareConsortium / ITK

Insight Toolkit (ITK) -- Official Repository. ITK builds on a proven, spatially-oriented architecture for processing, segmentation, and registration of scientific images in two, three, or more dimensions.
https://itk.org
Apache License 2.0
1.37k stars 660 forks source link

STYLE: Call `SetNthOutput(i, MakeOutput(i))` in a `for` loop #4687

Closed N-Dekker closed 1 week ago

N-Dekker commented 1 month ago

Reduced duplicate code by replacing manual SetNthOutput(i, MakeOutput(i)) calls for i = 0, 1, 2 with the corresponding for loops.

N-Dekker commented 1 month ago

@blowekamp Following our discussion at https://github.com/InsightSoftwareConsortium/ITK/pull/4654#discussion_r1596759648 I'm still trying to get clear in which particular scenario's MakeOutput(i) is actually useful, within a default-constructor. Is this an example use case? I mean, the ability to call it iteratively in a for loop?

N-Dekker commented 1 month ago

OK, so this could be a nice use case, being able to call Self::MakeOutput(i) is a for loop. Still I'm interested to hear if calling Self::MakeOutput(i) is also useful outside the constructor of a class (the class Self).

blowekamp commented 1 month ago

@blowekamp Following our discussion at #4654 (comment) I'm still trying to get clear in which particular scenario's MakeOutput(i) is actually useful, within a default-constructor. Is this an example use case? I mean, the ability to call it iteratively in a for loop?

Thanks for the continued investigation and the discussion. These filters have multiple outputs of different types. These different output types are not represented by the template parameters. Hence the MakeOutput method has been overloaded to create the correct "nth" output. This is important because the base class ImageSource just creates all outputs of the same type. Having the correctly typed output was a hard issue to track down with forced casted and failed dynamic casts. The pattern used in as many places possible was to use the MakeOutput method as the generic way to create the outputs.

Note: An incorrectly casted image type can work a surprising long time as the wrong type.

blowekamp commented 1 month ago

Still I'm interested to hear if calling Self::MakeOutput(i) is also useful outside the constructor of a class (the class Self).

It is also used here: https://github.com/InsightSoftwareConsortium/ITK/blob/8e5abbd87129e1cba589d5999b28957802ca28c0/Modules/Core/Common/src/itkProcessObject.cxx#L464

Which indirectly gets called when operations the "ReleaseDataFlagOn" or "DisconnectPipeline" are called and the output created in the constructor is created again.

N-Dekker commented 1 month ago

It is also used here:

https://github.com/InsightSoftwareConsortium/ITK/blob/8e5abbd87129e1cba589d5999b28957802ca28c0/Modules/Core/Common/src/itkProcessObject.cxx#L464

Interesting, thanks! Apparently SetOutput(name, output) creates a new output by calling MakeOutput, if the output argument passed by the user is NULL. I did not know that. 🤷

N-Dekker commented 1 week ago