godotengine / godot

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

Godot 4.1.1: Questioning today's usefulness of Sprite2D property "region_filter_clip_enabled" #87847

Open MeanRaccoon opened 7 months ago

MeanRaccoon commented 7 months ago

Tested versions

Reproducible in (please see screenshots) image image

System information

Godot v4.1.1.stable.mono - Windows 10.0.19045 - Vulkan (Compatibility) - Intel(R) HD Graphics 630 (Intel Corporation; 27.20.100.8280) - Intel(R) Core(TM) i5-7400 CPU @ 3.00GHz (4 Threads)

Issue description

I've been testing and studying in detail Sprite2D node, its capabilities, methods and properties. I am questioning the usefulness of the boolean property region_filter_clip_enabled because I've been unable so far to make it work how I imagined it should work, despite all my testings. I understand also this subject has to do with the concept of "pixel bleeding". According to the docs:

_If true, the outermost pixels get blurred out. region_enabled must be true._

By "outermost pixels", I believe we're talking about the pixels at the border of the selected rectangular region, though the thickness of this border is not specified (1 pixel, 2 pixels witdh...?).

I've been discussing the issue in Godot's communities, and I've been taught that spritesheets with tighly grouped sprites could lead to pixel bleeding. With this set up, I could start testing out the functionality of the aforementioned property. When set to true, I expected this property (at least to a certain degree) to mitigate the pixel bleeding that arises when the CanvasItem texture_filter is set to any of its linear variations (linear, linear mipmap, linear mipmap anisotropic), but I see no difference at all, which leads to suspect if this property is working properly, or if its been deprecated long ago and now is rubble.

Meanwhile, when texture_filter is set to any of its "nearest" variations, no bleeding occurs regardless of the value of region_filter_clip_enabled (the preferred case for pixel art).

Steps to reproduce

All these screenshots were made in Godot 4.1.1. To test this out, I used this classic Sokoban spritsheet from Kenney's assets, where the boxes sprites have no spacing at all between them:

image

Carefully select the region for this blue box:

image

Try out many combinations of region_filter_clip_enabled and texture_filter properties. (Not all texture_filter are shown in this screenshot because the visual result repeats itsel,f as explained in the issue description above):

image

Minimal reproduction project (MRP)

Sprite2D region_filter_clip_enabled test.zip

Calinou commented 7 months ago

Please test this on 4.2.1 to make sure the issue is still present in the latest Godot version.

region_filter_clip_enabled is only used a single time in the Sprite2D code (excluding getter/setter and binding): https://github.com/godotengine/godot/blob/10e111477db68fe65776a1d68fb1ffccaf6520fc/scene/2d/sprite_2d.cpp#L82

MeanRaccoon commented 7 months ago

Here's the testing in v4.2.1. To my eyes, the results are the same as for v4.1.1

image

image

Mickeon commented 7 months ago

It's... weird. Very lacking class reference description aside, all that property does is clip the UVs. You did understand what it's supposed to do, and yet it does not seem to work.

The same happens with the equivalent draw_* method. Changing the last parameter clip_uv does not affect anything with linear filtering.

draw_texture_rect_region(preload("res://sokoban_spritesheet.png"),
    Rect2(32, 32, 64, 64), Rect2(256, 256, 64, 64), Color.AQUA, false, true)

image

Mickeon commented 7 months ago

I think this should be classified as a bug. Something is not right here.

MeanRaccoon commented 7 months ago

I don't know yet enough C++ to dig into the details of the source code, but it seems this feature has indeed not been developed. Not even in Godot 3.x I was able to obtain any result whatsoever by doing that experiment.

The way texture_filter was used in Godot 3.x looks different because the GUI has changed significantly. The user had to change the texture filter from the Import tab (unlike in Godot 4.x, which is done in the inspector window)

image

Also the concept of "nearest" for texture filter didn't seem to be very popular. This is what the docs look like for v3.5.3 and v4.2.1:

image

Last thing I'd like to add in this reply is that it seems simple to avoid pixel bleeding: by working with a nicely built spritesheet, with transparency and enough space between the individual sprites. These are my impressions so far.

romlok commented 6 months ago

FWIW, I'm also experiencing the same with AtlasTexture's filter_clip property. On or off, it doesn't seem to make any difference. :confused:

v4.2.1.stable.official [b09f793f5]

Mickeon commented 6 months ago

I've seen enough to classify this omission as a bug at this point.

CC @clayjohn you may know something no one does

kleonc commented 6 months ago

I've digged a little and here's a brief history of this property:


Clamping UV like that doesn't affect how the texture sampling is done, filtered sampling still can sample from outside of such region, hence the bleeding.

I'd guess shrinking the region which UV are clipped to by some arbitrary threshold (less than the 0.5 texel used in the original implementation) is not a viable solution, as it will probably also introduce some artifacts depending on the scale/zoom/etc.?

The best would be to prevent filtered sampling from sampling outside of such region, but I don't know if this is feasible at all.

And if there's no way to solve this reliably then maybe it should be considered to deprecate this clipping / relevant properties. AFAICT currently it does pretty much nothing (UV are already based on the source rect in case of using region so in theory it doesn't change anything; unless it prevents some floating point issue? :thinking:).

This needs to be assessed by the rendering team. @godotengine/rendering

clayjohn commented 6 months ago

Thanks for doing a deep dive into this I think https://github.com/godotengine/godot/pull/12828 was an incorrect fix. The way I see it, you described two distinct problems:

  1. When using linear filtering on an atlas, filtering results in a small amount of color bleed. Turning on region_filter_clip_enabled should remove that bleed.
  2. When enabling region_filter_clip_enabled pixelated textures using nearest filtering stretch at the edges

To me this indicates that we should restore the half pixel offset (might only need to be a quarter pixel offset) when using linear filtering and remove it with nearest filtering. The trouble with that is that filtering can vary based on the texture itself. In custom shaders this setting becomes a little bit dangerous.

Some background, this code is only ever used when rendering rects or ninepatchs. which means the UV is always calculated by us in the vertex shader: https://github.com/godotengine/godot/blob/68ad520da4365c866ceea42e0238b2ea24647289/drivers/gles3/shaders/canvas.glsl#L216

Then interpolated for each fragment before clipping can happen. Errors can arise in two ways:

  1. imprecision in UV calculation / imprecision in interpolation
  2. exact sampling position in src_rect might be off:

https://github.com/godotengine/godot/blob/68ad520da4365c866ceea42e0238b2ea24647289/drivers/gles3/rasterizer_canvas_gles3.cpp#L940

src_rect is calculated in floating point and may point at a sub pixel position in the first place.

Accordingly I would do the following:

  1. Validate the UVs in the problematic cases (i.e. see if the issue comes from precision/interpolation, or if we are just calculating the UVs badly
  2. See if we can do something to improve the data coming in to the fragment shader (i.e. consider flooring + offsetting pixel positions in src_rec to ensure the sample point always aligns with the source texel)
  3. Then evaluate how to implement the filter clip (i.e., if we are guaranteed that the rect corners are always at texel centers, then we can easily do a 0.25 texel contraction to fix both cases)
oxeron commented 2 months ago

Any news on this subject ? Thanks a lot