lighttransport / tinyusdz

Tiny, dependency-free USDZ/USDA/USDC library written in C++14
Other
519 stars 44 forks source link

Composition: Inheritance management #209

Open rdeioris opened 2 weeks ago

rdeioris commented 2 weeks ago

Hi, the current Inheritance composition arc implementation is mostly correct. to sum up inheritance from spec:

A PrimSpec gets its props and children from another PrimSpec with the specifier "class" (this is not strictly required, but very common as classes do not appear in the final Stage). Properties and children of the inheriting PrimSpec have higher priority.

Note: the class must not be under the inheriting PrimSpec (as specified in https://remedy-entertainment.github.io/USDBook/terminology/inherits.html#inherits)

Prims cannot inherit from an ancestor or descendant, inherit “bases” should be defined as siblings or outside of the target prim’s hierarchy, for example at the root level

Giving the following usda:

#usda 1.0

class Mesh "Base" {
    def Cube "Box" 
    {
        string value = "Test"
    }
}

def "Root" {
    def "Test" ( inherits = </Base>)
    {
        over "Box"
        {
            string value = "Test2"
        }
    }
}

def "Root2" {
    def "Test" ( inherits = </Base/Box>)
    {

    }
}

def "Root3" {
    def "Test" ( inherits = </Base>)
    {
        string value = "Test2"
    }
}

OpenUSD usdcat returns:

#usda 1.0
(
    doc = """Generated from Composed Stage of root layer /mnt/e/Documents/Unreal Projects/GltfUsdPlayground/Assets/classes.usda
"""
)

class Mesh "Base"
{
    def Cube "Box"
    {
        string value = "Test"
    }
}

def "Root"
{
    def Mesh "Test"
    {
        def Cube "Box"
        {
            string value = "Test2"
        }
    }
}

def "Root2"
{
    def Cube "Test"
    {
        string value = "Test"
    }
}

def "Root3"
{
    def Mesh "Test"
    {
        string value = "Test2"

        def Cube "Box"
        {
            string value = "Test"
        }
    }
}

While tusdcat fails as there is a bug in the caching implementation (Root3 will fail as the value is assumed to be in cache, fix will be included in the pull request).

Once the caching is fixed the output is:

#usda 1.0

class Mesh "Base"
{
    def Cube "Box"
    {
        string value = "Test"
    }
}

def "Root2"
{
    def "Test"
    {
        string value = "Test"
    }
}

def "Root"
{
    def "Test"
    {
        def Cube "Box"
        {
            string value = "Test2"
        }
    }
}

def "Root3"
{
    def "Test"
    {
        string value = "Test2"
        def Cube "Box"
        {
            string value = "Test"
        }
    }
}

Almost correct, the only missing piece is the typeName inheritance (fix in pull request).

Finally:

#usda 1.0

def "Root" {
    def "Test" ( inherits = </Root/Test/Base>)
    {
        class Mesh "Base" {
            def Cube "Box" 
            {
                string value = "Test"
            }
        }
    }

}

This is an invalid config, usdcat returns:

In </Root/Test>: Cycle detected:
@XXX@</Root/Test>
CANNOT inherit from:
@XXX@</Root/Test/Base>
 (instantiating stage on stage @XXX@ <0x5648ae149360>)

while tusdcat processes it (swallowing the class block)

#usda 1.0

def "Root"
{
    def "Test"
    {
        def Cube "Box"
        {
            string value = "Test"
        }
    }
}

I think tusdcat should fail too here, thoughts?

syoyo commented 2 weeks ago

Thanks! Let me give some time to double-check the logic of Inheritance and its implementation in TinyUSDZ.

syoyo commented 2 weeks ago

For 'cycle detected' error case, this rule is applied:

Prims cannot inherit from an ancestor or descendant

'Base' Prim is the child of 'Test' Prim, so TinyUSDZ also should fail(and report an error)

I will summerize the logic of Inherit op and put it to wiki. Then do some tests, then merge the PR.

syoyo commented 2 weeks ago

I have described the logic of Inherits to wki: https://github.com/lighttransport/tinyusdz/wiki/Composition-arcs#inherits

PR #210 has been merged.

Prims cannot inherit from an ancestor or descendant, inherit “bases” should be defined as siblings or outside of the target prim’s hierarchy, for example at the root level

After adding Cycle detection check of "inherits" Prim path in another PR, Inheritance management should be complete in TinyUSDZ!