Custom tooltip placement bug when using _make_custom_tooltip

[akien-mga]

Godot version:
3.2 (b3af0b2)
Note: Commit b3af0b2 is needed for RichTextLabel's 'Fit Content Height' property used in the MRP - but the bug is reproducible even without it / without using RichTextLabel.

This is "reproducible" in the master branch but harder to see as a bug since default tooltip also go out of the window just fine now with multiple window support.

OS/device including version:
Linux

Issue description:
When showing tooltips, Viewport is responsible for ensuring that the tooltip panel will be within the bounds of the viewport, to avoid having content cut on the edges:

Control has a virtual method _make_custom_tooltip that can be overridden from scripts to return a custom Control that will be used as the tooltip (still nested in a TooltipPanel in Viewport). When using this, the logic that makes it stay within bounds seems to fail:

I don't know why yet, so opening an issue.

Relevant code in 3.2 branch (might be relevant in master too):

godot/scene/main/viewport.cpp

Lines 1592 to 1632 in 42a3150

	gui.tooltip_popup = which->make_custom_tooltip(tooltip);
	if (!gui.tooltip_popup) {
	gui.tooltip_popup = memnew(TooltipPanel);
	gui.tooltip_label = memnew(TooltipLabel);
	gui.tooltip_popup->add_child(gui.tooltip_label);
	Ref<StyleBox> ttp = gui.tooltip_label->get_stylebox("panel", "TooltipPanel");
	gui.tooltip_label->set_anchor_and_margin(MARGIN_LEFT, Control::ANCHOR_BEGIN, ttp->get_margin(MARGIN_LEFT));
	gui.tooltip_label->set_anchor_and_margin(MARGIN_TOP, Control::ANCHOR_BEGIN, ttp->get_margin(MARGIN_TOP));
	gui.tooltip_label->set_anchor_and_margin(MARGIN_RIGHT, Control::ANCHOR_END, -ttp->get_margin(MARGIN_RIGHT));
	gui.tooltip_label->set_anchor_and_margin(MARGIN_BOTTOM, Control::ANCHOR_END, -ttp->get_margin(MARGIN_BOTTOM));
	gui.tooltip_label->set_text(tooltip);
	}
	rp->add_child(gui.tooltip_popup);
	gui.tooltip_popup->force_parent_owned();
	gui.tooltip_popup->set_as_toplevel(true);
	if (gui.tooltip) // Avoids crash when rapidly switching controls.
	gui.tooltip_popup->set_scale(gui.tooltip->get_global_transform().get_scale());
	Point2 tooltip_offset = ProjectSettings::get_singleton()->get("display/mouse_cursor/tooltip_position_offset");
	Rect2 r(gui.tooltip_pos + tooltip_offset, gui.tooltip_popup->get_minimum_size());
	Rect2 vr = gui.tooltip_popup->get_viewport_rect();
	if (r.size.x * gui.tooltip_popup->get_scale().x + r.position.x > vr.size.x)
	r.position.x = vr.size.x - r.size.x * gui.tooltip_popup->get_scale().x;
	else if (r.position.x < 0)
	r.position.x = 0;
	if (r.size.y * gui.tooltip_popup->get_scale().y + r.position.y > vr.size.y)
	r.position.y = vr.size.y - r.size.y * gui.tooltip_popup->get_scale().y;
	else if (r.position.y < 0)
	r.position.y = 0;
	gui.tooltip_popup->set_global_position(r.position);
	gui.tooltip_popup->set_size(r.size);
	gui.tooltip_popup->raise();
	gui.tooltip_popup->show();

Steps to reproduce:

• See MRP
• Compare the tooltip when you hover "Debug build" when _make_custom_tooltip returns a custom Control (here a PanelContainer) or the default tooltip. Uncomment #return null in addons/debug_watermark/debug_watermark_label.gd to use the default tooltip.

Minimal reproduction project:
godot-debug-watermark.zip
Example code from https://github.com/akien-mga/godot-debug-watermark/tree/custom-tooltip

[rxlecky]

This turns out to be a very simple one. The tooltip code in viewport.cpp is using get_minimum_size() for which Label - used in default tooltip - correctly reports minimum size based on its contents, whereas the RTL - used in your custom tooltip - returns (0, 0) hence the difference in behaviour. This is also reported as issue #18260.

Using get_size() fixes the problem. This seems to be the correct function to call anyway - we want to account for the "true" size, rather than the minimum one. Will open a PR with a fix in a bit.

[rxlecky]

Actually, the documentation on _make_custom_tooltip function has this note attached.

Note: The tooltip is shrunk to minimal size. If you want to ensure it's fully visible, you might want to set its rect_min_size to some non-zero value.

So it was a deliberate decision to use min_size instead of the real size. In that case I'd say this is not a bug, it's an expected behaviour. I'd still argue using a size instead of min_size would be better solution but that's not the decision for me to make.

[akien-mga]

What's puzzling is that even forcing a non-zero rect_min_size doesn't fix the issue. E.g. in my example:

func _make_custom_tooltip(for_text):
	var tooltip = $Tooltip.duplicate()
	var rtl = tooltip.get_node("MarginContainer/RichTextLabel")
	rtl.bbcode_text = for_text
	# Work around https://github.com/godotengine/godot/issues/18260.
	# FIXME: Not working :(
	rtl.rect_min_size = rtl.rect_size
	return tooltip

Still produces the same result.

The tooltip node of type PanelContainer, which is returns as the Control for _make_custom_tooltip, also has a non-zero rect_min_size defined in the scene.

[akien-mga]

The problem with get_minimum_size() is more complex than "just" #18260.

Control::get_minimum_size() doesn't honor rect_min_size set from script, which is used instead in Control::get_combined_minimum_size(). My project uses a PanelContainer, whose PanelContainer::get_minimum_size() actually uses Control::get_combined_minimum_size() for its child nodes, but only for those which are visible in tree... which is likely not the case of the tooltip control in Viewport::_get_gui_show_tooltip() as it's only show()-ed as the last step. So all calculations are done with the (combined) minimum size which has not yet been updated for neither the PanelContainer nor its child RichTextLabel.

This is all pretty messy, I think all *minimum* APIs in Control and derived classes need a serious refactoring.

[akien-mga]

This patch makes setting a custom rect_min_size functional:

diff --git a/scene/main/viewport.cpp b/scene/main/viewport.cpp
index 1063309130..7db7b28916 100644
--- a/scene/main/viewport.cpp
+++ b/scene/main/viewport.cpp
@@ -1607,6 +1607,8 @@ void Viewport::_gui_show_tooltip() {
        }
 
        rp->add_child(gui.tooltip_popup);
+       gui.tooltip_popup->show();
+
        gui.tooltip_popup->force_parent_owned();
        gui.tooltip_popup->set_as_toplevel(true);
        if (gui.tooltip) // Avoids crash when rapidly switching controls.
@@ -1629,7 +1631,6 @@ void Viewport::_gui_show_tooltip() {
        gui.tooltip_popup->set_size(r.size);
 
        gui.tooltip_popup->raise();
-       gui.tooltip_popup->show();
 }
 
 void Viewport::_gui_call_input(Control *p_control, const Ref<InputEvent> &p_input) {

I don't know if this could introduce flickering to have the show() happen before setting the position, but it seems necessary for the min size logic to work.

[akien-mga]

And as I supposed, this also works:

diff --git a/scene/gui/panel_container.cpp b/scene/gui/panel_container.cpp
index 74663c33e3..7e7daa780d 100644
--- a/scene/gui/panel_container.cpp
+++ b/scene/gui/panel_container.cpp
@@ -43,7 +43,7 @@ Size2 PanelContainer::get_minimum_size() const {
 	for (int i = 0; i < get_child_count(); i++) {
 
 		Control *c = Object::cast_to<Control>(get_child(i));
-		if (!c || !c->is_visible_in_tree())
+		if (!c || !c->is_visible())
 			continue;
 		if (c->is_set_as_toplevel())
 			continue;

(exclusive of the other patch)

@reduz changed is_visible() to is_visible_in_tree() in 04c749a when introducing this API, but in this case it's maybe not the right choice as we want to be able to calculate a min size for what would be visible once added to the tree.

[rxlecky]

What's puzzling is that even forcing a non-zero rect_min_size doesn't fix the issue. E.g. in my example:
code
Still produces the same result.

Oh, you are on the right track here! It is indeed the problem with how PanelContainer computes the minimum size. However, I think no engine changes are needed :). Tooltip panel that you are using as a template is not visible in the scene. When you duplicate it, the duplicate - and thus all its children - are also not visible. Hence when the PanelContainer::get_minimum_size() is called, it considers all its children not visible and only returns (20, 20) as a minimum size, which is just the sum of margins as configured in the child MarginContainer. Changing your script to this mostly solves the issue:

func _make_custom_tooltip(for_text):
	# Uncomment this to fix bug.
	#return null

	var tooltip = $Tooltip.duplicate()
	tooltip.visible = true
	var rtl = tooltip.get_node("MarginContainer/RichTextLabel")
	rtl.bbcode_text = for_text
	rtl.rect_min_size = rtl.rect_size
	return tooltip

The only issue still is that the RTL rect_size is not correctly returning the height, so I end up with the tooltip cropped fro mthe bottom a little bit.

[akien-mga]

	# Uncomment this to fix bug.
	return null

This would return a default tooltip, not my custom one :)

[rxlecky]

Whoops, sorry, my bad. Forgot to remove that from the MRP code :)

[rxlecky]

Here's cleaned up code.

func _make_custom_tooltip(for_text):
	var tooltip = $Tooltip.duplicate()
	tooltip.visible = true
	var rtl = tooltip.get_node("MarginContainer/RichTextLabel")
	rtl.bbcode_text = for_text
	rtl.rect_min_size = rtl.rect_size
	return tooltip

The important change is setting tooltip visible after duplicating it so the parent Panel uses the min_size as set in the property

[akien-mga]

So I was about to make a patch with #39677 (comment), but thinking more about it, maybe this should be a requirement on the user side to make their custom tooltip visible before returning it in _make_custom_tooltip?

For 3.2 I'm on the fence, but in master now that tooltips are windows, the custom tooltip passed to _make_custom_tooltip is actually added as child of a new TooltipPanel, which is the one that Viewport makes visible. So it's the user's responsibility to pass a visible Control that will be added as child to TooltipPanel if you want to see anything. (This brings new issues as you can't access the TooltipPanel created by Viewport to customize it, but that's a side issue.)

So in the end, maybe this just needs a note in the documentation of _make_custom_tooltip?

[rxlecky]

Yes, I think it is reasonable to ask of user to return a visible control. Also, it might be just my personal preference, but I tend to save the scene tree branches that I plan on instantiating as scenes, rather than storing them hidden in the containing scene. That way they are stored in the exact state I want them to appear when instantiated. If users prefer this workflow too, they wouldn't even come across this problem with returning invisible control.

[rxlecky]

Actually, I would go as far as to say that the _gui_show_tooltip() should display the popup control exactly as it is returned from the user-defined function, not even change its visible property. The only changes it should make to the returned control is add it to the scene tree and set its position to not "leak" out of the containing window. That way it doesn't make any unexpected changes to the popup properties. And if users return a control that is not visible, it would just not render anything - or in case of 4.0, it would render an empty TooltipPanel.

If I, as a user, find that my custom tooltip is not rendering, I'd find it easier to figure out that I made a mistake and returned an invisible control. If, instead, I get a tooltip but with a wrong position, I would assume I did everything right and there is a bug in the engine - after all, this was your case too.

It would be a whole different story if the _make_custom_tooltip didn't expect a whole new instance of popup control but a control that already exists in the scene tree and is just invisible. In that case, as a user, I'd expect the engine to manage some of the properties of that control, including showing it when hovered and hiding it when no longer hovered.

[rxlecky]

But in the end, this is not really a problem with the popup API, it is more deep-rooted problem with controls and *minimum_size* API as you pointed out. And it really does need some rework to make it work more predictably.