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

Perform update_mouse_cursor_state at most once per frame. #77781

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

Conversation

Sauermann
Copy link
Contributor

@Sauermann Sauermann commented Jun 2, 2023

Set a flag to perform the update after input event processing during NOTIFICATION_PROCESS.

fix no longer #77773 (was solved in a different way)

@KoBeWi
Copy link
Member

KoBeWi commented Jun 2, 2023

This enables processing of all Window nodes, which is a bit wasteful.
I think the queue_redraw() approach would be more suitable (just check how it's implemented; it's a flag + deferred call).

@Sauermann
Copy link
Contributor Author

This enables processing of all Window nodes, which is a bit wasteful.

I was afraid of that and am open to other suggestions.

I think the queue_redraw() approach would be more suitable

queue_redraw() is defined in CanvasItem, so I am not sure how to use it from Window, which doesn't inherit from CanvasItem.

@KoBeWi
Copy link
Member

KoBeWi commented Jun 2, 2023

I mean to use the same approach as that method, not the method itself.
Something like

void update_mouse_cursor_state()
    if (update_queued) return

    update_queued = true
    call_deferred("_update_mouse_cursor_state")

func _update_mouse_cursor_state()
    update here
    update_queued = false

@Sauermann Sauermann force-pushed the fix-mouse-cursor-state-only-once branch from 8360370 to 774f563 Compare June 3, 2023 00:04
@Sauermann
Copy link
Contributor Author

Indeed I prefer this approach much more. Thanks for pointing me in the direction of call_deferred.

Set a flag to perform the update once after input event processing.
Add a color picker unit-test for the related issue.
@Sauermann Sauermann force-pushed the fix-mouse-cursor-state-only-once branch from 774f563 to 19c62f7 Compare June 4, 2023 11:48
@Sauermann Sauermann requested a review from a team as a code owner June 4, 2023 11:48
@Sauermann
Copy link
Contributor Author

Sauermann commented Jun 4, 2023

While I prefer the second solution, I was unable to implement it in a way, that could be merged in a timely manner.

The problem was the interaction of Viewport::update_mouse_cursor_state with Viewport::local_input_handled.
In my opinion the best solution for this would be, that Viewport::update_mouse_cursor_state should not utilize InputEvents.
An initial, not yet ready implementation of this solution is available at the temporary branch https://github.com/Sauermann/godot/tree/wip-fix-77773-better-solution-for-slider-stuck
That solution would make this PR obsolete. However that solution is based on PRs which are unlikely to make it into 4.1.

So I have reverted this PR to its previous state which enables processing on Window nodes, in order to provide a working solution to fix the linked bug.

@Sauermann Sauermann marked this pull request as draft June 5, 2023 08:43
@KoBeWi
Copy link
Member

KoBeWi commented Jun 5, 2023

For lesser performance impact, you could enable process when the update is pending and disable it after update.
Not sure how costly is toggling processing though (for something that potentially can happen relatively often).

@Sauermann
Copy link
Contributor Author

Right, I am not going to use processing, but the flag + deferred call method.

However this will have to wait until I have rewritten update_mouse_cursor_state so that it doesn't use InputEvents.
#67791 is the first step of this rewrite.

But since the referenced bug-report has been solved in a different way, it is fine for me, if this PR take more time.

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