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.

Sign up for GitHub

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

Jump to bottom

[SofaVolumetricData] Split the module into multiple plugin #389

Merged
merged 12 commits into from
Sep 21, 2017
Merged

[SofaVolumetricData] Split the module into multiple plugin #389

merged 12 commits into from
Sep 21, 2017

Conversation

damienmarchal
Copy link
Contributor

@damienmarchal damienmarchal commented Sep 6, 2017 •
edited
Loading

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.

@damienmarchal
[SofaVolumetricData] Split the module into multiple plugin
3c80d5f 
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 damienmarchal added enhancement About a possible enhancement pr: status to review To notify reviewers to review this pull-request pr: clean Cleaning the code and removed enhancement About a possible enhancement labels Sep 7, 2017
@damienmarchal
Copy link
Contributor Author

Hi guys...

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

@untereiner
Copy link

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

@damienmarchal
Copy link
Contributor Author

@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
Copy link
Contributor

guparan 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
Copy link

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
Copy link
Contributor Author

damienmarchal commented Sep 11, 2017 •
edited
Loading

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
Copy link
Contributor Author

Not ready ?

damienmarchal and others added 3 commits September 18, 2017 13:47
@damienmarchal
Merge remote-tracking branch 'upstream/master' into splitSofaVolumetr…
7a1ca09 
…icDataSquashed

# Conflicts:
#	applications/plugins/CMakeLists.txt
#	modules/SofaComponentAdvanced/CMakeLists.txt
#	modules/SofaVolumetricData/InterpolatedImplicitSurface.cpp
#	modules/tests/CMakeLists.txt
@damienmarchal
[tests] Fix CMakefile issue.
19dec2d
@guparan
[CI] ADD plugins SofaDistanceGrid and SofaImplicitField to option build
7800ee3
@guparan
Copy link
Contributor

guparan commented Sep 19, 2017

Let's see if it really builds ;-)

@damienmarchal
Copy link
Contributor Author

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

@guparan
Copy link
Contributor

guparan commented Sep 19, 2017

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

@guparan guparan added pr: status wip Development in the pull-request is still in progress and removed pr: status to review To notify reviewers to review this pull-request labels Sep 19, 2017
@damienmarchal damienmarchal mentioned this pull request Sep 19, 2017
Closed
@damienmarchal damienmarchal added the pr: status to review To notify reviewers to review this pull-request label Sep 20, 2017
@damienmarchal damienmarchal removed the pr: status wip Development in the pull-request is still in progress label Sep 20, 2017
@guparan guparan 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 Sep 20, 2017
@damienmarchal
Copy link
Contributor Author

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

@damienmarchal
Copy link
Contributor Author

[ci-build]

@damienmarchal damienmarchal merged commit 32a6597 into sofa-framework:master Sep 21, 2017
@guparan guparan added this to the v17.12 milestone Dec 14, 2017
@damienmarchal damienmarchal deleted the splitSofaVolumetricDataSquashed branch February 12, 2018 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
pr: clean Cleaning the code pr: status ready Approved a pull-request, ready to be squashed
Projects
None yet
Milestone
v17.12
Development

Successfully merging this pull request may close these issues.
3 participants
@damienmarchal @untereiner @guparan