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

[CGal plugin][CImgPlugin] Image data moved from Image/ to CImgPlugin/ #1104

Merged
merged 4 commits into from
Jul 26, 2019

Conversation

omarboukhris
Copy link

This "move" is originally done to uncouple CGAL plugin and Image. This was done by moving Image class (along with dependencies) from Image plugin (ImageTypes.h) to CImgPlugin (CImgData.h).

The point is to reduce compilation time since CImg takes less time to compile than Image.
Now Image and CGAL both depend on CImgPlugin which is lighter.


This PR:

  • builds with SUCCESS for all platforms on the CI.
  • does not generate new warnings.
  • does not generate new unit test failures.
  • does not generate new scene test failures.
  • does not break API compatibility.
  • is more than 1 week old (or has fast-merge label).

Reviewers will merge only if all these checks are true.

@epernod epernod added location: plugins refactoring Refactor code pr: status to review To notify reviewers to review this pull-request labels Jul 18, 2019
@epernod epernod changed the title Image data moved from Image/ to CImgPlugin/ [CGal plugin][CImgPlugin] Image data moved from Image/ to CImgPlugin/ Jul 18, 2019
Copy link
Contributor

@epernod epernod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is a shame that git doesn't see the move from ImageTypes to ImageData as the same file to see only the real modifications.
I suppose you didn't change the file except the includes and class names ?

@@ -36,7 +36,7 @@
#define CGALPLUGIN_MESHGENERATIONFROMIMAGE_CPP

#include <CGALPlugin/MeshGenerationFromImage.h>
#include <image/ImageTypes.h>
//#include <image/ImageTypes.h>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can remove the line definitively

@@ -45,7 +45,9 @@
#include <sofa/defaulttype/Mat.h>
#include <sofa/defaulttype/Quat.h>
#include <sofa/helper/rmath.h>
#include <image/ImageTypes.h>

//#include <image/ImageTypes.h>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here. You can remove the line.

@@ -86,7 +87,7 @@ add_library(${PROJECT_NAME} SHARED ${HEADER_FILES} ${SOURCE_FILES})

target_compile_options(${PROJECT_NAME} PUBLIC ${CIMG_CFLAGS})
target_compile_options(${PROJECT_NAME} PRIVATE "-DSOFA_BUILD_CIMGPLUGIN")
target_link_libraries(${PROJECT_NAME} SofaCore ${EXTERNAL_LIBS})
target_link_libraries(${PROJECT_NAME} SofaCore SofaSimulationCommon ${EXTERNAL_LIBS})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SofaSimulationCommon is needed?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is not, forgot how it ended there.
I removed it, along with the commented includes and pushed the updated code to remote.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't change the files/code/algos, I just moved classes around.

@guparan
Copy link
Contributor

guparan commented Jul 24, 2019

[ci-build][with-all-tests]

@guparan guparan merged commit 55bb11b into sofa-framework:master Jul 26, 2019
@guparan guparan added this to the v19.12 milestone Jan 14, 2020
@hugtalbot hugtalbot added pr: status ready Approved a pull-request, ready to be squashed and removed pr: status to review To notify reviewers to review this pull-request labels May 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: status ready Approved a pull-request, ready to be squashed refactoring Refactor code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants