Skip to content

[Constraint/Lagrangian] Replace d_index by d_indices in StopperLagrangianConstraint - #6381

Merged
fredroy merged 4 commits into
sofa-framework:masterfrom
CRIStAL-PADR:pr-change-index-footerconstraint
Nov 6, 2026
Merged

fredroy merged 4 commits into
sofa-framework:masterfrom
CRIStAL-PADR:pr-change-index-footerconstraint

Conversation

@damienmarchal

@damienmarchal damienmarchal commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

This allows to apply the constraint to one or more dofs instead of a single one.
This is consistant with other lagrangian constraint component.

[with-all-tests]


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

This is consistant with other LagrangianConstraint.
@damienmarchal damienmarchal added pr: breaking Change possibly inducing a compilation error pr: status to review To notify reviewers to review this pull-request labels Oct 12, 2026

@bakpaul bakpaul left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

An by the way I don't like this class at all, it is pretty bad design in term of application.

Comment on lines +52 to +61
if(!d_indices.isSet())
{
auto indices = sofa::helper::getWriteOnlyAccessor(d_indices);
indices.resize(x.size());
for(sofa::Index i=0;i<x.size();i++)
{
indices[i] = i;
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This changes the default behavior of the ocmpoent, forcing to apply the constraint to all points if nothing is given. To keep old behavior you should only put it to the first vertex. Otherwise this PR should be marked as breaking

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank for your comment,

The PR is actually already flagged as breaking.
That's said, I think the underlying question is more what are the SOFA guidelines regarding this indices/index stuff to increase component consistency.
I searched in the doc, but cannot find guidelines, maybe worth to discuss and formalize (if not done yet).

A quick look in Lagrangian constraint plugin already contains three different pattern.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have opened an issue regarding the behavioral in-consistencies on indices/index
#6418

@damienmarchal

damienmarchal commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor Author

An by the way I don't like this class at all, it is pretty bad design in term of application.

Can you elaborate a bit more on what would be a good design in term of application.

On my side I find it useful to implement a collision response through a non-penetration constraint by preventing values to be <0. Is there alternative component for such a thing ?

@hugtalbot hugtalbot changed the title [Constraint/Lagrangian] Replace d_index by d_indices in FooterLagrangianConstraint [Constraint/Lagrangian] Replace d_index by d_indices in StopperLagrangianConstraint Oct 29, 2026
@damienmarchal

Copy link
Copy Markdown
Contributor Author

Anything to change on this PR ?

@hugtalbot

Copy link
Copy Markdown
Contributor

Last week report: Launch the scenes tests then ready

@hugtalbot

Copy link
Copy Markdown
Contributor

I restarted Ubuntu other OS seem OK

@fredroy fredroy 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 Nov 6, 2026
@fredroy
fredroy merged commit b862477 into sofa-framework:master Nov 6, 2026
34 of 41 checks passed
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 ready Approved a pull-request, ready to be squashed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants