Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: 3393 transform image io as shared #115

Conversation

hjmjohnson
Copy link
Member

No description provided.

jcfr added 4 commits July 5, 2016 15:23
…SoftwareConsortium#3393

This commit updates the Mesh IO factory to ensure it can be built
as shared library so it is registered only once when linked to both
a main application and application plugin dynamically loaded.

In a nutshell, it updates the code so the itk::MeshIOFactory is
registered only once when statically initialized.

More specifically, it applies the following changes:

 (1) Uses of "RegisterFactoryInternal" to make sure the factory
     is registered once into the TransformIOFactory.
     See 39b4ab3 (BUG: Internal factory must use
     RegisterFactoryInternal method) for more details.

 (2) Introduces ITKIOMesh explicit instantiation where
     templated reader and writer are explicitly instantiated over
     dimensions, types and traits.
     This will allow templated factories to be registered in one
     translation unit and instantiated in an other.
     See http://stackoverflow.com/questions/8024010/why-do-template-class-functions-have-to-be-declared-in-the-same-translation-unit

 (3) Update TestDriver to explicitly register the MeshIO
     factories needed by QuadEdgeMeshFiltering and ITKIOMesh tests.

Note that in a follow-up commit, the MeshIO library will be split into
multiple modules MeshIOBase and one for (or one for each) Mesh IO factories.

Open questions:

(1) It is not clear why on Visual Studio 2008, the export
macro need to be removed from MeshFileReader. Otherwise, it reports the
following link error when building ITKIOMeshTestDriver:

  error C2487: 'itk::ProcessObject::SetOutput' : member of dll interface
  class may not be declared with dll interface
    C:\ath/to\ITKv4\Modules\IO\Mesh\include\itkMeshFileReader.h	77

(2) This commits introduces ITKVoronoi dependency to the ITKIOMesh
module. Moving forward, this could be updated by having every module
explicitly listing instantiations they expect.

Change-Id: I80ea34becae742a63b5c98220b20003303188d9c
…sortium#3393

This change updates Mesh IO infrastructure to be consistent with
the Image and Transform ones.

Each Mesh factories along with the UseITK file have been updated to
ensure static initialization works. "Private" function has been removed
from MeshIOFactory and added to each Mesh format specific factories.

Change-Id: I213e51b2a41677d3430e08cf1d81b41b3221c6ec
This commit updates explicit instantiation to use "extern" keyword similarly
to what was done in InsightSoftwareConsortium/ITK@b72726c (BUG: Explicitly
instantiate common MetaDataObjects).

While "extern" is only part of the c++ standard starting with c++11 (see
section 14.7.2 of the standard), it is supported by Visual Studio 2008,
GCC and Clang using compiler specific extension in earlier version:

* Visual Studio 2008: https://msdn.microsoft.com/en-us/library/by56e477(v=vs.90).aspx

* Gcc: https://gcc.gnu.org/onlinedocs/gcc-4.8.4/gcc/Template-Instantiation.html#Template-Instantiation

* Clang (since v2.9): http://clang.llvm.org/cxx_status.html

For completeness, initial draft of the "extern" keyword were first
introduced in 2003 [1] and then revised in 2006 [2], and ultimately
standardized in c++11 [3]:

[1] N1448: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2003/n1448.pdf
[2] N1987: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2006/n1987.htm
[3] N3242: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2011/

See InsightSoftwareConsortium#3393

Change-Id: I74a9260e4ad9996470a54159c55cae6a7fa7f10d
@hjmjohnson
Copy link
Member Author

http://review.source.kitware.com/#/c/20548/

Jean-Chr...on-Robin
Uploaded patch set 1.Dec 11, 2015

Kitware Build Robot
Patch Set 1: Verified+1 Build Successful: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=20548-1&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Dec 11, 2015

Jean-Chr...on-Robin
Uploaded patch set 2: Patch Set 1 was rebased.Dec 11, 2015

Kitware Build Robot
Patch Set 2: Verified-1 Build Failed: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=20548-2&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Dec 11, 2015

Jean-Chr...on-Robin
Uploaded patch set 3: Patch Set 2 was rebased.Dec 11, 2015

Kitware Build Robot
Patch Set 3: Verified-1 Build Failed: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=20548-3&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Dec 11, 2015

Hans J. Johnson
Patch Set 3: Code-Review-1 Does not pass, there is now a merge conflict. Perhaps this is outdated.Jan 6, 2016

Johan Andruejol
Uploaded patch set 4: Patch Set 3 was rebased.Jul 5, 2016

Kitware Build Robot
Patch Set 4: Verified+1 Build Successful: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=20548-4&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Jul 5, 2016

Matt McCormick
Patch Set 4: Code-Review+2Jul 8, 2016

Hans J. Johnson
Patch Set 4: JC: Would you mind rebasing this set of patches so that we can move forward with reviewing and accepting these changes?Nov 28, 2017

Jean-Chr...on-Robin
Patch Set 4: @francois: Considering you recent updates, should we abandon this topic ?Nov 28, 2017

Francois Budin
Patch Set 4: @jc: I'll review the patch. It seems that this patch also does factory registration for Meshes, which the patch that was merged to update the IO factory registration did not modify (see http://review.source.kitware.com/#/c/22759).Dec 1, 2017

Jean-Chr...on-Robin
Patch Set 4: @francois: Indeed, this is the last patch of a multi-commit topic. Thanks for your help with thisDec 1, 2017

@hjmjohnson
Copy link
Member Author

@jcfr @fbudin69500 Is there value in this long outstanding PR from gerrit? It has been stale for almost a year.

@hjmjohnson hjmjohnson closed this Nov 6, 2018
@hjmjohnson hjmjohnson deleted the 3393-transform-image-io-as-shared branch November 6, 2018 14:48
@hjmjohnson hjmjohnson restored the 3393-transform-image-io-as-shared branch November 6, 2018 19:21
@hjmjohnson
Copy link
Member Author

Accidently deleted/closed branch when the script for moving from Gerrit went haywire.

@hjmjohnson hjmjohnson reopened this Nov 6, 2018
@stale
Copy link

stale bot commented Mar 6, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the status:Use_Milestone_Backlog Use "Backlog" milestone instead of label for issues without a fixed deadline label Mar 6, 2019
@stale stale bot removed the status:Use_Milestone_Backlog Use "Backlog" milestone instead of label for issues without a fixed deadline label Mar 6, 2019
@stale
Copy link

stale bot commented Jul 4, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the status:Use_Milestone_Backlog Use "Backlog" milestone instead of label for issues without a fixed deadline label Jul 4, 2019
@dzenanz
Copy link
Member

dzenanz commented Jul 5, 2019

I think that a lot of this got integrated in other patches over the years. @jcfr Can you check whether this can be closed, or something still needs to be done?

@stale stale bot removed the status:Use_Milestone_Backlog Use "Backlog" milestone instead of label for issues without a fixed deadline label Jul 5, 2019
@hjmjohnson
Copy link
Member Author

This is now so far outdated that the current PR is probably no longer relevant as a starting point.

@hjmjohnson hjmjohnson closed this Aug 28, 2019
@jcfr
Copy link
Contributor

jcfr commented Aug 29, 2019

For reference, these commits were integrated as 1869a17

@hjmjohnson hjmjohnson deleted the 3393-transform-image-io-as-shared branch October 23, 2019 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants