From a75edea74389880ae26fab3475963dab0379fc66 Mon Sep 17 00:00:00 2001 From: blitzRahul <104504393+blitzRahul@users.noreply.github.com> Date: Sat, 15 Jun 2024 10:05:54 +0530 Subject: [PATCH 1/5] Descriptive custom pixel shader compilation errors More descriptive warnings are triggered when custom pixel shader compilation fails. Tests performed: 1. Compile time error 2. File not found 3. Path not found 4. Access denied --- .../Resources/en-US/Resources.resw | 4 ++-- src/cascadia/TerminalControl/TermControl.cpp | 4 ++-- src/renderer/atlas/BackendD3D.cpp | 16 ++++++++++++---- 3 files changed, 16 insertions(+), 8 deletions(-) diff --git a/src/cascadia/TerminalControl/Resources/en-US/Resources.resw b/src/cascadia/TerminalControl/Resources/en-US/Resources.resw index 54c6a4643b5..9dc614c1771 100644 --- a/src/cascadia/TerminalControl/Resources/en-US/Resources.resw +++ b/src/cascadia/TerminalControl/Resources/en-US/Resources.resw @@ -207,7 +207,7 @@ Please either install the missing font or choose another one. {0} is a file name - Unable to compile the specified pixel shader. + Pixel shader failed to compile: {0} Renderer encountered an unexpected error: {0} @@ -320,4 +320,4 @@ Please either install the missing font or choose another one. Suggested input: {0} {Locked="{0}"} {0} will be replaced with a string of input that is suggested for the user to input - + \ No newline at end of file diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index 1fffe66dc50..80a3a237b28 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -1154,7 +1154,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation const auto hr = std::bit_cast(args.Result()); const auto parameter = args.Parameter(); winrt::hstring message; - + switch (hr) { case HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND): @@ -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) }; diff --git a/src/renderer/atlas/BackendD3D.cpp b/src/renderer/atlas/BackendD3D.cpp index b266d3ba092..07168773da7 100644 --- a/src/renderer/atlas/BackendD3D.cpp +++ b/src/renderer/atlas/BackendD3D.cpp @@ -452,14 +452,22 @@ void BackendD3D::_recreateCustomShader(const RenderingPayload& p) if (error) { LOG_HR_MSG(hr, "%.*hs", static_cast(error->GetBufferSize()), static_cast(error->GetBufferPointer())); + if (p.warningCallback) + { + //to handle compile time errors + const char* errorMessage = static_cast(error->GetBufferPointer()); + std::wstring errorMessageWString(errorMessage, errorMessage + strlen(errorMessage)); + p.warningCallback(D2DERR_SHADER_COMPILE_FAILED, errorMessageWString); + } } 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); + } } } From 542467e7edb8e18ea399712047b56a9cc22187b7 Mon Sep 17 00:00:00 2001 From: blitzRahul <104504393+blitzRahul@users.noreply.github.com> Date: Sat, 15 Jun 2024 11:28:53 +0530 Subject: [PATCH 2/5] proper formatting for TermControl.cpp Ran Invoke-CodeFormat --- src/cascadia/TerminalControl/TermControl.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index 80a3a237b28..5228b5c96d7 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -1154,7 +1154,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation const auto hr = std::bit_cast(args.Result()); const auto parameter = args.Parameter(); winrt::hstring message; - + switch (hr) { case HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND): @@ -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") },parameter) }; + 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) }; From a76e3702aaa903c234324fba8bf8012b4057d222 Mon Sep 17 00:00:00 2001 From: blitzRahul <104504393+blitzRahul@users.noreply.github.com> Date: Sun, 16 Jun 2024 01:24:16 +0530 Subject: [PATCH 3/5] ensure unicode text compatibility BackendD3D.cpp line 457: The error message generated by the compiler is correctly converted to a wstring. Made changes to TermControl.cpp (_RendererWarning function) and Resource.resw to make sure that the file location shows up when an 'unexpected error' occurs. --- .../TerminalControl/Resources/en-US/Resources.resw | 5 +++-- src/cascadia/TerminalControl/TermControl.cpp | 2 +- src/renderer/atlas/BackendD3D.cpp | 9 ++++----- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/cascadia/TerminalControl/Resources/en-US/Resources.resw b/src/cascadia/TerminalControl/Resources/en-US/Resources.resw index 9dc614c1771..0015ca179c7 100644 --- a/src/cascadia/TerminalControl/Resources/en-US/Resources.resw +++ b/src/cascadia/TerminalControl/Resources/en-US/Resources.resw @@ -208,6 +208,7 @@ Please either install the missing font or choose another one. Pixel shader failed to compile: {0} + {0} is the error message generated by the compiler Renderer encountered an unexpected error: {0} @@ -222,8 +223,8 @@ Please either install the missing font or choose another one. {Locked="2023.5.31","MacType"} - Renderer encountered an unexpected error: {0:#010x} {1} - {Locked="{0:#010x}","{1}"} {0:#010x} is a placeholder for a Windows error code (e.g. 0x88985002). {1} is the corresponding message. + Renderer encountered an unexpected error: {0:#010x} {1} "{2}" + {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. Read-only mode is enabled. diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index 5228b5c96d7..45ed3665a40 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -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; } } diff --git a/src/renderer/atlas/BackendD3D.cpp b/src/renderer/atlas/BackendD3D.cpp index 07168773da7..231ede7695e 100644 --- a/src/renderer/atlas/BackendD3D.cpp +++ b/src/renderer/atlas/BackendD3D.cpp @@ -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" @@ -451,18 +452,16 @@ void BackendD3D::_recreateCustomShader(const RenderingPayload& p) { if (error) { - LOG_HR_MSG(hr, "%.*hs", static_cast(error->GetBufferSize()), static_cast(error->GetBufferPointer())); if (p.warningCallback) { //to handle compile time errors - const char* errorMessage = static_cast(error->GetBufferPointer()); - std::wstring errorMessageWString(errorMessage, errorMessage + strlen(errorMessage)); - p.warningCallback(D2DERR_SHADER_COMPILE_FAILED, errorMessageWString); + const std::string_view errMsgStrView{ static_cast(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) { //to handle errors such as file not found, path not found, access denied From 3f4d5829188cd42ebb7014017906ddaa0e6c31b9 Mon Sep 17 00:00:00 2001 From: blitzRahul <104504393+blitzRahul@users.noreply.github.com> Date: Fri, 21 Jun 2024 12:53:01 +0530 Subject: [PATCH 4/5] add conditional warning message construction The default case in TermControl.cpp _rendererWarning function now conditionally constructs the warning message. If the parameter is not empty, we add "{2}" to the 'resourceString'. Removed the "{2}" from resources.resw to accommodate this mechanism. --- .../TerminalControl/Resources/en-US/Resources.resw | 2 +- src/cascadia/TerminalControl/TermControl.cpp | 12 +++++++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/cascadia/TerminalControl/Resources/en-US/Resources.resw b/src/cascadia/TerminalControl/Resources/en-US/Resources.resw index 0015ca179c7..d618171e58b 100644 --- a/src/cascadia/TerminalControl/Resources/en-US/Resources.resw +++ b/src/cascadia/TerminalControl/Resources/en-US/Resources.resw @@ -223,7 +223,7 @@ Please either install the missing font or choose another one. {Locked="2023.5.31","MacType"} - Renderer encountered an unexpected error: {0:#010x} {1} "{2}" + Renderer encountered an unexpected error: {0:#010x} {1} {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. diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index 45ed3665a40..744b61b5958 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -1175,7 +1175,17 @@ 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, parameter) }; + std::wstring resourceString = RS_(L"RendererErrorOther").c_str(); + //conditional message construction + if (!parameter.empty()) + { + resourceString += L" \"{2}\""; + message = winrt::hstring{ fmt::format(std::wstring_view{ resourceString }, hr, msg, parameter) }; + } + else + { + message = winrt::hstring{ fmt::format(std::wstring_view{ resourceString }, hr, msg) }; + } break; } } From 7dbaa3bbcbd04ea0b29acb01b8b2418d4f9f6a59 Mon Sep 17 00:00:00 2001 From: blitzRahul <104504393+blitzRahul@users.noreply.github.com> Date: Fri, 21 Jun 2024 23:58:53 +0530 Subject: [PATCH 5/5] improve efficiency of message construction --- src/cascadia/TerminalControl/TermControl.cpp | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index 744b61b5958..bc045f36381 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -1177,15 +1177,12 @@ namespace winrt::Microsoft::Terminal::Control::implementation const std::wstring_view msg{ &buf[0], len }; std::wstring resourceString = RS_(L"RendererErrorOther").c_str(); //conditional message construction + std::wstring partialMessage = fmt::format(std::wstring_view{ resourceString }, hr, msg); if (!parameter.empty()) { - resourceString += L" \"{2}\""; - message = winrt::hstring{ fmt::format(std::wstring_view{ resourceString }, hr, msg, parameter) }; - } - else - { - message = winrt::hstring{ fmt::format(std::wstring_view{ resourceString }, hr, msg) }; + fmt::format_to(std::back_inserter(partialMessage), LR"( "{0}")", parameter); } + message = winrt::hstring{ partialMessage }; break; } }