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

[Core] Brep Surface - trimmed integration (Part 2 - Inner loop) #13080

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

rickyaristio
Copy link
Contributor

📝 Description
This is the second part of the PR series to fix the problem related to the trimmed integration in the IGA application.
This PR adds additional operations for the inner loop trimming.

Dependencies: #13072

The test cases will be elaborated on in the next PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Some comments:

  1. Why not "solution_outer" instead of "solution" for more clarity and consistence?
  2. Can we propose a better name for the Clipper operation, rather than "d"?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just for the sake of consistency, I would propose separating all the composed names with _, for example: best_vertex instead of bestvertex, or min_weight instead of minweight

Copy link
Contributor

@juancamarotti juancamarotti left a comment

Choose a reason for hiding this comment

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

Just some minor comments regarding format...

@juancamarotti juancamarotti self-requested a review February 4, 2025 07:20
We change "solution" to "solution_outer" and add a proper naming for the inner and outer Clipper operations
juancamarotti
juancamarotti previously approved these changes Feb 4, 2025
Copy link
Contributor

@juancamarotti juancamarotti left a comment

Choose a reason for hiding this comment

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

Ok from my side 👍

@rickyaristio rickyaristio dismissed juancamarotti’s stale review February 4, 2025 12:21

The merge-base changed after approval.

@juancamarotti
Copy link
Contributor

@KratosMultiphysics/technical-committee

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

Successfully merging this pull request may close these issues.

2 participants