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

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

Merged
merged 7 commits into from
May 13, 2021
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
15 changes: 15 additions & 0 deletions doc/codingGuidelines.md
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,21 @@ Our goal is to develop [maya-usd](https://github.com/autodesk/maya-usd) followin
* `nullptr` keyword
* …

## Diagnostic Facilities

Developers are encouraged to use TF library diagnostic facilities in Maya USD. Please follow below guideline for picking the correct facility: https://graphics.pixar.com/usd/docs/api/page_tf__diagnostic.html

In MayaUSD, `UsdMayaDiagnosticDelegate` converts TF diagnostics into native Maya infos, warnings, and errors. The environment flag `MAYAUSD_SHOW_FULL_DIAGNOSTICS`, controls the granularity of TF error/warning/status messages being displayed in Maya:

e.g
```
Error: Failed verification: ' image ' -- Unable to create an image from RetroTVBody_Albedo.png
```
vs
```
Error: Failed verification: ' image ' -- Unable to create an image from RetroTVBody_Albedo.png -- Coding Error in _LoadTexture at line 422 of C:\maya-usd\lib\mayaUsd\render\vp2RenderDelegate\material.cpp
```

# Coding guidelines for Python
We are adopting the [PEP-8](https://www.python.org/dev/peps/pep-0008) style for Python Code with the following modification:
* Mixed-case for variable and function names are allowed
Expand Down
3 changes: 2 additions & 1 deletion lib/mayaUsd/render/vp2RenderDelegate/material.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -426,7 +426,8 @@ _LoadTexture(const std::string& path, bool& isColorSpaceSRGB, MFloatArray& uvSca
#else
GlfImageSharedPtr image = GlfImage::OpenForReading(path);
#endif
if (!TF_VERIFY(image)) {

if (!TF_VERIFY(image, "Unable to create an image from %s", path.c_str())) {
return nullptr;
}

Expand Down
36 changes: 26 additions & 10 deletions lib/mayaUsd/utils/diagnosticDelegate.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,12 @@ TF_DEFINE_ENV_SETTING(
"If batching is off, all secondary threads' diagnostics will be "
"printed to stderr.");

TF_DEFINE_ENV_SETTING(
MAYAUSD_SHOW_FULL_DIAGNOSTICS,
false,
"This env flag controls the granularity of TF error/warning/status messages "
"being displayed in Maya.");

// Globally-shared delegate. Uses shared_ptr so we can have weak ptrs.
static std::shared_ptr<UsdMayaDiagnosticDelegate> _sharedDelegate;

Expand Down Expand Up @@ -61,14 +67,19 @@ class _WarningOnlyDelegate : public UsdUtilsCoalescingDiagnosticDelegate

static MString _FormatDiagnostic(const TfDiagnosticBase& d)
{
// strip out all the characters before "maya-usd"
std::string filterFilePath { d.GetContext().GetFile() };
filterFilePath = filterFilePath.substr(filterFilePath.find("maya-usd"));
Copy link
Collaborator

@seando-adsk seando-adsk May 11, 2021

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


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());
return msg.c_str();
filterFilePath.c_str());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


return TfGetEnvSetting(MAYAUSD_SHOW_FULL_DIAGNOSTICS) ? msg.c_str() : d.GetCommentary().c_str();

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

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 would be happy to see the profiler results to understand where we hit this issue.

}

static MString _FormatCoalescedDiagnostic(const UsdUtilsCoalescingDiagnosticDelegateItem& item)
Expand Down Expand Up @@ -104,12 +115,13 @@ void UsdMayaDiagnosticDelegate::IssueError(const TfError& err)
{
// Errors are never batched. They should be rare, and in those cases, we
// want to see them separately.
// In addition, always display the full call site for errors by going
// through _FormatDiagnostic.

const auto diagnosticMessage = _FormatDiagnostic(err);

if (ArchIsMainThread()) {
MGlobal::displayError(_FormatDiagnostic(err));
MGlobal::displayInfo(diagnosticMessage);
} else {
std::cerr << _FormatDiagnostic(err) << std::endl;
std::cerr << diagnosticMessage << std::endl;
}
}

Expand All @@ -119,10 +131,12 @@ void UsdMayaDiagnosticDelegate::IssueStatus(const TfStatus& status)
return; // Batched.
}

const auto diagnosticMessage = _FormatDiagnostic(status);

if (ArchIsMainThread()) {
MGlobal::displayInfo(status.GetCommentary().c_str());
MGlobal::displayInfo(diagnosticMessage);
} else {
std::cerr << _FormatDiagnostic(status) << std::endl;
std::cerr << diagnosticMessage << std::endl;
}
}

Expand All @@ -132,10 +146,12 @@ void UsdMayaDiagnosticDelegate::IssueWarning(const TfWarning& warning)
return; // Batched.
}

const auto diagnosticMessage = _FormatDiagnostic(warning);

if (ArchIsMainThread()) {
MGlobal::displayWarning(warning.GetCommentary().c_str());
MGlobal::displayInfo(diagnosticMessage);
} else {
std::cerr << _FormatDiagnostic(warning) << std::endl;
std::cerr << diagnosticMessage << std::endl;
}
}

Expand Down
1 change: 0 additions & 1 deletion lib/mayaUsd/utils/stageCache.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ struct _OnSceneResetListener : public TfWeakBase

void OnSceneReset(const UsdMayaSceneResetNotice& notice)
{
TF_STATUS("Clearing USD Stage Cache");
UsdMayaStageCache::Clear();

std::lock_guard<std::mutex> lock(_sharedSessionLayersMutex);
Expand Down