Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve shader logging #1241

Merged
merged 1 commit into from
Oct 5, 2024
Merged

Conversation

VReaperV
Copy link
Contributor

@VReaperV VReaperV commented Aug 13, 2024

Parse info logs on shader compilation failure in an implementation-dependent way, which poduces an output like this:

 136: 	vec4 color;
 137: 	vec2 texCoord, lmCoord;a
 138: 
 139: 	VertexFetch( position, LB, color, texCoord, lmCoord );
------	-----------^-------------------------------------------------------
0(139) : error C0000: syntax error, unexpected '(', expecting "::" at token "("

 140: 	color = color * u_ColorModulate + u_Color;

If the exact character is not known, the line will just be like "^^^^^^^^^^".

Also reorganised some code a bit due to changes. Removed BuildGPUShaderText() since it was just unnecesserily fragmenting the code at this point.

@VReaperV
Copy link
Contributor Author

I have tested this on Nvidia and Intel (the latter sometimes reports incorrect line numbers though...), so just need someone to test with AMD.

@illwieckz
Copy link
Member

Mesa is probably all the same whatever the hardware (AMD, Nvidia, Intel, etc.), and Mesa is probably different different to Windows/macOS proprietary AMD even on AMD. With some luck AMD OGLP on Linux is same as AMD on Windows and macOS. On Windows and Linux, one may try Mesa llvmpipe as a generic Mesa driver.

@slipher
Copy link
Member

slipher commented Aug 17, 2024

Guess I'll wait for this to be rebased on #1242 :)

@VReaperV VReaperV added T-Improvement Improvement for an existing feature A-Renderer A-Logging labels Aug 22, 2024
@VReaperV VReaperV force-pushed the improve-shader-logs branch 2 times, most recently from 72520a0 to 11c1e04 Compare September 29, 2024 10:10
@VReaperV
Copy link
Contributor Author

Rebased. This now uses the functionality added in #1242.

Copy link
Member

@slipher slipher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat, works for me on Nvidia and Intel. Well, sort of works on Intel, as promised :)

_shaderManager( manager )
{}
{
size_t lineCount = std::count( text.begin(), text.end(), '\n' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is a text with 0 newlines a special case?

Copy link
Contributor Author

@VReaperV VReaperV Sep 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They either added a newline if empty or affected which line was reported by the driver, I'll add a comment for whichever it was.

Copy link
Contributor Author

@VReaperV VReaperV Oct 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is actually because each \n corresponds to 2 lines so any header with at least 1 needs to have its line count increased by 1.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The driver can't know whether the input had an empty string concatenated in it. Maybe it's just the "compat header" adding one by being empty, canceling out the -1 subtracted from all the line count sums.

Copy link
Contributor Author

@VReaperV VReaperV Oct 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I checked everything again, and it turns out that what was throwing me off were the shader compile macros. They're never passed to InitShader(), but they are added when the shader code is actually sent to the driver to compile. This is a problem on master as well w. r. t. #insert getting incorrect line numbers because of this, so I've removed the erroneus line calculation, but the bug resulting from macros is still there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've opened an issue for it: #1339. I consider that out of scope here though, since this pr is about logging.

src/engine/renderer/gl_shader.h Outdated Show resolved Hide resolved
src/engine/renderer/gl_shader.cpp Outdated Show resolved Hide resolved
src/engine/renderer/gl_shader.cpp Outdated Show resolved Hide resolved
src/engine/renderer/gl_shader.h Outdated Show resolved Hide resolved
@VReaperV VReaperV force-pushed the improve-shader-logs branch 5 times, most recently from 6f3d0e5 to bd7364d Compare October 2, 2024 21:08
src/engine/renderer/gl_shader.cpp Outdated Show resolved Hide resolved
_shaderManager( manager )
{}
{
size_t lineCount = std::count( text.begin(), text.end(), '\n' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The driver can't know whether the input had an empty string concatenated in it. Maybe it's just the "compat header" adding one by being empty, canceling out the -1 subtracted from all the line count sums.

Parse info logs on shader compilation failure in an implementation-dependent way. Inserts a line like "------^------" into the log pointing to the exact place of the error or "^^^^^^^" if only the line is known. Also reorganised some code a bit due to the changes.
@slipher
Copy link
Member

slipher commented Oct 5, 2024

LGTM

@VReaperV VReaperV merged commit 97cc353 into DaemonEngine:master Oct 5, 2024
9 checks passed
@VReaperV VReaperV deleted the improve-shader-logs branch October 5, 2024 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Logging A-Renderer T-Improvement Improvement for an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants