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

Fix "selectable nodes at position clicked" feature in 3D editor #94387

Merged
merged 1 commit into from
Jul 26, 2024

Conversation

mihe
Copy link
Contributor

@mihe mihe commented Jul 15, 2024

I just stumbled upon this one, so forgive the issue and PR rolled into one.

The viewport toolbar button titled "Show list of selectable nodes at position clicked", also triggered by Alt+RMB on Windows, is apparently broken (in 3D specifically) as of 97b8ad1, and as far as I can tell was broken by #92188.

The issue is hopefully apparent from looking at the change. The potential_selection_results could inherently never be incorporated into selection_results as its node would only be added to selection_results if selection_results.has(node), which doesn't make much sense.

This PR fixes that by inverting that if-check.

CC @SaracenOne.

@mihe
Copy link
Contributor Author

mihe commented Jul 25, 2024

@AThousandShips @akien-mga This should be labeled as a regression, and I would argue should be merged for 4.3. The list selection in 3D is just broken without it.

It would've been nice to get some input from the PR author, but the change seems relatively harmless to me.

@AThousandShips
Copy link
Member

AThousandShips commented Jul 25, 2024

It is marked for 4.3, so it's likely going to be merged in 4.3 if nothing happens, but it needs a review by the relevant team

But will add regression

@akien-mga
Copy link
Member

It is marked for 4.3, so it's likely going to be merged in 4.3 if nothing happens

But will add regression

In this case it's quite important to tag regressions accurately, as indeed they're the only bugs I care about at this stage. This would likely have been pushed to 4.4 this week as we're retriaging everything still in 4.3 if not re-reviewed correctly (I would have caught that it's a regression but maybe not all triagers would have).

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

I already started a build for 4.3.rc1 so this will be for rc2.

@AThousandShips
Copy link
Member

In the future this would be helped by having a dedicated issue report as well clarifying when and what happened as it was somewhat vague from the description here

@mihe
Copy link
Contributor Author

mihe commented Jul 25, 2024

Fair enough. I'll avoid combining the issue and PR in the future.

@akien-mga akien-mga merged commit 6a1ac99 into godotengine:master Jul 26, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks!

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.

3 participants