godotengine / godot

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

Fragment shader on Control material: SCREEN_UV uses editor viewport coordinate instead of canvas layer coordinate #92311

Open hsandt opened 6 months ago

hsandt commented 6 months ago

Tested versions

System information

Godot v4.2.1.stable - Ubuntu 22.04.4 LTS 22.04 - X11 - Vulkan (Mobile) - dedicated NVIDIA GeForce GTX 860M (nvidia; 535.161.07) - Intel(R) Core(TM) i7-4710HQ CPU @ 2.50GHz (8 Threads)

Issue description

This is a reboot of https://github.com/godotengine/godot/issues/33905 for Godot 4

When using SCREEN_UV in a fragment shader, and assigning a Shader Material using it on a Control on some Canvas Layer, the material preview will consider the editor viewport coordinates as UV instead of the Canvas Layer, which will be used in the game at runtime.

Using this shader:

shader_type canvas_item;

void fragment()
{
    COLOR = vec4(SCREEN_UV, 0.0, 1.0);
}

on a fullscreen ColorRect, I get different results depending on viewport view.

When the rect fills the viewport, I get a result similar to what I want ingame at runtime:

image

Runtime: image

When zooming out and panning the view, results differ:

image image image

I understand that there is no easy way to guess what SCREEN_UV will be in all cases:

However, when the shader material is used on a Control under a Canvas Layer, it may be feasible to estimate where it will be relative to the future game screen (need to mind Stretch Scale though). In this case, it may be worth previewing the shader material using Canvas Layer normalized coordinates as SCREEN_UV.

Steps to reproduce

  1. Open MWE, demo scene
  2. Pan the view and zoom in/out in the editor
  3. Observe how the ColorRect changes appearance

Minimal reproduction project (MRP)

v4.2.1 - Editor viewport affects behavior of SCREEN_UV.zip

powertomato commented 3 weeks ago

As a workaround you can pass the editor-viewport data from a tool-script to the shader and fix the UVs:

@tool
extends Node2D

var shader: ShaderMaterial

func _ready():
    shader = $ColorRect.material as ShaderMaterial
    # real size of the viewport
    shader.set_shader_parameter("real_pixel_size", Vector2(1./1920, 1./1080))
    if not Engine.is_editor_hint():
        shader.set_shader_parameter("editor_offset", Vector2(0,0))
        shader.set_shader_parameter("editor_zoom", 1)

func _process(delta: float) -> void:
    if Engine.is_editor_hint():
        var t = get_viewport().get_final_transform()
        shader.set_shader_parameter("editor_offset", -t.get_origin())
        shader.set_shader_parameter("editor_zoom", t.get_scale().x)
shader_type canvas_item;

uniform vec2 editor_offset;
uniform vec2 real_pixel_size;
uniform float editor_zoom;

void fragment() {
    vec2 zoom = 1.0/SCREEN_PIXEL_SIZE*real_pixel_size/editor_zoom;
    vec2 fixed_screen_uv = (SCREEN_UV*zoom +editor_offset*SCREEN_PIXEL_SIZE*zoom);
    ...
}

The solution to this bug is to perform this in the global shader code i.e. drivers\gles3\shaders\canvas.glsl and servers\rendering\renderer_rd\shaders\canvas.glsl

I could create a PR, but I would need help answer the following:

clayjohn commented 3 weeks ago

The solution to this bug is to perform this in the global shader code i.e. drivers\gles3\shaders\canvas.glsl and servers\rendering\renderer_rd\shaders\canvas.glsl

I could create a PR, but I would need help answer the following:

Are there any header-guards that I could use to only perform the fix in the editor? Where in code would it be a good idea to pass the uniforms required for the fix? Are there any more backends that need fixing (beside the two I mentioned)?

You are right that one way to "fix" this would be to modify the internal shader to have knowledge about whether we are in the editor and then modify the calculation of SCREEN_UV accordingly. However, doing so requires invasive changes to the renderer which go against our design principles.

Further, you can't fully solve the problem either, as you will find users who expect the SCREEN_UV to line up exactly with the size of the Viewport itself, but you will also find people who expect SCREEN_UV to line up with the bounds of the Camera2D (like the OP expects).

I'm not sure that there is a good solution here that will make everyone happy

kitt-schlatter commented 3 weeks ago

The solution to this bug is to perform this in the global shader code i.e. drivers\gles3\shaders\canvas.glsl and servers\rendering\renderer_rd\shaders\canvas.glsl I could create a PR, but I would need help answer the following: Are there any header-guards that I could use to only perform the fix in the editor? Where in code would it be a good idea to pass the uniforms required for the fix? Are there any more backends that need fixing (beside the two I mentioned)?

You are right that one way to "fix" this would be to modify the internal shader to have knowledge about whether we are in the editor and then modify the calculation of SCREEN_UV accordingly. However, doing so requires invasive changes to the renderer which go against our design principles.

Further, you can't fully solve the problem either, as you will find users who expect the SCREEN_UV to line up exactly with the size of the Viewport itself, but you will also find people who expect SCREEN_UV to line up with the bounds of the Camera2D (like the OP expects).

I'm not sure that there is a good solution here that will make everyone happy

Could this not be a configuration option in a menu somewhere to let users choose which way makes the most sense for their project?

powertomato commented 3 weeks ago

The solution to this bug is to perform this in the global shader code i.e. drivers\gles3\shaders\canvas.glsl and servers\rendering\renderer_rd\shaders\canvas.glsl I could create a PR, but I would need help answer the following: Are there any header-guards that I could use to only perform the fix in the editor? Where in code would it be a good idea to pass the uniforms required for the fix? Are there any more backends that need fixing (beside the two I mentioned)?

You are right that one way to "fix" this would be to modify the internal shader to have knowledge about whether we are in the editor and then modify the calculation of SCREEN_UV accordingly. However, doing so requires invasive changes to the renderer which go against our design principles.

Further, you can't fully solve the problem either, as you will find users who expect the SCREEN_UV to line up exactly with the size of the Viewport itself, but you will also find people who expect SCREEN_UV to line up with the bounds of the Camera2D (like the OP expects).

I'm not sure that there is a good solution here that will make everyone happy

The way I see this there is a bug, namely that the SCREEN_UV has different behavior in the editor and in the final render. From a user perspective the former is a WYGIWYS model of the latter, which can be considered the absolute truth.

Aligning both requires some change in the rendering process. Whether that needs to be as deep as my proposed solution I cannot say. The current situation requires it to be as deep as it possibly can: in the user shader. I think the user shader should not consider whether it is being executed in the context of the editor or not.

Independently from that bug, there might be a separate issue on what SCREEN_UV does, as you say whether SCREEN_UV maps to the Camera2D or viewport. From my point of view, both are fine, they both can be easily mapped to each other by using the Camera2Ds transformation matrix or its inverse, respectively.

I would fix the bug to align the behavior of SCREEN_UV in the editor to accurately represent the final render as it is now.

clayjohn commented 2 weeks ago

The way I see this there is a bug, namely that the SCREEN_UV has different behavior in the editor and in the final render. From a user perspective the former is a WYGIWYS model of the latter, which can be considered the absolute truth.

I fully agree with this. This is definitely a bug and it appears to impact at least a few users.

In my comment above I'm trying to take the conversation one step further and explore the solution space. The options are not good. Please don't take my comment as "this is not a bug so it doesn't need to be fixed". Its more like "this is clearly a bug, but the solution is possible more harmful than the original bug, so we need to be careful".

Bluntly, inserting editor-only code into the core renderer is not going to happen. It goes against our design principles and it makes Godot harder to maintain and improve overall. That doesn't mean that this bug is not solvable, but it does mean that a solution is going to be much harder to find.