search

dreamworksanimation / usdmanager

USD Manager
http://www.usdmanager.org
Apache License 2.0
321 stars 60 forks source link

Do not escape usd matching lines #25

Closed fabal closed 2 years ago

fabal commented 2 years ago

Referencing statements in the form of uris may use special characters which will be escaped by cgi.escape, leading to broken links.

mds-dwa commented 2 years ago

Thank you for the contribution! Do you mind updating the commit with the signoff flag, please? git commit --amend --signoff and then force pushing it should do the trick. Thank you!

mds-dwa commented 2 years ago

Are you able to post a particular line this was failing on? I'm concerned this fix may not work as well for some non-USD parsers so we may just need to move where this takes place, plus I'm a little worried there's a deeper bug in the USD parsing logic and would like to double-check some things. Thanks in advance for your help!

fabal commented 2 years ago

No worries at all, this is just how we've fixed it internally without spending too much time on it but feel free to suggest a better place to apply the change. I guess you get the idea: our referencing statements follow the standard uri syntax and the links break in usdmanager for those using an ampersand & in the query part.

mds-dwa commented 2 years ago

I do have a version that moves where the escapes happen so the file evaluation should be using the non-escaped path. However, I don't have any examples using URIs right now, so I can't fully test it. I might be able to upload this to a separate branch tomorrow, if you're willing to test it. Does your URI look something like this?

@my_uri://island.usdz?foo=bar&hello=world@

And if so, did you have to modify the USD RegEx at all in parser.py to be fine with the ? query string portion of the path?

mds-dwa commented 2 years ago

I've pushed a new branch, tmp_pull_25, that has an alternative solution here. Since I don't have a good URI example and resolver to test with, it would be awesome if you could give it a spin at some point. This fix does actually resolve one minor internal issue we had, so I'll probably go with it if it's no better or worse for you. Thanks in advance for any testing feedback you're able to offer.

fabal commented 2 years ago

Thanks Mark, tmp_pull_25 works perfectly with our uris. They are formed very much like your example above. We had issues only with the ampersand (which is not always present in our case), never with the question mark (which is always present). Closing that PR now, in favour of your change.

mds-dwa commented 2 years ago

Thanks for confirming! This fix is now released in 0.13.0!