Skip to content

Fix selecting wrong vertices in the Polygon2D editor when adding a polygon #108249

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Nikitf777
Copy link
Contributor

@Nikitf777 Nikitf777 commented Jul 3, 2025

Applied the same fix as in #107932, but for another action: ACTION_ADD_POLYGON. Sorry, I didn't know that this algorithm was used twice in the code. But this one seems to be the last. The interesting thing is that this code seems to be writter by none other than @reduz. I wounder why did he implement it like this.

@Nikitf777 Nikitf777 requested a review from a team as a code owner July 3, 2025 19:20
@Chaosus Chaosus added this to the 4.5 milestone Jul 4, 2025
@KoBeWi
Copy link
Member

KoBeWi commented Jul 4, 2025

This loop could be extracted to a method, it's the same in both cases.

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

Needs rebase.

@Nikitf777 Nikitf777 force-pushed the polygon-2d-editor-fix-selecting-wrong-point branch from e93a58f to abfa249 Compare July 5, 2025 13:24
Vector2 tuv = mtx.xform(editing_points[i]);
real_t dist = tuv.distance_to(mb->get_position());
if (dist < 8 && dist < closest_dist) {
if (tuv.distance_to(mb->get_position()) < 8) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (tuv.distance_to(mb->get_position()) < 8) {
if (tuv.distance_squared_to(mb->get_position()) < 64.0) {

Better to avoid the square root if we don't need the result here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not the only place where it makes sense. I can change it everywhere here or create another pull request with these optimizations.

Copy link
Member

Choose a reason for hiding this comment

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

Been looking into that myself so might make a PR soon, just to avoid double work

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.

4 participants