gongpha / gdQmapbsp

A simple plugin that loads Quake's MAP/BSP files into Godot 4 interactively
MIT License
42 stars 5 forks source link

Non animated textures/materials are black #3

Closed victorbstan closed 1 year ago

victorbstan commented 1 year ago

Can load map now, but all textures that are not animated are completely black.

Screenshot 2023-06-16 at 7 16 21 AM Screenshot 2023-06-16 at 7 17 42 AM

Errors in log after running game:

Screenshot 2023-06-16 at 7 30 26 AM

Attached log file.

godot.log

PS. Thanks for fixing the import issue.

gongpha commented 1 year ago

Looks like it's an engine rendering or platform-specific issue (MacOS).

Can you re-run the project with the project setting "Verbose Stdout" (debug/settings/stdout/verbose_stdout) enabled and resend a log here again please ?

I might figure out what was going on.

Or you can try finding related Godot issues here. https://github.com/godotengine/godot/issues?q=label%3Aplatform%3Amacos+label%3Atopic%3Arendering

victorbstan commented 1 year ago

Good idea about verbose logging, I'm not yet very familiar with GD 4... I've attached the new log, seems like a shader issue, but could be an engine issue as well, I'm not sure. Thanks for looking into it... godot.log

gongpha commented 1 year ago

I'm not sure if there's a limit on macOS's vulkan that caused the errors.

Try running this minimal project and check if this will get the same issue. i_47725dba2_001.zip

(This project just shows 256 noise textures at the center of the screen. Also, ignore the icon file error)

victorbstan commented 1 year ago

Hmm getting some errors logged but not sure if same issue? godot.log

Screenshot 2023-06-16 at 7 33 45 PM
victorbstan commented 1 year ago

I guess I would wonder, what is it about the shaders you've shared that's different than the default meterials/shader that seems to work?

Screenshot 2023-06-16 at 7 56 09 PM

Hmm, and if I switch the Godot renderer to Compatibility mode, it crashes the editor when trying to startup... hmm I thought Godot was for making cross platform games... :/ wonder what the funky parts of the custom shaders are that Vulkan to Metal translator can't deal with...

victorbstan commented 1 year ago

I don't have a PC handy, but I'm curious if on your end, if you switch to "Mobile" or "Compatibility" rendering mode in the editor, does it still render properly?

victorbstan commented 1 year ago

uniform sampler2D[256] might be a problem...

A non indexed sampler2D works (I can see noise texture):

uniform sampler2D texs : source_color;

void fragment() {
    ALBEDO = texture(texs, UV).xyz;
//  [int(TIME * 4.0f) % 256]
}

Edit 2:

either sampler2D texs[256] or sampler2D[256] texs seem to indicate that texs is an array of texture values (which are unassigned). And the % 256 part might want to be % 255?

victorbstan commented 1 year ago

This is probably not what you wanted to do, but this fades a noise texture to white...

shader_type spatial;
render_mode unshaded;

uniform sampler2D texs : source_color;

void fragment() {
    ALBEDO = texture(texs, UV).xyz * TIME * 4.0f;
}

Edit (disregard this comment):

So I suspect there might be something hardware specific/non-standard in some of the shader code, which is why it's not rendering properly (to go back to the original post) -- maybe there's a way to rewrite it to keep in mind some lower-common-denominator / wider compatibility shader features?

Edit 2:

I'm less inclined to think that texs[texidx] is the problem... still investigating...

victorbstan commented 1 year ago

So I went and commented out the texs[256] and changed them to just texts and this lol:

Screenshot 2023-06-16 at 10 02 19 PM

Edit:

OK, gotta see how we can use an array of textures...

victorbstan commented 1 year ago

Hmm, could this hint at the issue: https://stackoverflow.com/questions/45167538/error-sampler-arrays-indexed-with-non-constant-expressions-are-forbidden-in-gl

The problem is that in line texColor = texture(textures[tid], fs_in.uv) the sampler array texturues is given a non constant index.
gongpha commented 1 year ago

Hmm getting some errors logged but not sure if same issue? godot.log

Likely.

I guess I would wonder, what is it about the shaders you've shared that's different than the default meterials/shader that seems to work?

My shader has used a uniform array. This is what I have to test.

Hmm, and if I switch the Godot renderer to Compatibility mode, it crashes the editor when trying to startup... hmm I thought Godot was for making cross platform games... :/ wonder what the funky parts of the custom shaders are that Vulkan to Metal translator can't deal with...

I got crashed either with Compatibility mode. (When opening the project in the editor). This seems an OpenGL issue.

I don't have a PC handy, but I'm curious if on your end, if you switch to "Mobile" or "Compatibility" rendering mode in the editor, does it still render properly?

Yes, in Mobile. But not for Compatibility. T^T

uniform sampler2D[256] might be a problem...

A non indexed sampler2D works (I can see noise texture):

Yes, but you might see only one single noise texture.

This is what it will show properly if nothing occurs.

https://github.com/gongpha/gdQmapbsp/assets/13400398/a6a20600-bb18-49db-aab8-1cb62b5b6bed

Edit 2:

either sampler2D texs[256] or sampler2D[256] texs seem to indicate that texs is an array of texture values (which are unassigned). And the % 256 part might want to be % 255?

% 256 is already correct. Since the size of the array is 256. The index value should wrap around 0 to 255.

This is probably not what you wanted to do, but this fades a noise texture to white...

Yeah, The shader code you've modified is trying to multiply the time value to color fragments instead of trying to access the different noise textures by the time value

Edit (disregard this comment):

So I suspect there might be something hardware specific/non-standard in some of the shader code, which is why it's not rendering properly (to go back to the original post) -- maybe there's a way to rewrite it to keep in mind some lower-common-denominator / wider compatibility shader features?

Yes, but before I will do that. I must figure out what the problem really is.

Edit 2:

I'm less inclined to think that texs[texidx] is the problem... still investigating...

I think about that either. The uniform arrays are like there's a kind of hardware-specific work inside them . . .

gongpha commented 1 year ago

Can you try changing all the 256 numbers in both script code and shader code to anything lower (e.g. 64, 32) then re-run the project again ?

It seems like there are 2 possible problems. The limitation of textures (256) or accessing index by using non-constant value.

SO, The workaround is we've to change from a single material (of the whole level) to texture-dependent material.

Anyways, don't forget to report this issue or find the duplicate one on Godot (https://github.com/godotengine/godot/issues/new/choose). This issue shouldn't be occurred and can be considered as a bug. And you can also use my above minimal project as an MRP.

victorbstan commented 1 year ago

Not sure if this is good news or bad news. but changing the limit to 64 works!

Screenshot 2023-06-17 at 9 04 21 PM

However, the lightmap seems off... (I'll make a new issue for that, just in case you are bored :P )

I will also make a bug report with Godot as you suggested.

Again, thanks for debugging this with me. I'm really hopeful that I can use this project to make quake 1 based mods/custom games with.

victorbstan commented 1 year ago

SO, The workaround is we've to change from a single material (of the whole level) to texture-dependent material.

This would seem more future proof, and less hacky TBH...

victorbstan commented 1 year ago

96 Seems to be the upper limit in the sampler 2D index.

victorbstan commented 1 year ago

Filed a bug with Godot here: https://github.com/godotengine/godot/issues/78405

gongpha commented 1 year ago

Not sure if this is good news or bad news. but changing the limit to 64 works! However, the lightmap seems off... (I'll make a new issue for that, just in case you are bored :P )

Maybe the unmatched array size made the shader goes buggy.

As a workaround, Try to add this code in res://addons/qmapbsp/importer/bsp_parser.gd, Line 663

global_textures.resize(64) # or any value you've set in the shader code

before global_surface_mat.set_shader_parameter(&'texs', global_textures) line.

victorbstan commented 1 year ago

That does fix it global_textures.resize(GLOBAL_TEXTURE_LIMIT); but also, setting the limit to 96, I haven't seen any issues in any of the quake 1 maps anymore

victorbstan commented 1 year ago

Will close as initial issue was clarified -- not a bug with code, but platform specific, and work-around found Thanks! :)