Compute/texture demo is broken at current master version of Godot

jsjtxietian commented 7 months ago

Tested versions

Godot v4.3.dev (29b3d9e9e)

System information

Windows 10.0.19045 - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 3060 (NVIDIA; 31.0.15.3619) - 11th Gen Intel(R) Core(TM) i7-11700K @ 3.60GHz (16 Threads)

Issue description

I open a issue at demo repo https://github.com/godotengine/godot-demo-projects/issues/1031 but it seems it's an engine issue and should be reported here.

When I opened the project https://github.com/godotengine/godot-demo-projects/tree/master/compute/texture, open the water scene and switch back to main scene, the log spams as this image:

scene\resources\texture_rd.cpp:90 - Condition "!RD::get_singleton()->texture_is_valid(p_texture_rd_rid)" is true.
servers\rendering\rendering_device.cpp:3991 - Parameter "pipeline" is null.
servers\rendering\rendering_device.cpp:4079 - Parameter "uniform_set" is null.
servers\rendering\rendering_device.cpp:4079 - Parameter "uniform_set" is null.
servers\rendering\rendering_device.cpp:4079 - Parameter "uniform_set" is null.
This compute pipeline requires (0) bytes of push constant data, supplied: (32)
No compute pipeline was set before attempting to draw.

Trying to run the project will crash at start:

Steps to reproduce

See above

Minimal reproduction project (MRP)

https://github.com/godotengine/godot-demo-projects/tree/master/compute/texture

BalmundSM commented 7 months ago

Hello! I'm new to contributing. Is this issue manageable for newbies? If yes I would like to be assigned this issue

Calinou commented 7 months ago

Hello! I'm new to contributing. Is this issue manageable for newbies? If yes I would like to be assigned this issue

For future reference, we only use the assignees feature for core contributors (and we don't always use it in the first place). Since contributors can leave at any time for any reason, we'd rather avoid a situation where nobody is working on an issue because it's assigned to someone who is no longer active.

Also, this issue is likely difficult to tackle as it involves low-level graphics programming APIs like Vulkan. For a first contribution, I suggest looking at issues labeled good first issue instead.

Zatarita commented 5 months ago

I too followed the same tutorial and found it to have a few issues. One thing was the push_constants didn't work properly. When I defined my layout for push_constants in the shader as only having one float, it stated that I needed 16 bytes of data. Instead of using push constants I had to create my own uniform set and bind that instead. then it was happy with the 4 bytes of data that was sent.

I also seem to have a similar issue with the error after fixing all of that when I reload the scene:

"set_texture_rd_rid: Condition "!RD::get_singleton()->texture_is_valid(p_texture_rd_rid)" is true."

My scene loads just fine and everything runs as it's supposed to, but I can't seem to narrow down what is causing the issue. When I place a break point in the scene no matter where I place it the error happens immediately once something is drawn to screen. I even placed a break point before any of the compute shader stuff tried to compile.

The "RD::get_singleton()->texture_is_valid(p_texture_rd_rid)" error seems to be a part of the Texture2DRD, and judging by the fact that the error occurs before anything is done I have a sneaking suspicion that it has something to do with the construction of the Texture2DRD. I never use the Texture2DRD before it has been assigned an RID, and everything works perfectly. It just complains that the texture is invalid.

Just a hunch, but maybe it has something to do with the fact that when a Texture2DRD is initialized it hasn't been assigned a RID to reference. This I'm able to see in the stack trace. Before it has been assigned a RID it has an "Invalid RID". This may be avoidable by giving the Texture2DRD a constructor that allows for the RID to be assigned at construction instead of after it's been constructed with an invalid RID, and/or only doing validation on the Texture2DRD after it's attempting to be referenced.

Edit: So I tried editing the Texture2DRD constructor to give it a default RID; however, even with an explicate constructor setting the RID to 0, AND checking if the RID was 0 in the set RID function to ignore it. (just to see if that was the issue) I still got the error. It appears like something always calls the set_texture_rd_rid function and assigns the exact RID: "1521084142722790" at the start of the program. I was hoping to be able to attach a debugger in VS and work backwards, but vs doesn't seem to let me debug the actual code, and I got tired of waiting 20 mins for it to compile every time. For my uses I'm just gonna say it's safe to ignore this error, it's just annoying but it doesn't effect my project at all. I see the proper RID assigned right afterwards.

EDIT 2: Seeing a red dot in my debug console constantly cannot be ignored. I managed to finally build godot with debug symbols (Commenting again about how scons doesn't actually default yes for debug_symbols) and I can infact confirm that my presumption is what is happening; however, it's happening because of a default property setter on boot not because of a constructor. I was able to actually prevent the error from happening by setting a default value for the set_texture_rd_rid method:

line 37: ClassDB::bind_method(D_METHOD("set_texture_rd_rid", "texture_rd_rid"), &Texture2DRD::set_texture_rd_rid, DEFVAL(RID::from_uint64(0)));

And checking if the value was null in the set_texture_rd_rid function:

line 74: ERR_FAIL_NULL(RS::get_singleton());
line 75: if (p_texture_rd_rid == RID::from_uint64(0))
    return;

HOWEVER, this isn't a full solution. I personally feel the Texture2DRD should be null by default, OR force it to be instantiated with a valid RID (This doesn't really follow Godot's established flow though.)

So, IMO the solution to the "set_texture_rd_rid: Condition "!RD::get_singleton()->texture_is_valid(p_texture_rd_rid)" is true." bug is to initialize the Texture2DRD as null, and check for null in the setter (as a null RID isn't invalid, it just means it's unassigned) and instead the check should be done where the Texture2DRD is being utilized. This should only be an error when a null reference is attempted to be used, NOT when the null reference is assigned (funny enough though, I can't seem to reproduce this error by just creating an empty Texture2DRD, so I'm not entirely sure what's creating the invalid Texture2DRD)

Edit 3: Cleaned things up a bit because this ended up being a long post Should I make this it's own bug post as it's only tangentially related to this OP?

godotengine / godot