godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
89.92k stars 21.06k forks source link

ItemList has clipped items, unwanted extra spacing and inconsistent handling of mouse clicks depending on spacing parameters #88217

Open davthedev opened 7 months ago

davthedev commented 7 months ago

Tested versions

System information

Godot v4.2.1.stable - Ubuntu 22.04.3 LTS 22.04 - X11 - Vulkan (Mobile) - integrated Intel(R) Iris(R) Plus Graphics (ICL GT2) () - Intel(R) Core(TM) i7-1065G7 CPU @ 1.30GHz (8 Threads)

Issue description

Depending on the constants set to adjust items spacing on the ItemList control, there are quirks in the rendering and pointer event handling that cause:

I have played a bit, had a look in the source code and attempted to describe how it works. Those are quite a bunch of issues grouped together, for the reason that the root cause is the item positioning algorithm. Below is how it looks when the vertical spacing is set to 10px.

image

Mouse events inconsistency

The mouse detection area for each item is smaller than the perceived selection rectangle. If horizontal (for multiple columns) or vertical spacing is more than zero, all items appear to be touching each other, due to the way the selection / hover rectangle is rendered, but there is a gap between items where clicks do not have any effects. (also reported on #88095)

From the drawing above, the green (B) rectangle on each item is the mouse events target. The blue (C) rectangle is where the selection/hover background is drawn on the corresponding item.

Measurement inconsistency

In the above example, vertical spacing is set to 10px, but the result is 20px more vertical space in comparison to when the same spacing is set to 0. The algorithm ends up adding twice the desired spacing.

Extra spacing at the bottom

image

In this example, the more vertical spacing between items, the larger the gap at the bottom of the list when fully scrolled at the end. Only applies to single-columns lists with scrolling. No issue if vertical spacing is zero.

The extra spacing appears to be half the vertical spacing value, and behaves like being the vertical spacing value regarding mouse events.

Cut-off selection rectangle at the top of single-column list

image

The first item has its selection rectangle cut-off. It is even more visible with larger values for vertical spacing. No cutoff occurs if spacing is 0. This is due to a compensation by the algorithm to attempt having some visual consistency with the baseline of the first row text.

Cut-off selection rectangle on all sides of multiple column lists

image

image

The cutoff effect of the selection rectangle is visible at both the top and bottom of the list when set to multiple columns with icons. Note that the rounding of the corner is missing to notice the cutoff. Leftmost example has no spacing.

Icon margin setting causes text to appear outside the item rectangle

image

image

On the second screenshot, we can see how the previous row text is hidden by the selection. Also notice that the text is clipped beyond the last row. Leftmost example has no spacing.

Refactoring proposal

Due to the presence of multiple issues related by the positioning algorithm, I have a proposal to reimplement the rendering of those items and solve all those bugs at once. I have a nearly working prototype. Needs some discussion as it may cause some little breaking changes.

https://github.com/godotengine/godot-proposals/issues/9076

Steps to reproduce

Open the reproduction project and run the scene. You can play with the window size; layout is anchored to it.

Select items in the different lists to view the issues with spacing and some cut-off elements.

Minimal reproduction project (MRP)

itemlist-issue.zip

Calinou commented 7 months ago
davthedev commented 7 months ago

Sure, there is a possibility to fix the deadzone and the bottom gap with small changes.

The current algorithm computes item positions, uses this reference for the mouse events detection, then the drawing pass grows the box to "close" the gap. It would require bringing the second growth pass to the box sizing calculation.

I propose to kill three birds with one stone by directly reimplementing the list layout algorithm (see mentioned proposal link), instead of doing separate fixes that will be undone in case the list layout algorithm is redone later. Which solution would make more sense?

Calinou commented 7 months ago

I propose to kill three birds with one stone by directly reimplementing the list layout algorithm (see mentioned proposal link), instead of doing separate fixes that will be undone in case the list layout algorithm is redone later. Which solution would make more sense?

A big refactor is unlikely to be merged in time for 4.3, which is why I'd prefer a fix for the current editor theme issue to land first.

davthedev commented 7 months ago

Bugfix for the click deadzone issue only: #88347

davthedev commented 7 months ago

Bugfix for the following in #88577