Skip to content

Conversation

@NiziL
Copy link

@NiziL NiziL commented May 13, 2025

SED Sprint 2025

Base class Contact, function modified:

  • getCollisionModels
  • setDetectionOuputs
  • createResponse
  • removeResponse

Child classes modified:

  • Sofa: components
    • BarycentricStickContact
    • BarycentricPenaltyContact
    • BaseUnilateralContactResponse
    • RayContact
    • StickContactConstraint
  • child class RegistrationContact modified in Registration plugin (see below)

[ci-depends-on https://github.com/sofa-framework/Registration/pull/24]


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).

@NiziL NiziL added pr: breaking Change possibly inducing a compilation error pr: status wip Development in the pull-request is still in progress refactoring Refactor code labels May 13, 2025
@sofabot
Copy link
Collaborator

sofabot commented May 13, 2025

[ci-depends-on] detected during build #1.

To unlock the merge button, you must

@sofabot
Copy link
Collaborator

sofabot commented May 13, 2025

[ci-depends-on] detected during build #2.

To unlock the merge button, you must

@sofabot
Copy link
Collaborator

sofabot commented May 13, 2025

[ci-depends-on] detected during build #3.

To unlock the merge button, you must

@sofabot
Copy link
Collaborator

sofabot commented May 14, 2025

[ci-depends-on] detected during build #4.

To unlock the merge button, you must

@NiziL NiziL force-pushed the 2025_sedsprint_Contact branch from 789c8a0 to 109fed4 Compare May 14, 2025 11:09
@sofabot
Copy link
Collaborator

sofabot commented May 14, 2025

[ci-depends-on] detected during build #5.

To unlock the merge button, you must

@sofabot
Copy link
Collaborator

sofabot commented May 14, 2025

[ci-depends-on] detected during build #6.

To unlock the merge button, you must

@NiziL
Copy link
Author

NiziL commented May 14, 2025

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

@sofabot
Copy link
Collaborator

sofabot commented May 14, 2025

[ci-depends-on] detected during build #7.

To unlock the merge button, you must

* which is the method to override from now on.
*
**/
virtual void createResponse(objectmodel::BaseContext* group) final
Copy link
Contributor

@damienmarchal damienmarchal May 14, 2025

Choose a reason for hiding this comment

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

I'm not sure it makes sens to move from absrtact method to a virtual final. Shouldn't it be a non virtual one ? (same comment apply everywhere)

Copy link
Author

@NiziL NiziL May 14, 2025

Choose a reason for hiding this comment

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

I'm going to talk about this to the team during the daily meeting, might apply to all the SED Sprint PRs

Copy link
Author

Choose a reason for hiding this comment

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

@damienmarchal Talked about it with the team, for us it's important to declare them as virtual final to ensure child class cannot shadow them.

Copy link
Contributor

@bakpaul bakpaul May 14, 2025

Choose a reason for hiding this comment

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

Yes, I confirm, putting virtual final will make sure no one override it

Copy link
Contributor

Choose a reason for hiding this comment

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

@damienmarchal's point is pertinent but the context must be taken into account:
The function createResponse was virtual. There are chances that someone overloaded this function in a derived class. And there are two cases:

  1. The overload has the override keyword:
    • if the base class function is virtual final, it will trigger a compilation error because of a final method cannot be overloaded.
    • If the base class function is a non-virtual function, it will also trigger a compilation error because there is nothing to override.
  2. The overload has not the override keyword:
    • if the base class function is virtual final, it will trigger a compilation error because it tries to override a final function.
    • If the base class function is a non-virtual function, the function in the derived class will be valid. And that's not what we want. We want to notice the derived class that something changed and the code must be adapted. This case will not notice the developer of the derived class, and will introduce a bug.

Because of that, virtual final is not silly. However, this case should be rare because the override keyword should be widely used. So going with a non-virtual function should not be risky, and be the objective.

@sofabot
Copy link
Collaborator

sofabot commented May 14, 2025

[ci-depends-on] detected during build #8.

To unlock the merge button, you must

@sofabot
Copy link
Collaborator

sofabot commented May 14, 2025

[ci-depends-on] detected during build #9.

To unlock the merge button, you must

@bakpaul bakpaul self-assigned this Jan 6, 2026
@bakpaul bakpaul 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 Jan 6, 2026
@bakpaul bakpaul force-pushed the 2025_sedsprint_Contact branch from 04c368d to ac5ac59 Compare January 6, 2026 10:32
@sofabot
Copy link
Collaborator

sofabot commented Jan 6, 2026

[ci-depends-on] detected during build #10.

To unlock the merge button, you must

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr: breaking Change possibly inducing a compilation error pr: status to review To notify reviewers to review this pull-request refactoring Refactor code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants