Skip to content

Conversation

@th-skam
Copy link
Collaborator

@th-skam th-skam commented Aug 5, 2025

Info & Merging

  • A series of PRs to clean-up and simplify the algorithm, and make it easier to make additions in the future.
  • Suggest to squash-merge each PR into the dev-refactor branch and then rebase-merge that into the master branch.

Dependencies

Changes

  • The removal and addition of coupling points should be exclusive with respect to one another (within the same time step). With this PR, we first check whether a coupling point should be added and, only in case it does not, we check whether it should be removed. This avoids an inconsistent scenario where a point could both be added and removed as the two checks could be true at the same time.

@th-skam th-skam added pr: enhancement pr: fix pr: status to review To notify reviewers to review this pull-request labels Aug 5, 2025
@th-skam th-skam changed the base branch from dev-refactor to master August 7, 2025 12:54
@th-skam th-skam force-pushed the rework6-algorithm-logic branch from 9423090 to 871e747 Compare August 8, 2025 07:20
@th-skam th-skam requested a review from epernod August 8, 2025 10:24
const SReal dist = ab.norm();
if (dist > d_tipDistThreshold.getValue())
const type::Vec3 tip2Pt = m_couplingPts.back()->getPosition() - tipProx->getPosition();
if (tip2Pt.norm() > d_tipDistThreshold.getValue())
Copy link
Contributor

@epernod epernod Aug 8, 2025

Choose a reason for hiding this comment

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

I checked in the full method code, this is not the case here, but just FYI, if possible, .getValue() should not be called inside a for loop because they imply some memory access that the compilator can't optimize (due to the Data container code etc...)

So it is better to do:

const auto& data = d_data.getValue();
for (;;)
{
   mycheck(value, data);
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah yes, that rings a bell. I'll actually adjust this in the next PR 'cause it's a good tip.

@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 8, 2025
@epernod epernod merged commit c72dfa1 into master Aug 8, 2025
@epernod epernod deleted the rework6-algorithm-logic branch August 8, 2025 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr: enhancement pr: fix pr: status ready Approved a pull-request, ready to be squashed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants