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

[SofaVolumetricData] Split the module into multiple plugin #389

Merged

Conversation

3 participants
@damienmarchal
Copy link
Contributor

commented Sep 6, 2017

This PR is here to support Issue #388

This is the beginning of a work on cleaning and modularizing the
SofaVolumetricData.
The module is splitted in two Plugins.

  • SofaDistanceGrid
  • SofaImplicitField

A third plugin act as a transitional package SofaVolumetricData guiding
other developpers on the change they have to do in order to have their
code compiling again.

CHANGELOG.txt:

  • SofaVolumetricData was strongly refactored and splitted in two plugins SofaDistanceGrid and SofaImplicitField. Please report to sofa-dev any broken behavior. A transitional plugin SofaVolumetricData is provided for one year.

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.

[SofaVolumetricData] Split the module into multiple plugin
This is the beginning of a work on cleaning and modularizing the
SofaVolumetricData.
The module is splitted in two Plugins.
- SofaDistanceGrid
- SofaImplicitField

A third plugin act as a transitional package SofaVolumetricData guiding
other developpers on the change they have to do in order to have their
code compiling again.
@damienmarchal

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2017

Hi guys...

OMG no one answered in 5 days !!
Still 2 days and it will be merged as-is.
Are you sure :) ?

@untereiner

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2017

Is there a removal date for the third plugin ? (To force developers to make the changes)

@damienmarchal

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2017

@untereiner thank for the question.

What I would dream of some kind of consensus about how we proceed and I'm totally open for suggestions.

I see the goods of making smooth transition in a code base (user of the code base will praise you) and the bad of maintaining the transitional package.

On my side... I would say:

  • we remove the transitional packages after 1 year and at each Sofa release we make their use more and more "verbose".
@guparan

This comment has been minimized.

Copy link
Member

commented Sep 11, 2017

Hi @damienmarchal, thank you for this massive work.
117 changed files in 1 commit is not easy to review so this may take a while to be merged but be sure we (I included) are going to check it out.
About the deprecation policy, I agree with your proposal of 1 year transition + highlights in releases.
I'm curious to see this PR [ci-build]'ed [with-scene-tests] 😉

@untereiner

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2017

I agree with you but to avoid calendar issues I would speak rather in term of release: Two releases for the transition and remove at the third one.

@damienmarchal

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2017

Thanks you all the for the feedback.

I appologize about the reviewing work. And you are right, this one is very hard. We can also be a bit more "lazy" in the reviewing, merging it, write a good changelog a tell people to send feedback if something goes wrong.

I'm pushing this one because we have a nice other PR waiting and this one have new cool features (from distance field modeling to tetrahedral meshing).

EDIT: actually moving from module to plugins without refactoring is much easier...but well... I was not able to prevent me.

@damienmarchal

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2017

Not ready ?

damienmarchal and others added some commits Sep 18, 2017

Merge remote-tracking branch 'upstream/master' into splitSofaVolumetr…
…icDataSquashed

# Conflicts:
#	applications/plugins/CMakeLists.txt
#	modules/SofaComponentAdvanced/CMakeLists.txt
#	modules/SofaVolumetricData/InterpolatedImplicitSurface.cpp
#	modules/tests/CMakeLists.txt
@guparan

This comment has been minimized.

Copy link
Member

commented Sep 19, 2017

Let's see if it really builds ;-)

@damienmarchal

This comment has been minimized.

Copy link
Contributor Author

commented Sep 19, 2017

Thanks for spotting that... can you do the same for the others plugnization ?

@guparan

This comment has been minimized.

Copy link
Member

commented Sep 19, 2017

I did, the plugins are activated by default in the others.

@damienmarchal

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2017

[ci-build] to take into account the last merge.

@damienmarchal

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2017

[ci-build]

@damienmarchal damienmarchal merged commit 32a6597 into sofa-framework:master Sep 21, 2017

5 checks passed

Dashboard Builds triggered.
Details
centos_clang-3.4_options OK (tests ignored, see details)
Details
mac_clang-3.4_options OK (tests ignored, see details)
Details
ubuntu_gcc-5.4_options OK (tests ignored, see details)
Details
windows7_VS-2015_options_amd64 OK (tests ignored, see details)
Details

@guparan guparan added this to the v17.12 milestone Dec 14, 2017

@damienmarchal damienmarchal deleted the SofaDefrost:splitSofaVolumetricDataSquashed branch Feb 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.