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 stuck cursor in Advanced Scene Importer #84661

Merged

Conversation

Sauermann
Copy link
Contributor

@Sauermann Sauermann commented Nov 9, 2023

Display an Arrow mouse cursor, while the mouse is moved within the SubViewportContainer of the Advanced Scene Importer.

resolve #84002
alternative to #84003

Related to the comment #79805 (comment)
This is a quick solution. Implementing the methodology of this comment would take more time.

Display an Arrow mouse cursor, while the mouse is moved within the
`SubViewportContainer` of the Advanced Scene Importer.
@akien-mga
Copy link
Member

As a workaround it's a bit cleaner than #84003, so this might be ok for the time being. CC @SaracenOne @YuriSizov

But I wonder what other editor subviewports and those used in user projects might suffer from a similar regression.

The other option would be to revert #79805 for the time being, if we consider that the bug it fixed is not as critical as having some potential regressions in user projects.

@YuriSizov
Copy link
Contributor

YuriSizov commented Nov 9, 2023

But I wonder what other editor subviewports and those used in user projects might suffer from a similar regression.

I'm pretty sure there was another one the last time we looked into it? But I don't recall what it was.

The issue definitely has a potential to affect any container/viewport, in user projects as well, in a similar configuration. So while this workaround is definitely better, it's only fixing one specific case of this issue manifesting. So I dunno. A revert may be safer, but I'll let @Sauermann to be the judge of that.

Edit: Remi found the other one. I guess not exactly the same, but a setup is similar to #84003 so the issues can be related: #83425

@akien-mga
Copy link
Member

Edit: Remi found the other one. I guess not exactly the same, but a setup is similar to #84003 so the issues can be related: #83425

Yeah I thought those were similar, but after further testing #83171 isn't a regression, and may not be a bug. So it's unrelated to #79805.

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 think this can be fine as a stopgap solution.

But we should open an issue to keep track of #79805 (comment), and link that issue as a comment in this fix to clarify that this is a temporary measure.

We can hope that the regression won't affect too many users in production, and if it does it's not a deal-breaker as it's mostly cosmetic. We should point out the change in the changelog/migration guide with some advice for workaround.

@Sauermann
Copy link
Contributor Author

Sauermann commented Nov 9, 2023

So while this workaround is definitely better, it's only fixing one specific case of this issue manifesting. So I dunno. A revert may be safer, but I'll let @Sauermann to be the judge of that.

The regression got merged into master between 4.2-dev1 and 4.2-dev2, so 4.1 is not affected by it.

4.1.3 is still affected by the bug #74805
Reverting #79805 would reintroduce the same bug into 4.2 that is in 4.1. So it is safe in the sense, that it isn't worse than the previous release.

Personally I find the situation in 4.1 worse than in 4.2-beta5, because in 4.1 the workaround would require much more work involving overriding Cursor.get_cursor_shape and accessing child nodes, while currently in 4.2 as a workaround you would only need to call DisplayServer.cursor_set_shape on entering the SubViewportContainer. So I'm inclined to not revert #79805.

The implementation of this PR is not optimal, since it sets the cursor shape on every mouse move and not on entering the node, but since this implementation is much easier and it is only a single node-type and I see it as a temporary solution, I went with this way of doing it.

Regarding breaking existing projects: It is very difficult for me to compare the amount of breaking projects against the number of projects that are hindered by #74805.

My take on #83171 is written in #83425 (comment) and I think, that it is unrelated to the issue here.

@akien-mga akien-mga merged commit 8c53a72 into godotengine:master Nov 10, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@Sauermann Sauermann deleted the fix-cursor-in-advanced-importer branch November 12, 2023 20:08
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.

HSplit cursor gets stuck in Advanced Scene Importer when hovering 3D preview
3 participants