godotengine / godot

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

SubViewportContainer does not scale Viewport properly with CanvasItems project scale mode. #77149

Open EMBYRDEV opened 1 year ago

EMBYRDEV commented 1 year ago

Godot version

4.0.2.stable

System information

Windows 11, Vulkan, RTX2070 Super

Issue description

SubViewportContainer does not scale the underlying viewport appropriately to maintain quality in this mode.

Potentially related to #70791 and #76450.

image

Steps to reproduce

See reproduction project.

image

Minimal reproduction project

ViewportScalingRepro.zip

EMBYRDEV commented 1 year ago

Upon some investigation it appears that scaling that it can handle one axis of scaling but not both. Either vertical or horizonal scaling works properly but not combined.

Calinou commented 1 year ago

This doesn't happen automatically by design, as changing viewport size behind the user's back may break expectations (especially when using viewport for off-screen rendering, such as procedural textures).

You need to resize the SubViewports by listening to the size_changed signal on the root Viewport, as done in the 3D in 2D demo. See https://github.com/godotengine/godot-demo-projects/pull/891.

EMBYRDEV commented 1 year ago

This doesn't happen automatically by design, as changing viewport size behind the user's back may break expectations (especially when using viewport for off-screen rendering, such as procedural textures).

You need to resize the SubViewports by listening to the size_changed signal on the root Viewport, as done in the 3D in 2D demo. See godotengine/godot-demo-projects#891.

That commit removes any reference to SubViewportContainer from the example.

Is this not the point of SubViewportContainer's stretch option? To keep the viewport at a 1:1 pixel ratio on screen.

I think one of us is missing something here. If you look at the way the size of the container actually changes in the example, it isnt as simple as just multiplying it by the scaling factor. The containers laying it out etc for splitscreen actually keep one of the dimensions in a weird way.

Calinou commented 1 year ago

Is this not the point of SubViewportContainer's stretch option? To keep the viewport at a 1:1 pixel ratio on screen.

The Stretch option makes the SubViewportContainer stretch to cover the container's size. It does not resize the underlying Viewport.

EMBYRDEV commented 1 year ago

Is this not the point of SubViewportContainer's stretch option? To keep the viewport at a 1:1 pixel ratio on screen.

The Stretch option makes the SubViewportContainer stretch to cover the container's size. It does not resize the underlying Viewport.

image That is not what it states.

https://github.com/godotengine/godot/assets/20043270/048f0e2e-7152-4ca7-8671-78fe5913836f Also see that the container does not report it's size in a way that is able to calculate how much I should manually scale the viewport by.

EMBYRDEV commented 1 year ago

It does what it says on the tin to be fair, the problem is that when canvas_items scale mode is set, the control's size seems to have absolutely no obvious relation to screen size any more so maintaining the correct pixel size on the display doesn't have an immediately obvious solution.

I think canvas_items scale mode is a little bit of a rabbit hole that links back to these #54030 #76450. Another example that could cause issues is if you design your games HUD at 720p and use the scaling mode but then want to do split screen. You'd want the canvas items in each viewport to scale down accordingly.

Perhaps the scaling mode should be set per-viewport instead of per-project?

Calinou commented 1 year ago

Perhaps the scaling mode should be set per-viewport instead of per-project?

There was some talk about moving the content scale options from Window to Viewport, but this likely breaks compatibility.

rcorre commented 1 year ago

That commit removes any reference to SubViewportContainer from the example.

I saw that demo as answering the common question "How do I scale the 3D resolution for performance", which no longer requires a SubViewport in the forward renderer, as you can set the 3D scaling directly on the main viewport.

I split out a separate compatibility demo to show the "legacy" way of scaling with a SubViewport (which should also apply to e.g. multiple SubViewports for split-screen), where I think I hit this issue: https://github.com/godotengine/godot-demo-projects/pull/891#issuecomment-1489429468

OhiraKyou commented 1 year ago

Just ran into this while trying to add 3D collectible counter icons to a UI. I use a base resolution of 1024x768. When I maximize the window on my 2560x1440 monitor, the icons look extra blocky, with 2-pixel slope steps.

Ideally, the SubViewportContainer would just have a setting to enforce pixel-perfect rendering.

OhiraKyou commented 12 months ago

For anyone seeking a solution, my current workaround is to replace the SubViewportContainer with a TextureRect whose texture is set to New ViewportTexture and assigned the associated SubViewport. Then, in script, I reproduce the size scale calculation and apply the result to the SubViewport as a multiplier for its initial size.

Setup

For TextureRect scaling, I use a parent MarginContainer with a custom_minimum_size equal to the SubViewport's initial size. The TextureRect 's container sizing (both horizontal and vertical) is set to "Fill". To prevent unintended additional TextureRect scaling, I set its expand mode to "Ignore Size" and stretch mode to "Keep Aspect Centered".

Also, I set the TextureRect's texture_filter to "Near" and texture_repeat to "Disabled". The filter setting prevents camera background bleeding, which is particularly important when using a transparent background.

Script

class_name ViewportSizer
extends Node

@export var target_viewport: SubViewport

var _base_render_size: Vector2 = Vector2()
var _window: Window

func _ready():
    _window = get_tree().root as Window
    _window.size_changed.connect(update_size)
    _base_render_size = target_viewport.size

## Calculates a scale for width and height, and applies the lower of the two.
##
## This assumes the following display settings:
## - display/window/stretch/mode: canvas_items
## - display/window/stretch/aspect: expand
func update_size():
    # Cast from Vector2i (integer) to Vector2 (float) for smooth results
    var current_viewport_size: Vector2 = _window.size
    var reference_viewport_size: Vector2 = _window.content_scale_size

    var viewport_scale: Vector2 = current_viewport_size / reference_viewport_size
    var size_scale: float = minf(viewport_scale.x, viewport_scale.y)

    # Round back to integer size to prevent truncation
    var scaled_size: Vector2i = (_base_render_size * size_scale).round()
    target_viewport.size = scaled_size
newobj commented 10 months ago

@OhiraKyou this solution does not work for me. any steps you may have forgot to detail?

also, does this solution break object picking for yo?

OhiraKyou commented 10 months ago

@newobj

any steps you may have forgot to detail?

does this solution break object picking for yo?

Assuming you're referring to selecting objects by clicking on them from the viewport, I can still click on the TextureRect node. Any objects being rendered to or behind the texture can be selected in the hierarchy instead.

BenLubar commented 4 months ago

Here's my version of @OhiraKyou's script, this time as a replacement subclass for SubViewportContainer

# https://github.com/godotengine/godot/issues/77149
class_name SubViewportContainerBug77149
extends SubViewportContainer

@export var stretch_without_bug_77149 : bool

func _ready() -> void:
    resized.connect(_on_resized)
    _on_resized()

var _dont_recurse : bool

func _on_resized() -> void:
    assert(not stretch)
    if not stretch_without_bug_77149 or _dont_recurse:
        return

    var window := get_window()
    var viewport_size := Vector2(window.size)
    var reference_size := Vector2(window.content_scale_size)
    var viewport_scale := viewport_size / reference_size
    var size_scale := minf(viewport_scale.x, viewport_scale.y)

    var scaled_size := Vector2i((size * scale * size_scale).round()) / stretch_shrink
    if Vector2i(size.round()) == scaled_size:
        return

    _dont_recurse = true
    # we need to set the container's size first or it'll get resized by the viewport resize (ugh)
    scale = Vector2(1.0 / size_scale, 1.0 / size_scale)
    size = scaled_size

    for subviewport : SubViewport in find_children("*", "SubViewport", false, false):
        # avoid reallocating textures we don't need to reallocate
        if subviewport.size != scaled_size:
            subviewport.size = scaled_size

    _dont_recurse = false
KaruroChori commented 2 months ago

Sadly this problem is poisoning the entire hierarchy stack. For example, opening new windows or confirmation dialogs will exhibit the same issue, as they are all derived from ViewportContainer. image As you can see in the example, text drawn as part for the root window and the decoration of the dialog are fine, but the inside is not.

Aside from patching the engine directly, is there any feasible way to override this problem?