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

[All] Remove SOFA_ENABLE_LEGACY_HEADERS usage #4813

Merged

Conversation

fredroy
Copy link
Contributor

@fredroy fredroy commented Jul 5, 2024

A continuation of

but much bigger and less trivial.

This PR removes SOFA_ENABLE_LEGACY_HEADERS, implying:

  • no more collections/deprecated projects which was to ensure the continuity of Sofa.NG
  • some renaming in MultiThreading and SofaCUDA.

The deletion of the layer compat between old and new modules was acted for 23.06 so not a big deal by itself (only 1.5 year late🤷‍♂️ ).
But some olden modules still have some code/components:

  • SofaGraphComponent: with a Gravity component
  • SofaMiscCollision: with DefaultCollisionGroupManager, SolverMerger components and RayTriangleVisitor visitor
  • SleepController: with a SleepControllercomponent
  • SofaValidation: with various measuring stuff components.
    There is also SofaExporter which had some forgotten(?) examples of components which were moved. So the examples have been just moved as well

So what should be done for these components?
IMO (not done yet)

  • Gravity seems useless/not usable.
  • SleepController might maybe probably be interesting
  • DefaultCollisionGroupManager, SolverMerger were bogus and RayTriangleVisitor not used at all
  • SofaValidation may be totally transformed as a plugin (as in applications/plugins)

To be discussed.


By submitting this pull request, I acknowledge that
I have read, understand, and agree SOFA Developer Certificate of Origin (DCO).


Reviewers will merge this pull-request only if

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

@fredroy fredroy added pr: status wip Development in the pull-request is still in progress pr: clean Cleaning the code topic for next dev-meeting PR to be discussed in sofa-dev meeting labels Jul 5, 2024
@fredroy
Copy link
Contributor Author

fredroy commented Jul 5, 2024

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

@fredroy fredroy force-pushed the remove_collections_and_misccollis branch from 7365d3d to d7398bd Compare July 5, 2024 08:06
@fredroy fredroy added pr: status to review To notify reviewers to review this pull-request and removed pr: status wip Development in the pull-request is still in progress labels Jul 9, 2024
@fredroy
Copy link
Contributor Author

fredroy commented Jul 9, 2024

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

@fredroy fredroy force-pushed the remove_collections_and_misccollis branch 2 times, most recently from b3d6f23 to f25b6e9 Compare July 11, 2024 15:09
@hugtalbot
Copy link
Contributor

  • Gravity should be moved into MechanicalLoad
  • SleepController , @bakpaul will take a look
  • SofaMiscCollision should be fully removed
  • SofaValidation: archive
  • SofaExporter: to check

@fredroy fredroy added pr: status wip Development in the pull-request is still in progress and removed topic for next dev-meeting PR to be discussed in sofa-dev meeting pr: status to review To notify reviewers to review this pull-request labels Jul 17, 2024
@bakpaul
Copy link
Contributor

bakpaul commented Jul 18, 2024

So I took a look at the SleepController. It simply puts context to sleep where the mechanical object has a max velocity under a certain threshold. Then wake them up again only when a collision occurs with another object that is moving.
Three remarks :

  • Being put to sleep disables any mechanical visitor to be applied on the node and deactivates its constraint corrections. So no more computing (no system build, no integration, only collision detection)
  • This works well only for scenes where external interaction only arise between two objects colliding, neither by the mean of a change of external forces (dynamic vector field for instance), or of constraint state changes not managed by the collision pipeline (for instance statically defined interaction constraints such as cable constraints).
  • The code seems overly complicated for what it performs but certainly does what it says it does. I didn't try it but it looks ok.

Sincerely, I cannot see myself advising anyone to use it in their scene given the fact that the use case doesn't apply for the majority of the scenes (especially for soft robotics). So I wouldn't mind never seeing it again...

@fredroy fredroy force-pushed the remove_collections_and_misccollis branch from e078ccf to afb6400 Compare July 22, 2024 00:06
@fredroy fredroy added pr: status to review To notify reviewers to review this pull-request and removed pr: status wip Development in the pull-request is still in progress labels Jul 22, 2024
@fredroy
Copy link
Contributor Author

fredroy commented Jul 22, 2024

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

@fredroy fredroy force-pushed the remove_collections_and_misccollis branch from afb6400 to a99dcb8 Compare July 24, 2024 05:18
@hugtalbot hugtalbot 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 Jul 24, 2024
@fredroy fredroy added pr: status to review To notify reviewers to review this pull-request and removed pr: status wip Development in the pull-request is still in progress labels Jul 26, 2024
@fredroy fredroy force-pushed the remove_collections_and_misccollis branch 4 times, most recently from b0d3243 to bcb31f4 Compare July 29, 2024 11:29
@hugtalbot
Copy link
Contributor

hugtalbot commented Jul 29, 2024

Reviews seem to have been taken into account, right @fredroy ?
CI says yes!

@fredroy
Copy link
Contributor Author

fredroy commented Jul 29, 2024

Reviews seem to have been taken into account, right @fredroy ? CI says yes!

Yes

@fredroy fredroy force-pushed the remove_collections_and_misccollis branch 2 times, most recently from 93818b1 to 2f2ffd8 Compare August 1, 2024 01:12
@epernod epernod force-pushed the remove_collections_and_misccollis branch from 2f2ffd8 to f53ee95 Compare August 2, 2024 12:32
@epernod epernod 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 Aug 2, 2024
@epernod epernod merged commit 272772f into sofa-framework:master Aug 2, 2024
14 of 15 checks passed
@fredroy fredroy deleted the remove_collections_and_misccollis branch August 4, 2024 23:22
@hugtalbot hugtalbot added this to the v24.12 milestone Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: clean Cleaning the code pr: status ready Approved a pull-request, ready to be squashed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants