IECoreUSD : Use / for asset paths
#1426
Conversation
ericmehl
force-pushed
the
USDSlashWindows
branch
from
566dd76
to
a9eea20
Compare
|
Forgot to post the other day - I made the replacing of slashes contingent on the environment variable |
ericmehl
force-pushed
the
USDSlashWindows
branch
2 times, most recently
from
019efe1
to
c775c99
Compare
|
Thanks @johnhaddon! I think I have a nice efficient solution with the Ready for a new look. |
On Windows, the result of USD's `GetResolvedPath()` method returns the resolved path using `\` between directories. This can cause problems when doing string substition on a resolved path since the `\` will be interpreted as the start of an escape character. Using `/` also conforms to Cortex's standard of always using `/` for paths because they work on all operating systems.
ericmehl
force-pushed
the
USDSlashWindows
branch
from
c775c99
to
a3c9fff
Compare
No worries, these are nice improvements. I addressed the new comments, ready for another look! |
| fieldAsset.GetFilePathAttr().Get( &fieldAssetPath, time ); | ||
| const std::string fieldFileName = fieldAssetPath.GetResolvedPath(); | ||
| ConstDataPtr fieldFileNameData = DataAlgo::fromUSD( fieldAsset.GetFilePathAttr(), time ); | ||
| const std::string fieldFileName = runTimeCast<const StringData>( fieldFileNameData )->readable(); |
There was a problem hiding this comment.
runTimeCast should always be accompanied with a check for null - by using runTimeCast you are saying that you don't know what the type is. I think it would be reasonable to use static_cast or assertStaticCast here though, since we know the result type.
There was a problem hiding this comment.
Ah, I was not quite clear on when to use runTimeCast vs static_cast, thanks for that explanation. I went with static_cast in ffe4507 since dataFromSdfAssetPath() always returns at least an empty StringData.
| @@ -169,7 +193,7 @@ IECore::DataPtr dataFromSdfAssetPath( const SdfAssetPath &assetPath, const pxr:: | |||
| ) | |||
| { | |||
| return new StringData( | |||
| SdfComputeAssetPathRelativeToLayer( spec->GetLayer(), assetPath.GetAssetPath() ) | |||
| SdfComputeAssetPathRelativeToLayer( spec->GetLayer(), p ) | |||
There was a problem hiding this comment.
p is derived from GetResolvedPath(), but we need to continue to pass GetAssetPath() here. I think this is most likely responsible for the CI failure.
I also wonder if SdfComputeAssetPathRelativeToLayer() might introduces \ of its own? Maybe it's better to do the substitutions on the result than the argument?
There was a problem hiding this comment.
Right, that makes sense. Fixed in 041cfa7
ericmehl
force-pushed
the
USDSlashWindows
branch
from
a3c9fff
to
ffe4507
Compare
|
Ok, got the tests passing again and I think now have all the fixups made. |
|
Thanks for the update. I've squashed and merged this via command line, since I didn't have permissions to push a squashed version to the PR for some reason. |
This fixes a bug where asset and volume paths were using
\on Windows. This meant that doing string substitution would lead to interpreting the\as the start of an escape sequence.This came up for a user using a
ShaderQuerynode in Gaffer.ShaderQuerysets the default texture path for a query from the USD asset name, and the built-in string substitution will cause the\\to be collapsed into a single\, the start of an escape sequence and an incorrect file name.I think this can be considered a bug fix rather than a breaking change? If it would be better to consider it a breaking change I can retarget to
main.Checklist