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.

Sign up for GitHub

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

Display useful pixel shader compilation errors #17436

Merged
merged 5 commits into from
Jun 21, 2024
Merged

Display useful pixel shader compilation errors #17436

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a75edea
Descriptive custom pixel shader compilation errors
blitzRahul Jun 15, 2024
542467e
proper formatting for TermControl.cpp
blitzRahul Jun 15, 2024
a76e370
ensure unicode text compatibility
blitzRahul Jun 15, 2024
3f4d582
add conditional warning message construction
blitzRahul Jun 21, 2024
7dbaa3b
improve efficiency of message construction
blitzRahul Jun 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions src/cascadia/TerminalControl/Resources/en-US/Resources.resw
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,8 @@ Please either install the missing font or choose another one.</value>
<comment>{0} is a file name</comment>
</data>
<data name="PixelShaderCompileFailed" xml:space="preserve">
<value>Unable to compile the specified pixel shader.</value>
<value>Pixel shader failed to compile: {0}</value>
<comment>{0} is the error message generated by the compiler</comment>
</data>
<data name="UnexpectedRendererError" xml:space="preserve">
<value>Renderer encountered an unexpected error: {0}</value>
Expand All @@ -222,8 +223,8 @@ Please either install the missing font or choose another one.</value>
<comment>{Locked="2023.5.31","MacType"}</comment>
</data>
<data name="RendererErrorOther" xml:space="preserve">
<value>Renderer encountered an unexpected error: {0:#010x} {1}</value>
<comment>{Locked="{0:#010x}","{1}"} {0:#010x} is a placeholder for a Windows error code (e.g. 0x88985002). {1} is the corresponding message.</comment>
<value>Renderer encountered an unexpected error: {0:#010x} {1} "{2}"</value>
<comment>{Locked="{0:#010x}","{1}"} {0:#010x} is a placeholder for a Windows error code (e.g. 0x88985002). {1} is the corresponding message. {2} is the filename.</comment>
</data>
Copy link
Member

Choose a reason for hiding this comment

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

Is there a chance that parameter will be empty for non-shader-related Renderer errors? If so, should we conditionally add the filename or parameter to the message, when we construct it?

Copy link
Contributor Author

@blitzRahul blitzRahul Jun 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hello Dustin! I stared at the code for a while, I think it is safe to conclude that parameter won't be empty for non-shader-related Renderer errors.

BackendD3D.cpp Line 453 to 470
image
In the event that the compiler returns an error (faulty shader code), parameter = compiler related error message, else parameter = file path. Unless D3DCompileFromFile function, line 409 BackendD3D.cpp returns an invalid hr, we should not have problems.

TermControl.cpp _RendererWarning function
image

I am new to this project, infact this is my first contribution; I have a lot of code analysis to do before I can make claims/conclusions with resonable confidene. So, let me know if I'm missing something here : )

Edit: I had a quick question. What about other languages? It would be nice to have native language speakers edit the respective Resource.resw files; reason being that online translators aren't exactly accurate.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I missed your edit! Don't worry too much about other languages - the automatic translation service should come through and automatically update the messages.

Your analysis makes sense of when and where the filename/parameters come from, as well. I appreciate it!

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I think I found what I was worried about!

There is one more call to p.warningCallback in AtlasEngine.r.cpp:

    if (_p.warningCallback)
    {
        try
        {
            _p.warningCallback(hr, {});
        }
        CATCH_LOG()
    }

If the render engine fails for a non-shader-related reason, like DirectWrite ran out of memory or the graphics hardware did not respond in time, I think we will display an error message with empty quotes in it:

/!\ Error                                                                 [X]

The renderer encountered an unexpected error: 0x80070057 Invalid Parameter ""

                                                                       [ OK ]

Copy link
Contributor Author

@blitzRahul blitzRahul Jun 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I see the issue now! I made a commit to fix it, is the implementation fine?
image

<data name="TermControlReadOnly" xml:space="preserve">
<value>Read-only mode is enabled.</value>
Expand Down Expand Up @@ -320,4 +321,4 @@ Please either install the missing font or choose another one.</value>
<value>Suggested input: {0}</value>
<comment>{Locked="{0}"} {0} will be replaced with a string of input that is suggested for the user to input</comment>
</data>
</root>
</root>
4 changes: 2 additions & 2 deletions src/cascadia/TerminalControl/TermControl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1162,7 +1162,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
message = winrt::hstring{ fmt::format(std::wstring_view{ RS_(L"PixelShaderNotFound") }, parameter) };
break;
case D2DERR_SHADER_COMPILE_FAILED:
message = winrt::hstring{ fmt::format(std::wstring_view{ RS_(L"PixelShaderCompileFailed") }) };
message = winrt::hstring{ fmt::format(std::wstring_view{ RS_(L"PixelShaderCompileFailed") }, parameter) };
break;
case DWRITE_E_NOFONT:
message = winrt::hstring{ fmt::format(std::wstring_view{ RS_(L"RendererErrorFontNotFound") }, parameter) };
Expand All @@ -1175,7 +1175,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
wchar_t buf[512];
const auto len = FormatMessageW(FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS, nullptr, hr, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), &buf[0], ARRAYSIZE(buf), nullptr);
const std::wstring_view msg{ &buf[0], len };
message = winrt::hstring{ fmt::format(std::wstring_view{ RS_(L"RendererErrorOther") }, hr, msg) };
message = winrt::hstring{ fmt::format(std::wstring_view{ RS_(L"RendererErrorOther") }, hr, msg, parameter) };
break;
}
}
Expand Down
19 changes: 13 additions & 6 deletions src/renderer/atlas/BackendD3D.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "dwrite.h"
#include "wic.h"
#include "../../types/inc/ColorFix.hpp"
#include "../../types/inc/convert.hpp"

#if ATLAS_DEBUG_SHOW_DIRTY || ATLAS_DEBUG_COLORIZE_GLYPH_ATLAS
#include "colorbrewer.h"
Expand Down Expand Up @@ -451,15 +452,21 @@ void BackendD3D::_recreateCustomShader(const RenderingPayload& p)
{
if (error)
{
LOG_HR_MSG(hr, "%.*hs", static_cast<int>(error->GetBufferSize()), static_cast<char*>(error->GetBufferPointer()));
if (p.warningCallback)
{
//to handle compile time errors
const std::string_view errMsgStrView{ static_cast<const char*>(error->GetBufferPointer()), error->GetBufferSize() };
const auto errMsgWstring = ConvertToW(CP_ACP, errMsgStrView);
p.warningCallback(D2DERR_SHADER_COMPILE_FAILED, errMsgWstring);
}
}
else
{
LOG_HR(hr);
}
if (p.warningCallback)
{
p.warningCallback(D2DERR_SHADER_COMPILE_FAILED, p.s->misc->customPixelShaderPath);
if (p.warningCallback)
{
//to handle errors such as file not found, path not found, access denied
p.warningCallback(hr, p.s->misc->customPixelShaderPath);
}
}
}

Expand Down
Loading