PixarAnimationStudios / OpenUSD

Universal Scene Description
http://www.openusd.org
Other
5.45k stars 1.13k forks source link

Sdf permitting unnamed variants creates inconsistencies between Sdf and Usd #3018

Open nvmkuruc opened 1 month ago

nvmkuruc commented 1 month ago

Description of Issue

Sdf (seemingly erroneously) permits unnamed variants, but Usd does not. This creates two situations.

Sdf should always return the variant set spec and should warn or error if it considers the variant set invalid due to an unnamed variant.

Steps to Reproduce

  1. Consider the following snippet of usda.
    
    #usda 1.0

def "prim" (append variantSets = "color" variants = { string color = "red"} ){ variantSet "color" = { "yellow" { int i = 0 } "red" { int i = 7 } } }


2. Run the following python snippet

from pxr import Sdf layer = Sdf.Layer.FindOrOpen('test.usda') type(layer.GetObjectAtPath('/prim{color=}')) <class 'pxr.Sdf.VariantSetSpec'>

/prim{color=} refers to the variant set spec.

  1. Replace "yellow" with the empty string.
    
    #usda 1.0

def "prim" (append variantSets = "color" variants = {string color = "red"}){ variantSet "color" = { "" { int i = 0 } "red" { int i = 7 } } }


4. Run the following python snippet.  It now returns a `VariantSetSpec`.

from pxr import Sdf layer = Sdf.Layer.FindOrOpen('test.usda') type(layer.GetObjectAtPath('/prim{color=}')) <class 'pxr.Sdf.VariantSpec'>

  1. Run usdcat on the layer. You should get this output, where the variant set containing the unnamed variant has been completely removed without warning or error.
    
    #usda 1.0

def "prim" ( variants = { string color = "red" } append variantSets = "color" ) { }



### System Information (OS, Hardware)
Linux

### Package Versions

### Build Flags
jesschimein commented 1 month ago

Filed as internal issue #USD-9486

nvmkuruc commented 1 month ago

We suspect the issue is the VariantName grammar in pathParser.h which is used to validate variant names should be using PEGTL_NS::plus instead of PEGTL_NS::star to reject empty variant names.

spiffmon commented 1 month ago

Thanks, @nvmkuruc - we think the problem is actually the usda parser: it should reject your asset with the unnamed variant in it - it's only once an unnamed variant gets created that things go to pot (so we should reject in the creation of VariantSpec's as well). pathParser needs to accept empty variant names so that we can construct paths that identify the VariantSet.