godotengine / godot

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

INV_VIEW_MATRIX is incorrect with double precision #85711

Open fire opened 11 months ago

fire commented 11 months ago

Godot version

4.2 with double precision

System information

Windows 11, Nvidia 3090

Issue description

This issue is about a bug found in Godot where INV_VIEW_MATRIX does not work as expected when using double precision.

4.2 double build

editor_screenshot_2023-12-03T083902

4.2 steam build

editor_screenshot_2023-12-03T084001

Steps to reproduce

The issue can be reproduced by using the following shader code in a project with double precision enabled attached to a mesh:

shader_type spatial;

void vertex() {
    mat4 m = INV_VIEW_MATRIX * VIEW_MATRIX;
    COLOR = abs(m[3]); // This is meant to be black
}

void fragment() {
    ALBEDO = COLOR.rgb;
}

Minimal reproduction project

INV_VIEW_MATRIX bug Game Project.zip

fire commented 11 months ago

@celyk You mentioned some workaround and something about exposing two vectors for the translation.

AThousandShips commented 11 months ago

Duplicate of?:

fire commented 11 months ago

Maybe.

We worked around with

// Fix for double precision
#define INV_VIEW_MATRIX inverse(VIEW_MATRIX)
fire commented 11 months ago

@AThousandShips Well in this shader it's in the vertex stage.

AThousandShips commented 11 months ago

Don't think the specific stage matters necessarily here, should be same underlying issue

BastiaanOlij commented 11 months ago

cc @clayjohn any ideas?

@AThousandShips running inverse in the shader is relatively expensive, it's better to use the matrix in the UBO. I'm guessing the fix for double precision wasn't applied to this matrix

celyk commented 11 months ago

From what I've gathered, none of these matrices are meant to be used: MODEL_MATRIX, VIEW_MATRIX, MODELVIEW_MATRIX, and INV_VIEW_MATRIX

Because they represent transformations between model and view space, and the approach to split up the translation means that they can't be used properly without care. In this article they warn against using them.

The #define worked for my shader, but doesn't really solve the problem. Please correct me if I'm wrong. Either way, this breaks a lot of my shaders. I am happy to put in extra work to support double precision, but I want compatibility to be maintained for normal shaders.

Chaosus commented 11 months ago

Probably the bug is here: https://github.com/godotengine/godot/blob/d76c1d0e516fedc535a2e394ab780cac79203477/servers/rendering/renderer_rd/storage_rd/render_scene_data_rd.cpp#L62-L66

clayjohn commented 11 months ago

From what I've gathered, none of these matrices are meant to be used: MODEL_MATRIX, VIEW_MATRIX, MODELVIEW_MATRIX, and INV_VIEW_MATRIX

That's right. The tradeoff mentioned in the article and original PR is that we can't provide high precision versions of every intermediate matrix. So those matrices maintain their single precision nature. The enhanced precision code path is only for the default vertex codepath. Unfortunately, there is not a lot we can do here other than clarify in documentation as double precision is not well supported on GPUs yet.

nbaum commented 7 months ago

It is necessary for the matrices to be wrong even when the actual values are in the range of what is supported with single-precision floats?

I understand that CAMERA_POSITION_WORLD cannot be precise when the camera is very far from the origin, but it is wrong even when the camera is only a few hundred units away from the origin.

Edit: Contrary to what the closed issue reported, it doesn't appear that the camera position is simply inverted.

If it is not feasible for INV_VIEW_MATRIX to contain (nearly) correct data, perhaps CAMERA_POSITION_WORLD could be sent to the GPU separately?

I could send it from a script, but I don't know how to make that work in the editor view, or if there are multiple cameras rendering.