MAYA-108586: Keep the TF_ERROR message format consistence with TF_WARN and TF_STATUS #1386

HamedSabri-adsk · 2021-05-04T16:16:27Z

This PR keeps the TF_ERROR message format consistence with TF_WARN and TF_STATUS by displaying the by only displaying the commentary string.

Example below shows the case where an image resource is failed to created which results in a vague message to the user.

Before:

// Error: Failed verification: ' image ' -- Coding Error in _LoadTexture at line 422 of S:\jenkins\workspace\ecg-mayausd-full-2022-python3-windows\ecg-maya-usd\maya-usd\lib\mayaUsd\render\vp2RenderDelegate\material.cpp // 
// Error: Failed verification: ' image ' -- Coding Error in _LoadTexture at line 422 of S:\jenkins\workspace\ecg-mayausd-full-2022-python3-windows\ecg-maya-usd\maya-usd\lib\mayaUsd\render\vp2RenderDelegate\material.cpp // 
// Error: Failed verification: ' image ' -- Coding Error in _LoadTexture at line 422 of S:\jenkins\workspace\ecg-mayausd-full-2022-python3-windows\ecg-maya-usd\maya-usd\lib\mayaUsd\render\vp2RenderDelegate\material.cpp // 
// Error: Failed verification: ' image ' -- Coding Error in _LoadTexture at line 422 of S:\jenkins\workspace\ecg-mayausd-full-2022-python3-windows\ecg-maya-usd\maya-usd\lib\mayaUsd\render\vp2RenderDelegate\material.cpp // 
// Error: Failed verification: ' image ' -- Coding Error in _LoadTexture at line 422 of S:\jenkins\workspace\ecg-mayausd-full-2022-python3-windows\ecg-maya-usd\maya-usd\lib\mayaUsd\render\vp2RenderDelegate\material.cpp // 
// Error: Failed verification: ' image ' -- Coding Error in _LoadTexture at line 422 of S:\jenkins\workspace\ecg-mayausd-full-2022-python3-windows\ecg-maya-usd\maya-usd\lib\mayaUsd\render\vp2RenderDelegate\material.cpp // 
// Error: Failed verification: ' image ' -- Coding Error in _LoadTexture at line 422 of S:\jenkins\workspace\ecg-mayausd-full-2022-python3-windows\ecg-maya-usd\maya-usd\lib\mayaUsd\render\vp2RenderDelegate\material.cpp // 
// Error: Failed verification: ' image ' -- Coding Error in _LoadTexture at line 422 of S:\jenkins\workspace\ecg-mayausd-full-2022-python3-windows\ecg-maya-usd\maya-usd\lib\mayaUsd\render\vp2RenderDelegate\material.cpp //

After the fix with a more meaningful message:

// Error: Failed verification: ' image ' -- Unable to create an image from RetroTVBody_Albedo.png // 
// Error: Failed verification: ' image ' -- Unable to create an image from RetroTVBody_Emissive.png // 
// Error: Failed verification: ' image ' -- Unable to create an image from RetroTVBody_Metallic.png // 
// Error: Failed verification: ' image ' -- Unable to create an image from RetroTVBody_Normal.png // 
// Error: Failed verification: ' image ' -- Unable to create an image from RetroTVBody_AO.png // 
// Error: Failed verification: ' image ' -- Unable to create an image from RetroTVBody_Roughness.png // 
// Error: Failed verification: ' image ' -- Unable to create an image from RetroTVScreen_Albedo.png // 
// Error: Failed verification: ' image ' -- Unable to create an image from RetroTVScreen_Roughness.png //

…N and TF_STATUS by only displaying the commentary string describing the diagnostic message.

seando-adsk · 2021-05-04T17:02:29Z

lib/mayaUsd/utils/diagnosticDelegate.cpp

@@ -107,7 +107,7 @@ void UsdMayaDiagnosticDelegate::IssueError(const TfError& err)
    // In addition, always display the full call site for errors by going
    // through _FormatDiagnostic.
    if (ArchIsMainThread()) {
-        MGlobal::displayError(_FormatDiagnostic(err));
+        MGlobal::displayError(err.GetCommentary().c_str());


Why not do a fix inside of _FormatDiagnostic? If we fall, into the else then it will still print all the info (function/line/file) for error/status/warning. How about inside of _FormatDiagnostic you use if DEBUG check to print all info and in release only the commentary?

@seando-adsk Yeah, I thought about doing it that way but then not all the developers work in DEBUG. :) I myself most of the time work in RelWithDebInfo and barely ever bother with Debug.

The only other option would be to introduce an ENV variable to control this behaviour but again this could be controversial.

But at least having it in DEBUG would provide an option to show all the info (file/function/line). With your change the only way to get that is to not be in the main thread, which I don't know how/why we wouldn't be.

Sure, I don't have a strong preference but do people really need this? Why stop at "Errors"? What about Warnings?

For example, I see in many places in our code based people tend to use TF_WARN when checking for object validation.

if (shaderInstance == nullptr) { TF_WARN("Failed to create shader instance for %s", nodeId.asChar()); break; }

kxl-adsk · 2021-05-10T19:24:26Z

lib/mayaUsd/utils/diagnosticDelegate.cpp

@@ -68,7 +74,8 @@ static MString _FormatDiagnostic(const TfDiagnosticBase& d)
        d.GetContext().GetFunction(),
        d.GetContext().GetLine(),
        d.GetContext().GetFile());
-    return msg.c_str();
+
+    return TfGetEnvSetting(MAYAUSD_SHOW_FULL_DIAGNOSTICS) ? msg.c_str() : d.GetCommentary().c_str();


Most of the time we will not be in full diagnostics mode. With the current code structure, we will always generate a full string. String operations are costly, how about we insert if/else condition and only generate msg string when needed?

Yes agreed. I also wonder about the need to check this new env var on every call for _FormatDiagnostic(). Do we need to be able to modify this env var during a Maya run? How about just getting this var once into a static variable?

@kxl-adsk @seando-adsk I agree with you that std::string allocation can be costly and am very well aware of them. But the cost here is super minimal. I honestly don't believe we don't need to worry about operation costs in here. We can discuss this internally if needed.

Since this is status / warning / error reporting, I agree with Hamed and would not expect this to be high-frequency code, therefore performance isn't too much of a concern.

Now with the extra file path parsing above, I would really like to see this code turned into an if/else condition as Krystian suggests.

I would be happy to see the profiler results to understand where we hit this issue.

lib/mayaUsd/utils/stageCache.cpp

kxl-adsk

Left a small comment but other than this looks good to me.

lib/mayaUsd/utils/diagnosticDelegate.cpp

seando-adsk · 2021-05-10T19:47:17Z

lib/mayaUsd/utils/diagnosticDelegate.cpp

@@ -68,7 +74,8 @@ static MString _FormatDiagnostic(const TfDiagnosticBase& d)
        d.GetContext().GetFunction(),
        d.GetContext().GetLine(),
        d.GetContext().GetFile());


I wonder if we should strip off the path from this and leave only the filename? Isn't part of the problem that we are leaking our paths from the build VMs. I would again suggest that in Debug you leave it alone, but in RelWithDebInfo you strip it. Our Debug builds are never shipped.

Isn't part of the problem that we are leaking our paths from the build VMs

We are not leaking our paths based on the new changes made in this PR. TF Error/Warning/Status are now show only the code commentary.

But a user can enable the new env var and then they will see all the paths from our VMs.

Can we not create a simple regex to strip everything before maya-usd/src?

Ok, I will strip everything before maya-usd:

Before:
S:\jenkins\workspace\ecg-mayausd-full-2022-python3-windows\ecg-maya-usd\maya-usd\lib\mayaUsd\render\vp2RenderDelegate\material.cpp
After:
maya-usd\lib\mayaUsd\render\vp2RenderDelegate\material.cpp

HamedSabri-adsk · 2021-05-11T15:08:45Z

lib/mayaUsd/utils/diagnosticDelegate.cpp

    const std::string msg = TfStringPrintf(
        "%s -- %s in %s at line %zu of %s",
        d.GetCommentary().c_str(),
        TfDiagnosticMgr::GetCodeName(d.GetDiagnosticCode()).c_str(),
        d.GetContext().GetFunction(),
        d.GetContext().GetLine(),
-        d.GetContext().GetFile());
+        filterFilePath.c_str());


filter the file path so it discards everything before the project name (maya-usd).

e.g
Error: Failed verification: ' image ' -- Coding Error in _LoadTexture at line 422 of maya-usd\lib\mayaUsd\render\vp2RenderDelegate\material.cpp

seando-adsk · 2021-05-11T15:14:47Z

lib/mayaUsd/utils/diagnosticDelegate.cpp

+    std::string filterFilePath { d.GetContext().GetFile() };
+    filterFilePath = filterFilePath.substr(filterFilePath.find("maya-usd"));


find() returns npos (-1) if not found, so what if someone clones the repo at a root dir (or names it something other than maya-usd) what does substr(-1) give?

O.K. Yes, if people clone the project to some other names then this code won't do what is supposed to do.

I thought of a better solution where I convert the file path to a relative path. It looks even prettier than previous solution.

dd91145
e.g
// Failed verification: ' image ' -- Unable to create an image from RetroTVBody_Albedo.png -- Coding Error in _LoadTexture at line 430 of Users\sabrih\Desktop\maya-usd\lib\mayaUsd\render\vp2RenderDelegate\material.cpp

seando-adsk

Looks good. Thanks for changing the msg if/else.

MAYA-108586: Keep the TF_ERROR message format consistence with TF_WAR…

62b464c

…N and TF_STATUS by only displaying the commentary string describing the diagnostic message.

HamedSabri-adsk added the enhancement New feature or request label May 4, 2021

HamedSabri-adsk requested a review from kxl-adsk May 4, 2021 16:16

HamedSabri-adsk assigned ecp-maya-devops-adsk May 4, 2021

Removed redundant TF_STATUS("Clearing USD Stage Cache");

1958b13

seando-adsk reviewed May 4, 2021

View reviewed changes

Added consistency to how error/warning/status messages are displayed.

3a84cbe

HamedSabri-adsk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk May 10, 2021

Fix clang format.

bb129ca

HamedSabri-adsk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk May 10, 2021

kxl-adsk reviewed May 10, 2021

View reviewed changes

lib/mayaUsd/utils/stageCache.cpp Show resolved Hide resolved

kxl-adsk reviewed May 10, 2021

View reviewed changes

seando-adsk reviewed May 10, 2021

View reviewed changes

lib/mayaUsd/utils/diagnosticDelegate.cpp Outdated Show resolved Hide resolved

seando-adsk reviewed May 10, 2021

View reviewed changes

filter the file path.

23a4dd3

HamedSabri-adsk commented May 11, 2021

View reviewed changes

seando-adsk reviewed May 11, 2021

View reviewed changes

A better way to filter out the file path.

dd91145

HamedSabri-adsk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk May 11, 2021

Fix unittests.

8b6ea91

HamedSabri-adsk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk May 12, 2021

seando-adsk approved these changes May 12, 2021

View reviewed changes

kxl-adsk approved these changes May 12, 2021

View reviewed changes

HamedSabri-adsk added the ready-for-merge Development process is finished, PR is ready for merge label May 12, 2021

kxl-adsk merged commit 068786a into dev May 13, 2021

kxl-adsk deleted the sabrih/MAYA-108586/concistence_format_for_tf_errors branch May 13, 2021 18:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MAYA-108586: Keep the TF_ERROR message format consistence with TF_WARN and TF_STATUS #1386

MAYA-108586: Keep the TF_ERROR message format consistence with TF_WARN and TF_STATUS #1386

HamedSabri-adsk commented May 4, 2021

seando-adsk May 4, 2021

hamedsabri May 4, 2021

seando-adsk May 4, 2021

hamedsabri May 4, 2021

kxl-adsk May 10, 2021

seando-adsk May 10, 2021

HamedSabri-adsk May 11, 2021

ppt-adsk May 11, 2021

seando-adsk May 11, 2021

HamedSabri-adsk May 11, 2021

kxl-adsk left a comment

seando-adsk May 10, 2021

HamedSabri-adsk May 11, 2021

seando-adsk May 11, 2021

ppt-adsk May 11, 2021

HamedSabri-adsk May 11, 2021

HamedSabri-adsk May 11, 2021

seando-adsk May 11, 2021 •

edited

Loading

HamedSabri-adsk May 11, 2021

seando-adsk left a comment

		std::string filterFilePath { d.GetContext().GetFile() };
		filterFilePath = filterFilePath.substr(filterFilePath.find("maya-usd"));

MAYA-108586: Keep the TF_ERROR message format consistence with TF_WARN and TF_STATUS #1386

MAYA-108586: Keep the TF_ERROR message format consistence with TF_WARN and TF_STATUS #1386

Conversation

HamedSabri-adsk commented May 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kxl-adsk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seando-adsk May 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seando-adsk left a comment

Choose a reason for hiding this comment

seando-adsk May 11, 2021 •

edited

Loading