godotengine / godot

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

PointLight2D doesn't respect Nearest texture filter for normal-maps or shadows #76266

Open ppelikan opened 1 year ago

ppelikan commented 1 year ago

TLDR - jump to this post: https://github.com/godotengine/godot/issues/76266#issuecomment-1751739538


Godot version

4.0.2.stable.official

System information

Linux & Windows, various hardware

Issue description

We can see the expected behavior for PointLight2D - each pixel of the Sprite2D is shaded without any interpolation or filtering, when we zoom in. light_not_scaled_no_normal_map

When the light texture is scaled down, we can see that the shading actually happens for each pixel of the scaled light texture, even if pixels of the Sprite2D are bigger. I assume this is also expected behavior. light_scaled_down_4x_no_normal_map

But when we use the CanvasTexture with NormalMap, suddenly the lighting is being interpolated for each screen pixel (the light pixels suppose to be the same size as sprite's). Setting the texture filter to Nearest does not fix this. light_not_scaled_with_normal_map

It seems that the light() shader is being run for each screen pixel (despite Camera2D zooming), which besides some performance impact, would be acceptable on its own. But this also causes the CanvasTextures to look ugly for pixel-art games with in-game zooming (and it looks even worse for some custom shaders).

Steps to reproduce

  1. Add Sprite2D and set it's texture to CanvasTexture
  2. Load some Diffuse and NormalMap textures
  3. Add Camera2D and zoom it in to see individual pixels clearly
  4. Add PointLight2D with some texture
  5. Set texture filter to Nearest for all components (and observe there is no difference)

Minimal reproduction project

light_pixel_test.zip

lewiji commented 1 year ago

Does the same behaviour occur if you use a shader and manually set the normal map's sampler2d filter mode to nearest? Iirc, the non-shader materials force filtering on for normal maps regardless of the material's sampling mode.

edit: just realised that that may not be true for CanvasTexture specifically, as I'm unsure how it's implemented.

yeah, apologies, just checked and I'm pretty sure the filter mode is applied to all the generated uniforms for the textures in a CanvasTexture. My bad

ppelikan commented 1 year ago

I have noticed that a similar problem happens when we enable shadows and add some LightOccluder2D. (CanvasTexture with Normal-maps isn't needed to see this one).

Screenshot_20230420_103343-1

Should I report this as a separate bug?

clayjohn commented 1 year ago

I have noticed that a similar problem happens when we enable shadows and add some LightOccluder2D. (CanvasTexture with Normal-maps isn't needed to see this one).

Screenshot_20230420_103343-1

Should I report this as a separate bug?

Can you explain what about the screenshot is buggy? The shadow looks totally correct to me

ppelikan commented 1 year ago

Light2D's texture has the same resolution as the Sprite2D, so when the Nearest filter is set for those textures, then the rendered light is pixelated as it should be, but the shadow is smooth and it shouldn't be. Essentially, the pixels of the light are being sliced in the middle.

clayjohn commented 1 year ago

Light2D's texture has the same resolution as the Sprite2D, so when the Nearest filter is set for those textures, then the rendered light is pixelated as it should be, but the shadow is smooth and it shouldn't be. Essentially, the pixels of the light are being sliced in the middle.

Shadows are rendered at Viewport resolution. It sounds like you expect the lighting to happen in "texel" space instead of pixel space. In other words, you expect that the lighting happens on the texels in the source texture instead of the pixels in the Viewport. But that's not what is happening. Lighting happens for each pixel in the destination Viewport. So the shadow is accurately calculated for each pixel.

It sounds like you want everything to be low-resolution. Which you can achieve by reducing the resolution of your window/Viewport.

ppelikan commented 1 year ago

Lighting happens for each pixel in the destination Viewport

I agree (I've mentioned that in the first post).

you expect the lighting to happen in "texel" space instead (...) you want everything to be low-resolution

Exactly. I want the game art (textures and shaders) to be in low resolution, but physics and movement in high resolution. This is a common approach for many games.

I understand it's impossible to execute the light() shading before texture rendering and scaling. The problem is that the shadow and normal-map shading does not match the light texture itself (if it has been scaled with the Nearest setting). This makes Godot actually useless in half of the games based on pixel-art, especially, if they implement some camera zooming mechanism.

As a Godot user I would expect the possibility to switch between smooth light (default) and pixelated light (Nearest filtering seemed most intuitive).

Calinou commented 1 year ago

Exactly. I want the game art (textures and shaders) to be in low resolution, but physics and movement in high resolution. This is a common approach for many games.

This is already supported. This is what the viewport stretch mode achieves :slightly_smiling_face:

Movement and physics still occurs at the same precision level regardless of stretch mode (it only affects visuals).

ppelikan commented 1 year ago

This is already supported. This is what the viewport stretch mode achieves

Yes, I thought about such workaround: Screenshot_20230420_233340-1 (Sprite2D_B is used to display the contents of the SubVievport)

but this forces us to keep the Camera2D_A's zoom value constant and equal to exactly 1.0. Because if we want to give the user a possibility to freely pan and zoom the Camera2D_A, then the problem still occurs.

A dirty workaround would be to add Camera2D_B and use it for zooming and panning, but this is even dirtier workaround and would cause even more problems (e.g. with position calculation, animation, the fact that the SubViewport would have to be the size of the entire game level etc.)

Does every pixel-art game developer really needs to do all of that just to fix the broken lights?

Edit: not to mention, that nothing inside SubViewport could be moved smoothly.

dawsonc623 commented 1 year ago

Hello! Is there something to be done on this issue? I am looking to start contributing to Godot, and this is at the top of the "good first issue" list, but I am not sure I understand what actually needs doing.

ppelikan commented 1 year ago

Hello,

I've quickly prepared some info-graphic to illustrate this issue:

godot lighting issue

Now, this correct behavior is already achievable with a workaround illustrated in my previous post. Unfortunately, this workaround complicates everything, and makes impossible to do:

Below I'm providing a modified example project, where this workaround is applied. You can see, that the sprite movement is snapping to the pixels. Of course this is impossible to avoid with such method of rendering.

Is there another solution, or is this a Godot bug? I'm yet to be replied.

light_pixel_test2.zip

Calinou commented 1 year ago

Is there another solution, or is this a Godot bug? I'm yet to be replied.

To create pixelized shadows, you could use a light() function in a custom shader that modifies vertex position in the fragment shader to affect where light is sampled. This is essentially the 2D counterpart to https://github.com/godotengine/godot-proposals/issues/974.

However, this functionality isn't implemented in the engine yet.

AshutoshAkkole commented 9 months ago

have we come up with decision? weather we fixing this issue or depend on the work around?

AThousandShips commented 9 months ago

This issue is about documentation, see the details above on that this isn't a bug, and the "workaround" is simply how the engine works and that needs to be documented

ppelikan commented 9 months ago

I'm not convinced it's about documentation. It's labeled as documentation, but imho that's not accurate. It's a missing feature or a bug. Documenting some stuff doesn't make this problem go away.

Consider, what has been said in this discussion:

(...) Unfortunately, this workaround complicates everything, and makes impossible to do:

  • in-game zooming,
  • panning (smooth camera movement),
  • smooth movement of e.g. Sprites,
  • or avoiding any pixel snapping in general.

And most important, what Calinou said:

(...) However, this functionality isn't implemented in the engine yet.

Gurka2 commented 1 month ago

What is the status for this issue? If it is considered a bug/feature I would like to give it a try :blush:

SourceOfHTML commented 1 month ago

Hello, extreme newbie here!

Trying to reproduce the bug isn't working for me. I tried to do it with a LightOccluder as well, and despite being given an OccluderPolygon2D, Godot thinks it has none (unrelated to current issue), so I couldn't test for that.

Edit: I am an idiot, and I DID manage to test for it, and shadows from occluders still behave the same way the author experienced.

I attempted to coax some form of the bug by changing the viewport size and nothing happened.

Regardless, I can't reproduce the bug, so either it's been fixed, or I'm doing things wrong (not unlikely)

ppelikan commented 1 month ago

Please check the two attachments previously posted. They contain the Godot project files that reproduce this bug and the attempted workaround: light_pixel_test.zip light_pixel_test2.zip

I think this might be an advanced problem to solve. It would probably involve extensive understanding of shaders and rendering pipelines.

Uumutunal commented 3 weeks ago

I'm also a newbie so I'm sure there's a better solution to this, but this is how I fixed it.

From what I understand, it's not a bug, or a mistake, but rather that's how it's supposed to work.

in canvas.glsl

if (normal_used) {
    vec3 light_pos = vec3(light_array.data[light_base].position, light_array.data[light_base].height);
    vec3 pos = light_vertex;
    vec3 light_vec = normalize(light_pos - pos);

    light_color.rgb = light_normal_compute(light_vec, normal, base_color.rgb, light_color.rgb, specular_shininess, specular_shininess_used);
}

This calculates the angle which the light ray hits the surface, and calculates the light reflected using the normal map. This works when the camera is 100% zoom, because then one screen space pixel corresponds to one texture pixel. But when you zoom in now there are more than 1 screen pixel in 1 texture pixel, and they have different angles. This creates a gradient on that pixel.

The solution I found is to snap the screen space position (light_vertex) to the texture pixel.

if (normal_used) {
    vec3 light_pos = vec3(light_array.data[light_base].position, light_array.data[light_base].height);

    vec2 texture_size = vec2(textureSize(sampler2D(normal_texture, texture_sampler), 0));
    vec2 tex_uv_snap = (floor(tex_uv * texture_size.x) / texture_size.x) + (0.5/texture_size.x);

    vec4 tex_uv_h = vec4(tex_uv_snap, 0.0, 1.0);
    mat4 inverse_texture_matrix = inverse(mat4(
        light_array.data[light_base].texture_matrix[0], 
        light_array.data[light_base].texture_matrix[1], 
        vec4(0.0, 0.0, 1.0, 0.0), 
        vec4(0.0, 0.0, 0.0, 1.0)
    ));

    vec4 vertex_h = tex_uv_h * inverse_texture_matrix;
    vec2 vertex_snap = vertex_h.xy / vertex_h.w; 
    shadow_vertex = vertex_snap;
    vec3 pos = vec3(vertex_snap,0.0);

    vec3 light_vec = normalize(light_pos - pos);

    light_color.rgb = light_normal_compute(light_vec, normal, base_color.rgb, light_color.rgb, specular_shininess, specular_shininess_used);
}

https://github.com/user-attachments/assets/5a5f0742-8dcf-4b3b-bc20-0d5e12a1e963

I don't know how to add the option to switch between 2 modes though, and I don't know if this breaks some other stuff, but it seems to be working.

Zireael07 commented 3 weeks ago

how to add the option to switch between 2 modes though

Simplest way I found is to add a uniform boolean - gets exposed to the inspector automatically

demolen commented 1 week ago

I just ran into the same issue today. would be happy to see if this ends up in the engine as a feature. something like "pixel-perfect-lighting" for those who are making pixel art games. I really think godot should be feature proof regarding pixel art as it is the only presentation of the video games that has proved to be timeless and not the buzz of an era. :)

demolen commented 1 week ago

vec2 texture_size = vec2(textureSize(sampler2D(normal_texture, texture_sampler), 0)); vec2 tex_uv_snap = (floor(tex_uv * texture_size.x) / texture_size.x) + (0.5/texture_size.x);

vec4 tex_uv_h = vec4(tex_uv_snap, 0.0, 1.0); mat4 inverse_texture_matrix = inverse(mat4( light_array.data[light_base].texture_matrix[0], light_array.data[light_base].texture_matrix[1], vec4(0.0, 0.0, 1.0, 0.0), vec4(0.0, 0.0, 0.0, 1.0) ));

vec4 vertex_h = tex_uv_h * inverse_texture_matrix; vec2 vertex_snap = vertex_h.xy / vertex_h.w; shadow_vertex = vertex_snap; vec3 pos = vec3(vertex_snap,0.0);

vec3 light_vec = normalize(light_pos - pos);

light_color.rgb = light_normal_compute(light_vec, normal, base_color.rgb, light_color.rgb, specular_shininess, specular_shininess_used); }

This couldn't fix the problem on my side. The engine compiled just fine though. If you have made it so far to figure out how to make it an option/switchable you can make a PR, this would put this change into perspective and if needed or if there are other methods available, this would be discussed as a PR then.

Uumutunal commented 1 week ago

This couldn't fix the problem on my side. The engine compiled just fine though. If you have made it so far to figure out how to make it an option/switchable you can make a PR, this would put this change into perspective and if needed or if there are other methods available, this would be discussed as a PR then.

I'm working on adding the option to switch, it turned out to be more complicated than I imagined but I think I managed to do it.

https://github.com/user-attachments/assets/3948d8b5-20c9-4aa4-af7b-effce4b2fa4d

As for why it didn't work for you, you probably already know this but, if you changed the canvas.glsl after building the project it won't work because it actually uses the canvas.glsl.gen.h file which is generated from canvas.glsl. Either rebuild it or change canvas.glsl.gen.h directly to test it. If you already did this and it didn't work then it's interesting why it didn't work for you.

demolen commented 1 week ago

This couldn't fix the problem on my side. The engine compiled just fine though. If you have made it so far to figure out how to make it an option/switchable you can make a PR, this would put this change into perspective and if needed or if there are other methods available, this would be discussed as a PR then.

I'm working on adding the option to switch, it turned out to be more complicated than I imagined but I think I managed to do it.

2024-11-02_18-47-46.mp4

As for why it didn't work for you, you probably already know this but, if you changed the canvas.glsl after building the project it won't work because it actually uses the canvas.glsl.gen.h file which is generated from canvas.glsl. Either rebuild it or change canvas.glsl.gen.h directly to test it. If you already did this and it didn't work then it's interesting why it didn't work for you.

My bad, I have mistakenly editing the canvas.glsl in the drivers directory. I can confirm it is working as it should. Well done! Looking forward to see this merged as a feature. :)

demolen commented 1 week ago

This couldn't fix the problem on my side. The engine compiled just fine though. If you have made it so far to figure out how to make it an option/switchable you can make a PR, this would put this change into perspective and if needed or if there are other methods available, this would be discussed as a PR then.

I'm working on adding the option to switch, it turned out to be more complicated than I imagined but I think I managed to do it.

2024-11-02_18-47-46.mp4

As for why it didn't work for you, you probably already know this but, if you changed the canvas.glsl after building the project it won't work because it actually uses the canvas.glsl.gen.h file which is generated from canvas.glsl. Either rebuild it or change canvas.glsl.gen.h directly to test it. If you already did this and it didn't work then it's interesting why it didn't work for you.

regarding the light texture scale. is there something that can be done about it so that when the light texture is scaled we wouldn't get broken texture with texels smaller or bigger than the viewport resolution?

Screenshot 2024-11-02 at 17 40 18
Uumutunal commented 6 days ago

regarding the light texture scale. is there something that can be done about it so that when the light texture is scaled we wouldn't get broken texture with texels smaller or bigger than the viewport resolution?

As far as I'm aware that happens when you move the object in increments smaller than a size of a pixel, which is 1 if you didn't scale it, and 2 if you scaled it by 2 times for example, and so on. You have to treat the pixel size as the smallest unit you can move your objects so that the light and the texture always aligns.