Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
3 changes: 3 additions & 0 deletions package/com.unity.formats.usd/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
- Set material name in Unity to match the material name in the USD file on import.
- Fixed USDPayload's "Is Loaded" field in the inspector staying at false even when the payload has been loaded.

### Bug Fixes
- Fix the scene primMap that was corrupted with invalid prim making the write fail.

## [1.0.4-preview.1] - 2020-07-24
### Changed
- Updated USD version to 20.08
Expand Down
Binary file modified package/com.unity.formats.usd/Runtime/Plugins/USD.NET.dll
Binary file not shown.
Binary file modified package/com.unity.formats.usd/Runtime/Plugins/USD.NET.pdb
Binary file not shown.
28 changes: 28 additions & 0 deletions src/Tests/Cases/BasicTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -566,5 +566,33 @@ public static void GetUsdObjectsTest() {
AssertTrue(scene.GetRelationshipAtPath("/Foo.number") == null);
AssertTrue(scene.GetAttributeAtPath("/Foo.rel") == null);
}

public static void ReadNonExistingPrimsTest()
{
var scene = USD.NET.Scene.Create();

var sample = new MinimalSample();
// Read non existing prim, not in the prim map
scene.Read<MinimalSample>("/Foo", sample);
AssertTrue(sample.number == 0);
AssertTrue(sample.rel == null);

sample.number = 45;
sample.rel = new USD.NET.Relationship("/Foo");
scene.Write("/Foo", sample);

//// Reading the prim adds it the internal prim map
var sample2 = new MinimalSample();
scene.Read<MinimalSample>("/Foo", sample2);
AssertTrue(sample.number == sample2.number);
AssertTrue(sample.rel != null);

// Reading a non existing prim that is still in the prim map
scene.Stage.RemovePrim(new pxr.SdfPath("/Foo"));
var sample3 = new MinimalSample();
scene.Read<MinimalSample>("/Foo", sample3);
AssertTrue(sample3.number == 0);
AssertTrue(sample3.rel == null);
}
}
}
1 change: 1 addition & 0 deletions src/Tests/UnitTestDriver.cs
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ static void RunTestCases() {
Cases.BasicTests.PrimvarsTest();
Cases.BasicTests.SampleBaseTest();
Cases.BasicTests.GetUsdObjectsTest();
Cases.BasicTests.ReadNonExistingPrimsTest();

Cases.UnityIoTests.VectorsTest();
Cases.UnityIoTests.QuaternionTest();
Expand Down
10 changes: 6 additions & 4 deletions src/USD.NET/serialization/Scene.cs
Original file line number Diff line number Diff line change
Expand Up @@ -745,10 +745,12 @@ private UsdPrim GetUsdPrim(string path) {
private pxr.UsdPrim GetUsdPrim(SdfPath path) {
UsdPrim prim;
lock(m_stageLock) {
if (!m_primMap.TryGetValue(path, out prim) || !prim.IsValid()) {
prim = Stage.GetPrimAtPath(path);
m_primMap[path] = prim;
}
if (!m_primMap.TryGetValue(path, out prim) || !prim.IsValid()) {
prim = Stage.GetPrimAtPath(path);
if (prim.IsValid()) {
m_primMap[path] = prim;
}
Copy link
Contributor

@jcowles jcowles Oct 29, 2020

Choose a reason for hiding this comment

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

If the prim isn't valid (after calling GetPrimAtPath) and the path does exist in the map, should the path be cleared here to keep the primMap accurate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good question... I don't know ^^. What would be the case where it happens?... If a prim is removed on disk maybe?? Then, when refreshing... the primMap wouldn't be accurate?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that's what I was thinking -- like a file is regenerated and some prims are removed, or some flavor of that.

I don't think this is a /bug/ but I think it would be a good axiom to enforce that the prim map is always either pointing to a valid prim or has no entry.

Copy link
Contributor

@jcowles jcowles Oct 29, 2020

Choose a reason for hiding this comment

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

(It should be super easy to test this with an ascii file, just delete the prim and see what happens on reload at that point, but as I read the code, it looks like an entry will be left in the prim map)

Copy link
Contributor

Choose a reason for hiding this comment

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

@jcowles What is the benefit of the primMap here? Stage.GetPrimAtPath is a sdf operation and will be cached anyway so it should be fast.
If we keep a local cache in Scene I can't see a reliable way to ensure the cache stays in sync while we expose the stage as public. We could provide our own Scene.RemovePrim and Scene.Reload but nothing prevents the users to call scene.Stage.RemovePrim or scene.Stage.Reload ...

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the original motivation was to avoid the C++/C# roundtrip just to get a prim, though it seems silly now.

I wonder if we can just kill it entirely -- given that this requires grabbing a lock, seems like that would be nice.

Copy link
Contributor

Choose a reason for hiding this comment

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

Created a ticket for this one: https://jira.unity3d.com/browse/USDU-135

}
}
return prim;
}
Expand Down