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

Control Default Cursor Shape has no effect in a SubViewport #74805

Closed
ejektaflex opened this issue Mar 12, 2023 · 11 comments · Fixed by #79805
Closed

Control Default Cursor Shape has no effect in a SubViewport #74805

ejektaflex opened this issue Mar 12, 2023 · 11 comments · Fixed by #79805
Milestone

Comments

@ejektaflex
Copy link

Godot version

4.0

System information

Windows 11

Issue description

When using a SubViewport contained within a SubviewportContainer, none of my child Control nodes within the Subviewport have their mouse cursors changed to align with their Control's default mouse cursor option. They are always unchanged, even when an attempt is made to change the default with code. It seems like being within a SubViewport disables it's ability to know when it needs to change mouse cursors inside that SubViewport.

Steps to reproduce

  • Create a Control node root.
  • Put a SubViewportContainer inside it
  • Put a SubViewport inside of that
  • Put a ColorRect inside of the SubViewport and change it's default cursor to something else, such as "Forbidden"
  • Hit "Play" and mouse over the ColorRect.

Minimal reproduction project

TestRepro.zip

@Sauermann
Copy link
Contributor

The reason for this behavior is, that the Cursor Shape of the SubViewportContainer overrides the Cursor Shape of the inside ColorRect.

As a solution, I would suggest the following approach:

  • Ignore the Cursor Shape property of SubViewportContainer and use the pass-trough Cursor Shape of the SubViewport.

I have an idea of how to implement this change, but since this is a breaking change, I would like feedback, if there are better solutions.

@kumikumi
Copy link
Contributor

kumikumi commented May 26, 2023

I'm affected by this issue. More specifically, when moving the mouse, the cursor flickers between the proper cursor for the child node, and the default cursor for the SubViewportContainer (but the SubViewportContainer default cursor overrides it and gets shown most of the time). This is in an X11 Linux environment.

Edit: The cursor flickering is very occasional and I can only reproduce it with Vsync disabled

@Sauermann did you get feedback for your idea? Is there a workaround while we wait for the fix?

@Sauermann
Copy link
Contributor

Sauermann commented May 26, 2023

did you get feedback for your idea?

Nobody did answer in this thread, so: no.

After looking at the problem again, maybe my previous idea is not the best possible way to deal with this situation.

In the following code-section, Control::CURSOR_ARROW is used for two different intentions:

  • indicator for the default mouse cursor
  • indicator that the control has no cursor set

If a separate mouse cursor enum value was available for the second one, then it could be used for SubViewportContainer to indicate that the Control should not set a mouse cursor. But this would still not be enough to solve the problem completely, because there might be a parent Control behind the SubViewportContainer, which could interfere with the cursor shape.

godot/scene/main/viewport.cpp

Lines 1956 to 1978 in 2eec9a6

Control::CursorShape cursor_shape = Control::CURSOR_ARROW;
{
Control *c = over;
Vector2 cpos = pos;
while (c) {
if (!gui.mouse_focus_mask.is_empty() || c->has_point(cpos)) {
cursor_shape = c->get_cursor_shape(cpos);
} else {
cursor_shape = Control::CURSOR_ARROW;
}
cpos = c->get_transform().xform(cpos);
if (cursor_shape != Control::CURSOR_ARROW) {
break;
}
if (c->data.mouse_filter == Control::MOUSE_FILTER_STOP) {
break;
}
if (c->is_set_as_top_level()) {
break;
}
c = c->get_parent_control();
}
}

I might get back to this after #67791, which changes the way, how the Control is calculated above which the mouse cursor is. That new algorithm identifies the Control (even within nested SubViewports and embedded Windows) where the cursor is and this would provide a different, more natural solution for this issue.

@Insane-Owl
Copy link

This is still an Issue in Godot 4.1.1 stable

@Koyper
Copy link
Contributor

Koyper commented Jul 22, 2023

How about creating a new internal signal emitted by SubViewport whenever the cursor shape is set, and SubViewportContainer connects to this new subviewport signal at runtime, and sets its mouse_default_cursor_shape in accordance with the signal?

This behavior could be disabled by a new property if necessary for backward-compatibility.

@kumikumi
Copy link
Contributor

The problem is that SubViewportContainer's "mouse_default_cursor_shape" property is needlessly overriding the proper cursor that is already set by whatever is rendered in the viewport.

I did this as an experiment:

In my SubViewportContainer, set mouse_default_cursor_shape to "Can Drop" (arbitrarily selected a cursor that I don't need right now)

Then in Godot's scene/main/viewport.cpp add this check around the part that sets the cursor at line 2077 in Godot 4.1:

if (DisplayServer::get_singleton()->has_feature(DisplayServer::FEATURE_CURSOR_SHAPE)) {
	if (ds_cursor_shape != DisplayServer::CURSOR_CAN_DROP) {
		DisplayServer::get_singleton()->cursor_set_shape(ds_cursor_shape);
	}
}

Lo and behold, the cursors inside the viewport work now, as they are not being overridden.

The proper fix would involve either allowing us to set a "null" value for the cursor shape, or changing the code in a way that SubViewportContainer's own cursor setting is discarded so that the cursor set by the viewport contents stays in effect.

@Sauermann
Copy link
Contributor

#67791 rewrites the methodology, how the Control is determined, which is under the mouse cursor.
My current estimation is, that with these changes it becomes much easier to correctly set the Mouse cursor shape based on Controls within a SubViewport.

@Koyper
Copy link
Contributor

Koyper commented Jul 22, 2023

Lo and behold, the cursors inside the viewport work now, as they are not being overridden.

Thanks for figuring out a workaround - I might try this out until a fix happens after #67791 is merged.

@Sauermann Purely out of curiosity, is the original reason SubViewportContainer is setting the cursor shape, coming from the way the 3.x Viewport always intercepts gui input regardless of location in the tree?

@Sauermann
Copy link
Contributor

@Koyper I'm not as familiar with the 3.x-codebase, so I can't really make the comparison.

My understanding of the current implementation is, that first the SubViewport evaluates the CursorShape and sets it.
Afterwards the root Viewport evaluates the CursorShape and sets it.
So it is a problem of the sequence in which the CursorShape is set.
With #67791 it becomes trivial to identify the node that the cursor is over (even within nested SubViewports), so my idea is to set the CursorShape only once and not for for each Viewport separately.

@kumikumi
Copy link
Contributor

kumikumi commented Jul 23, 2023

With #67791 it becomes trivial to identify the node that the cursor is over (even within nested SubViewports), so my idea is to set the CursorShape only once and not for for each Viewport separately.

I made a one-line fix that just skips setting the cursor for SubViewports, I opened it as a PR at #79805

I made a repro with nested SubViewports here that is also fixed by the PR: nested-subviewports-demo.zip

@Sauermann I understand that this may be made obsolete by your effort, but I decided to post it anyway on the off chance that it helps. At least it demonstrates a possible simple way to fix it.

@jonSP12
Copy link

jonSP12 commented Aug 23, 2024

In the editor a diferent cursor shape gets stuck instead of the arrow ?

cursorShapeStuck.mp4
  • Changing the cursor shape in script doesnt seem to work....
  • The only way i found how to change it is to add another control node at the end of the tree.
  • But then those things can't be selected inside the subViewPortContainer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants