Prevent SubViewportContainer overriding Subviewport's cursor with its own cursor

[kumikumi]

Fixes #74805

A fix to the same issue is being worked on as part of a larger effort at #67791. However, to my knowledge, the cursor issue is not strictly related to notifications or signals, but instead is caused by the fact that SubViewportContainer inherits from Control, and all Control nodes define their own cursor. Currently whatever cursor the SubViewportContainer returns, it overrides the cursor that should be shown based on the contents of the viewport.

This fix is a oneliner that skips setting the cursor for all SubViewportContainer nodes. (One alternative that I can think of would be to allow Control nodes to return a "null" cursor shape from the get_cursor_shape function, which would be handled in viewport.cpp in a way that causes the cursor update to be skipped. This would get rid of the cast.)

Here is a demo with nested subviewports:
nested-subviewports-demo.zip

It shows the default cursor in the master branch, shows the correct I-Beam and resize cursors with this fix applied.

[Sauermann]

I agree with the solution, that SubViewportContainer should not have any effect on the mouse cursor.
Additionally it should be made clear to users, that changing the default cursor shape has no effect. Here is an example of how this could be done:

diff --git a/scene/gui/subviewport_container.cpp b/scene/gui/subviewport_container.cpp
index 9105837486..579e921068 100644
--- a/scene/gui/subviewport_container.cpp
+++ b/scene/gui/subviewport_container.cpp
@@ -247,6 +247,10 @@ PackedStringArray SubViewportContainer::get_configuration_warnings() const {
                warnings.push_back(RTR("This node doesn't have a SubViewport as child, so it can't display its intended content.\nConsider adding a SubViewport as a child to provide something displayable."));
        }
 
+       if (get_default_cursor_shape() != Control::CURSOR_ARROW) {
+               warnings.push_back(RTR("The default mouse cursor shape of SubViewportContainer has no effect.\nConsider leaving it at its initial value `CURSOR_ARROW`."));
+       }
+
        return warnings;
 }
 

[Sauermann]

Looking good to me.

[Sauermann]

One step still needs to happen: Please squash the commits together as explained in this documentation.

[kumikumi]

@Sauermann I just wrote a test case for this, it fails in current master and passes after the fix:

TEST_CASE("[SceneTree][SubViewportContainer] Mouse cursor is not overridden by SubViewportContainer") {
	SubViewportContainer *node_a = memnew(SubViewportContainer);
	SubViewport *node_b = memnew(SubViewport);
	Control *node_c = memnew(Control);

	node_a->set_name("SubViewportContainer");
	node_b->set_name("SubViewport");
	node_c->set_name("Control");
	node_a->set_position(Point2i(0, 0));
	node_c->set_position(Point2i(0, 0));
	node_a->set_size(Point2i(100, 100));
	node_b->set_size(Point2i(100, 100));
	node_c->set_size(Point2i(100, 100));
	node_a->set_default_cursor_shape(Control::CURSOR_ARROW);
	node_c->set_default_cursor_shape(Control::CURSOR_FORBIDDEN);
	Window *root = SceneTree::get_singleton()->get_root();
	DisplayServerMock *DS = (DisplayServerMock *)(DisplayServer::get_singleton());

	// Scene tree:
	// - root
	//   - node_a (SubViewportContainer)
	//     - node_b (SubViewport)
	//       - node_c (Control)

	root->add_child(node_a);
	node_a->add_child(node_b);
	node_b->add_child(node_c);

	Point2i on_c = Point2i(5, 5);

	SEND_GUI_MOUSE_MOTION_EVENT(on_c, MouseButtonMask::NONE, Key::NONE);
	CHECK(DS->get_cursor_shape() == DisplayServer::CURSOR_FORBIDDEN);

	memdelete(node_c);
	memdelete(node_b);
	memdelete(node_a);
}

Should I include it in this PR? Is it OK to add it to test_viewport.h or should it be somewhere else? Any other feedback?

[Sauermann]

It is always beneficial to add tests, so it would be good to include it. If a github issues exists, I usually try to note it in a comment.

CHECK(DS->get_cursor_shape() == DisplayServer::CURSOR_FORBIDDEN); // GH-74805

I would likely make a new test-case in test_viewport.h, because there are also other cursor-shape-tests, that would fit in there:

TEST_CASE("[SceneTree][Viewport] Control mouse cursor shape") {
	SUBCASE("[Viewport][CursorShape] Mouse cursor is not overridden by SubViewportContainer") {
		[...]
	}
}

[YuriSizov]

For the time being this is probably fine. But I think there is a use case for setting some cursor shape to the SubViewportContainer. This new limitation can be worked around with a custom control overlayed on top of the container, but we should probably support this case natively.

[YuriSizov]

Thanks, and congrats on your first merged Godot PR!

[Sauermann]

@YuriSizov

I think there is a use case for setting some cursor shape to the SubViewportContainer

In that case we need to distinguish the cases where the SubViewportContainer wants to force an Arrow-cursor and the cases where the SubViewport Container wants to ignore the mouse-cursor but has the Arrow-cursor as default.

I will keep that in mind.