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][Geometry] Old closest point deprecation #9243

Merged
merged 4 commits into from
Oct 21, 2021

Conversation

rubenzorrilla
Copy link
Member

📝 Description
This PR follows #9024 to deprecate the old ClosestPoint methods from the Geometry class as we agreed in #8491.

🆕 Changelog
Please summarize the changes in one list to generate the changelog:
E.g.

  • Deprecation of the ClosestPoint method in favor of the ClosestPointGlobalToLocalSpace and ClosestPointLocalToLocalSpace.

@rubenzorrilla rubenzorrilla self-assigned this Oct 21, 2021
@rubenzorrilla rubenzorrilla added this to To do in LEGACY Technical Committee via automation Oct 21, 2021
@rubenzorrilla rubenzorrilla moved this from To do to Next meeting todo in LEGACY Technical Committee Oct 21, 2021
@rubenzorrilla rubenzorrilla requested review from a team and tteschemacher October 21, 2021 08:42
@tteschemacher
Copy link
Contributor

Thanks for the changes.

Shall we add a date until when this function will be removed completely?

// Projection failed
return -1;
}
return ClosestPointGlobalToLocalSpace(rPointGlobalCoordinates, rClosestPointLocalCoordinates, Tolerance);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return ClosestPointGlobalToLocalSpace(rPointGlobalCoordinates, rClosestPointLocalCoordinates, Tolerance);
const int result = ClosestPointGlobalToLocalSpace(rPointGlobalCoordinates, rClosestPointLocalCoordinates, Tolerance);
if (result == 1) {
this->GlobalCoordinates(rClosestPointGlobalCoordinates, local_coordinates);
}
return result;

Not that this is of major importance as it will be removed. But I think the correct term is as this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree. But note that I'm keeping the previous behavior, which was wrong (see the deprecation message).

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, no worries, then.

@rubenzorrilla
Copy link
Member Author

Thanks for the changes.

Shall we add a date until when this function will be removed completely?

I think we should. Considering that there are only a very few usages I think we can safely remove them after the release.

@rubenzorrilla rubenzorrilla marked this pull request as ready for review October 21, 2021 09:27
@tteschemacher
Copy link
Contributor

I think we should. Considering that there are only a very few usages I think we can safely remove them after the release.

Sounds good to me. I have removed my use cases. I'd be fine with the removal.

@rubenzorrilla rubenzorrilla added this to Upcoming in Deprecations Oct 21, 2021
@rubenzorrilla rubenzorrilla merged commit fdd9bfe into master Oct 21, 2021
@rubenzorrilla rubenzorrilla deleted the core/deprecate-old-closestpoint-methods branch October 21, 2021 12:39
LEGACY Technical Committee automation moved this from Next meeting todo to Done Oct 21, 2021
Deprecations automation moved this from Upcoming to Already Done Oct 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Deprecations
  
Already Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants