ABI checker is failing, but I don't think it's because of the changes in this PR:

Codecov Report

Merging #836 (4be0ada) into sdf12 (8e832e7) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##            sdf12     #836   +/-   ##
=======================================
  Coverage   90.76%   90.76%           
=======================================
  Files          78       78           
  Lines       12535    12535           
=======================================
  Hits        11378    11378           
  Misses       1157     1157           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8e832e7...4be0ada. Read the comment docs.

These errors are just part of the USD component, does it make sense to include them in the module and not in the hole library ?

What do you think @adlarkin @azeey @scpeters ?

From the discussion in #828 (comment), I thought it would be okay to add USD error codes to the sdf ErrorCode enum. While I understand the motivation for placing these error codes in the USD component, I think the issue with that approach is that we'd miss out on the functionality provided by the
Error class (Error uses ErrorCode).

@scpeters and @azeey what do you think ?

It would nice to separate the USD error codes and put them in the USD component, but I'm not sure know how we can do that and still use sdf::Error, so I think this PR is the best approach we have.

It would nice to separate the USD error codes and put them in the USD component, but I'm not sure know how we can do that and still use sdf::Error, so I think this PR is the best approach we have.

How about wrapping a usd specific error class in a new WRAPPED_ERROR code? A new method will be added to Error to retrieved the wrapped error, in order to access the original error object, you will need to do some runtime type introspection.

But then looking at some of the methods in sdf::Error like XmlPath and LineNumber, it seems quite specific to sdf, so if possible, I would prefer using a different class specific to usd errors.

How about wrapping a usd specific error class in a new WRAPPED_ERROR code? A new method will be added to Error to retrieved the wrapped error, in order to access the original error object, you will need to do some runtime type introspection.

I'm not sure if I understand the approach proposed here - is the idea to add a new enum to ErrorCode that would represent a wrapped error class, and then at runtime, if the error is of this wrapped type, then another method can be called to get the underlying error type? In theory, I think this could work, but I'm not sure if there would be any drawbacks to this approach. @azeey @ahcorde what do you think? Should we stick with 00a3dc6, where the USD error types are listed directly in ErrorCode, or should we try to wrap/hide them somehow? There are several other SDF -> USD PRs that are being blocked by this, so it would be nice to figure out the best path moving forward so that we can get this merged.

I'm not sure if I understand the approach proposed here - is the idea to add a new enum to ErrorCode that would represent a wrapped error class, and then at runtime, if the error is of this wrapped type, then another method can be called to get the underlying error type?

Yeah, thats right. I think that if possible, we should use the usd error directly, and only wrap it in a sdf error when we need to pass it to other components.

I made kp/wrap-usd-error to experiment with wrapping errors. It adds a usd specific sdf::usd::Error class, one of the error code is a wrapped sdf error. Currently there is no need to propagate any usd errors to sdf components, but if there is a need, we can consider adding a generic error code to sdf::Error.

@koonpeng I like the approach you prototyped. We might need to be more careful about namespaces since Error and Errors are defined in sdf and sdf::usd. And at first I thought there some code duplication between sdf::Error and sdf::usd::Error and I wanted to see if there's a way to avoid that by using another sdf::Error object inside sdf::usd::Error to contain the data (eg. lineNumber, filePath), but I don't think we can avoid writing the constructors and member functions of sdf::usd::Error. Your approach gives more flexibility to the usd component and other future components to capture the context they need for their specific error types.

Whether sdf::Error needs a generic error code remains to be seen, but it might be a good idea to introduce the error code now so we can use it later without breaking ABI.

@koonpeng, thanks for the prototype - I agree with what @azeey said, this looks pretty good. Unfortunately, there is some code duplication that probably cannot be avoided, but if we want to separate USD errors from SDF errors, this may be the best approach we have at the moment. Regarding namespaces/potential naming collisions, perhaps we can make the following changes to the prototype in kp/wrap-usd-error:

• rename the Error.hh file in the usd component to UsdError.hh
• rename sdf::usd::ErrorCode to sdf::usd::UsdErrorCode
• rename sdf::usd::Error to sdf::usd::UsdError
• rename sdf::usd::Errors to sdf::usd::UsdErrors

I'm not sure if some of the renaming I proposed above makes sense since these objects are already in the USD namespace (perhaps the Usd prefix after the usd namespace seems redundant), but I would like to figure out a way to make more distinct use of naming/namespaces. Another option we have is to always use the usd namespace when creating errors in the USD component (so, code should always read as usd::Error instead of Error), but this may be difficult to enforce as the USD component codebase continues to grow (I can imagine a developer and/or code reviewer forgetting to enforce this rule at some point).

@koonpeng after some discussion that was had offline, we are going to go with your approach of creating an error class in the USD component and wrapping sdf::Error if needed. Would you mind either opening your own PR that targets the sdf12 branch with your prototype, or pushing a commit with your prototype to this PR? If you open your own PR, I will go ahead and close this one.

@adlarkin I merged my branch with the suggested changes into this pr. I'm going delete my branch now that it is merged in.

@koonpeng and @adlarkin

• I fixed the style to match with other files in the library
• Added UsdError_TEST.cc

@osrf-jenkins retest this please

@azeey or @scpeters do you mind to review it ?

All review feedback has been addressed, but I would like to hear what others think about #836 (comment).

I'm planning to merge this if CI comes back green, unless anyone has any other final review comments.

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-04-13-fortress-edifice/1367/1