Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Apple: Accessibility API schemas #3271

Open
wants to merge 17 commits into
base: dev
Choose a base branch
from

Conversation

dgovil
Copy link
Contributor

@dgovil dgovil commented Sep 4, 2024

This PR adds an Accessibility API schemas to UsdUI as an attempt to make it a Universally Accessible Scene Description. Previously it also included Localization, but after discussion, that has been moved to #3475

Disclaimer: This addition does not represent any future feature support in our products. These schemas are borne out of discussing the needs for the ecosystem to accommodate a wider range of users.

Overview

A brief overview of the schema:

Proposal

def "Foo"(
    apiSchemas = ["AccessibilityAPI:default"]
) {
    string accessibility:default:label = "Short Label"
    string accessibility:default:description = "A longer description of this prim"
    uniform token accessibility:default:priority = "standard"
}

UsdUIAccessibilityAPI is a multiple apply schema.
Each instance has three attributes:

label: A short description of the prim
description: An extended description of the prim
priority: A hint to a runtime on how this instance should be valued

Feedback and Remaining Work

It would be greatly appreciated if feedback could be provided with a focus on the schema first so we can nail down how to represent the data in the USD.

  • I have verified that all unit tests pass with the proposed changes
  • I have submitted a signed Contributor License Agreement

@nvmkuruc
Copy link
Collaborator

nvmkuruc commented Sep 5, 2024

Do you have any thoughts on how localization should work across schema domains? For example-- consider the following scene description.

def Xform "prim_with_primvar" (apiSchemas = ["LocalizationAPI:default", "LocalizationAPI:fr_CA"]) {
    string primvars:greeting = "hello"
    string primvars:greeting:lang:fr_CA = "bonjour"
}

I could see some clients may interact with primvars:greeting as a localized attribute with the UsdUILocalizationAPI and others may use the UsdGeomPrimvarsAPI and treat greeting and greeting:lang:fr_CA as two distinct primvars.

Localization and UsdShade is an interesting case to consider as well.

@dgovil
Copy link
Contributor Author

dgovil commented Sep 5, 2024

@nvmkuruc That's a good question and something I forgot to mention in my PR docs (I had a line in the proposal discussion somewhere)

But yes, I think I wanted to have language on restricting the set of attributes one should localize. I wasn't sure how best to phrase that or whether the API should do validation itself around that.

I think longer term, my thought would be that localization should only be used for things like strings (localized text), asset paths (localized textures?) and rels (localized animation bindings for lip-syncs?).

However, even there, I think at the start my recommendation phrasing is that it should be limited to only strings at the start, and that it's only for strings that will be presented to a user as part of content consumption.

I think that recommendation would perhaps side-step the issue you mention since I don't know of a scenario where a primvar would fall under that. If that kind of wording would help, I think we could brainstorm how to phrase it here.

@nvmkuruc
Copy link
Collaborator

nvmkuruc commented Sep 5, 2024

I'm not sure that it sidesteps it because string and asset paths are commonly used as primvars and shader inputs for specifying or generating texture paths.

I'd expect the current implementation of UsdGeomPrimarsAPI and UsdShadeShader would return a unique input or primvar that had localized string or asset properties.
https://openusd.org/dev/api/class_usd_geom_primvars_a_p_i.html#a2336d8d750d52f1a8950abe5cfafb75a
https://openusd.org/dev/api/class_usd_shade_shader.html#aef8e1bf11a790edd7ffa7a90de4c5cb7

@dgovil
Copy link
Contributor Author

dgovil commented Sep 5, 2024

Hmm, I think that would be fine if they get returned in the list of properties.
Unless a UI is showing every single property on a prim, I don't believe it would cause any issues since the use would be targeted, so the existence of extra properties would be benign? Would you have a specific scenario where it might cause issues?

But also I think the initial recommendation of only limiting it to strings that are presented to users for consumption might avoid that for the time being

@jesschimein
Copy link
Collaborator

Filed as internal issue #USD-10083

@jesschimein
Copy link
Collaborator

/AzurePipelines run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@spiffmon
Copy link
Member

spiffmon commented Sep 6, 2024

Apologies for joining the party so late, but my hope would be that the schemas' scoping to UsdUI would (self and explicit) document that we should only be using it for localization/accessibility of UI-facing data, and such localization should not have any cross-schema interaction (I can't decide whether UsdUI is actually "lower level" than, e.g. UsdGeom, but it doesn't feel right for UI concerns to affect scene evaluation for rendering).

@nvmkuruc , is it reasonable to say that asset rendering localization is a different problem than Accessibility, or are we really trying to get both of those birds in the same sack?

@dgovil
Copy link
Contributor Author

dgovil commented Sep 6, 2024

@spiffmon I believe I'm advocating for the same thing as you. I think (at least to start) it should only be used for strings that are presented to a user in a consumable way. Ie not generic attributes that are shown in an editor UI, but strings that themselves are shown as UI (such as the Autodesk text proposal) or expressed to the UI like accessibility over.

I do think that people might use it to express other localizations in the future, which is why I mention the other possibilities. However I have language in the doc to say that any uses of localization are dependent on knowing what the endpoints support.

I can strengthen the wording to emphasize the recommendation to only use it for user facing strings etc...

@spiffmon
Copy link
Member

spiffmon commented Sep 6, 2024

From my POV, it would be good to keep the scoping tighter for now... there's just some meaty problems to talk through and solve if we start allowing it to be used for localizing texture names, especially in a procedural way, and/or through the primvar-propagation mechanism.

@dgovil
Copy link
Contributor Author

dgovil commented Sep 6, 2024

Yep, agreed. I'll try and come up with suitable language for the doc to strengthen that recommendation

@nvmkuruc
Copy link
Collaborator

nvmkuruc commented Sep 6, 2024

Yep, agreed. I'll try and come up with suitable language for the doc to strengthen that recommendation

I think one of the things that has been throwing me is that the example in the PR description shows string foo as an attribute that can be localized. That implied to me that "any attribute" (or "any string attribute") can be localized and why I started to wonder about the implications for primvars, shader inputs, etc.

@spiffmon
Copy link
Member

spiffmon commented Sep 6, 2024

I agree that's a concern, @nvmkuruc , and the only way in USD-of-today to make the limited scope obvious in the data itself instead of documentation would be to require a namespace for all localizable strings.

In the future, if we are copacetic with core USD behaviors relying on OpenExec, I believe it should be possible for the "localization behavior" (which means tracking dependencies between attributes to get proper dirtying, as well as evaluation behavior for which attribute should be consulted) to be injected where it is needed, given an applied schema to impart it. I am not 100% sure of that and haven't had time to talk to exec folks about it yet. But if we prescribe a namespace now, it cuts off that possibility. If we leave it in the not-completely-satisfying-or-validatable state of "limitations by fiat/documentation", then we leave the door open.

Would it be worth taking some time to see how far we can go with validation without generating false negatives?

@dgovil
Copy link
Contributor Author

dgovil commented Sep 6, 2024

I'm thinking a reasonable check right now might be:

  • Is a string attribute
  • Is not a primvar or an input/output

I think that would cover most of the concerns?

@dgovil
Copy link
Contributor Author

dgovil commented Sep 16, 2024

Just pinging this thread again, do the changes in my previous message sound reasonable? I can update the PR accordingly?

@dgovil
Copy link
Contributor Author

dgovil commented Sep 24, 2024

@nvmkuruc and @spiffmon I added validation functions and parameters in the code to check if something can be localized.

CanLocalize(attribute) -> Checks if its a string attribute and if its not a primvar
CanLocalize(relationship) -> Returns false always but I figured it was nice to just have the API symmetry

All the creation functions now have a validate method that checks first if they can actually localize this, but I allow this to be skipped with a documentation note that this requires caution when doing so.

Let me know if that sounds better to you?

@spiffmon
Copy link
Member

Hi @dgovil , Can*() and Validatemethods sound great.

But also, upon another read, I did not think it was possible, but in any case not desirable, to have a multiple-apply schema that elides the instance-name from its builtin properties, and I think we do want to canonize the Accessibility properties. So I'd like to push back on eliding the "default" instance name from the "base" prim properties. We could still have a helper ApplyDefaultAPI() method to canonize the default without programs needing to supply the literal?

@dgovil
Copy link
Contributor Author

dgovil commented Sep 24, 2024

Okay that's fair. I updated it so it keeps the default schema name in the attribute name.
Regarding making the default easier to make, I just set the default token name in the header to default_. So you can create the API without naming it.

One downside is that the UsdGenSchema overwrites that default when run. I wonder if maybe having it understand a default instance name would be a good addition to the UsdGenSchema as well?

@dgovil
Copy link
Contributor Author

dgovil commented Sep 26, 2024

Also changed the localization attribute for default language to include the instance name too, but I disallow creation from other instance names than default, and fetching the attribute will always get it from the default instance.

@jesschimein
Copy link
Collaborator

/AzurePipelines run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@spiffmon
Copy link
Member

One downside is that the UsdGenSchema overwrites that default when run. I wonder if maybe having it understand a default instance name would be a good addition to the UsdGenSchema as well?

That could be something to consider for the future, but for now, you just can't, and the change will fail our internal automation if usdGenSchema gets a different result. So, similarly to [UsdShade::CreateMaterialBindSubset()|https://github.com/PixarAnimationStudios/OpenUSD/blob/release/pxr/usd/usdShade/materialBindingAPI.h#L924], can we stick to a custom method that we can document as the primary entrypoint?

@dgovil
Copy link
Contributor Author

dgovil commented Sep 26, 2024

@spiffmon if you're re-running usdGenSchema in your automation, how are you getting around needing extra headers in the cpp file? e.g I need primvars.hto check if its a primvar, but if I include it up top where all the other headers belong, it gets overwritten when running it again.

Are you putting the headers lower down in the file after the point where Jinja doesn't replace content in that case?

@dgovil
Copy link
Contributor Author

dgovil commented Sep 26, 2024

Anyway, made the change by adding CreateDefaultAPI and ApplyDefaultAPI methods on both schemas.
So now re-running the usdGenSchema doesn't cause any changes.

I haven't added any Python bindings for the custom methods yet. I can do that once I get a 👍 on the C++ API additions.

@spiffmon
Copy link
Member

spiffmon commented Sep 26, 2024

See Customizing Per-Class Properties for exactly that use-case (which appears in UsdGeom schema.usda) - spolier, it's customData[extraIncludes]

@dgovil
Copy link
Contributor Author

dgovil commented Sep 27, 2024

Ah that's good to know. I changed that over.
I'll probably make a separate PR down the road for the schema gen to allow for a section in the .cpp file where includes can be preserved since the extraIncludes puts it in the header itself (which is fine for this, but I imagine other cases where that might be undesirable)

@spiffmon
Copy link
Member

Are there cases where it's insufficient to just put includes in custom section of the cpp - that's what we generally do. Unless the header actually needed types defined in an external header, I don't think it's possible for the boilerplate section of the cpp to incur any other dependencies.

@dgovil
Copy link
Contributor Author

dgovil commented Sep 27, 2024 via email

@jesschimein
Copy link
Collaborator

/AzurePipelines run

This commit adds two applied API schemas to UsdUI as an attempt to make it a Universally Accessible Scene Description.

Disclaimer: This addition does not represent any future feature support in our products. These schemas are borne out of discussing the needs for the ecosystem to accommodate a wider range of users.

2. Localization schema as discussed at

## Overview
A brief overview of the two schemas:

### Accessibility API
[Proposal](https://github.com/PixarAnimationStudios/OpenUSD-proposals/tree/main/proposals/accessibility)

```
def "Foo"(
    apiSchemas = ["AccessibilityAPI:default"]
) {
    string accessibility:label = "Short Label"
    string accessibility:description = "A longer description of this prim"
    uniform token priority = "standard"
}
```

UsdUIAccessibilityAPI is a multiple apply schema.
Each instance has three attributes:

label: A short description of the prim
description: An extended description of the prim
priority: A hint to a runtime on how this instance should be valued

The default instance does not namespace itself with the instance name as we believe it will be the only one specified in many cases, and the excess namespace doesn't add clarity.

As a convenience, the API specifies a default name so users of Apply() do not need to specify a name most of the time.

### Localization API
[Proposal](https://github.com/PixarAnimationStudios/OpenUSD-proposals/tree/main/proposals/language)

```
def "Foo" (
    apiSchemas = ["LocalizationAPI:default", "LocalizationAPI:fr_CA"]
)
{
     uniform token localization:lang = "en_US"
     custom string foo = "Hello"
     custom string foo:lang:fr_CA = "Bonjour"
     custom string foo:lang:hi_IN = "नमस्ते"
}

```

UsdUILocalizationAPI is also a multiple apply schema.
However here it uses the multiple apply to denote what language are available or that a default localization is available on a prim.

It includes one attribute that doesn't use an instance name called `localization:langauge` which specifies the default language to assume.

All other applications are suffixed to properties with `:lang:<language>`

## Feedback and Remaining Work

It would be greatly appreciated if feedback could be provided with a focus on the schema first so we can nail down how to represent the data in the USD.

I also haven't finished wrapping the LocalizationAPI methods in Python because I would like to first get feedback on the C++ API so that I may iterate faster. Once we feel the API looks good, I'll handle the Python bindings and write appropriate tests for them as well.
@dgovil
Copy link
Contributor Author

dgovil commented Dec 20, 2024

Per the request, I've split up the PR so this one focuses just on Accessibility. I'll put up the Localization schema later today.

Copy link
Collaborator

@nvmkuruc nvmkuruc left a comment

Choose a reason for hiding this comment

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

Thanks Dhruv!

pxr/usd/usdUI/schema.usda Outdated Show resolved Hide resolved
pxr/usd/usdUI/schema.usda Show resolved Hide resolved
@dgovil
Copy link
Contributor Author

dgovil commented Dec 20, 2024

Would you guys have an example of wrapping method overloads for Python? I have two overloads for CreateDefaultAPI. One for each initializer:

  1. static UsdUIAccessibilityAPI CreateDefaultAPI(const UsdPrim& prim);
  2. static UsdUIAccessibilityAPI CreateDefaultAPI(const UsdSchemaBase& schemaObj);

Boost Python says RuntimeError: Boost.Python - All overloads must be exported before calling 'class_<...>("AccessibilityAPI").staticmethod("CreateDefaultAPI")'

In the meantime, I've only exposed the prim version of it to Python, because I couldn't find an incantation that allowed for both with the same name, and wasn't sure what the USDism is here

@dgovil dgovil changed the title Apple: Accessibility and Localization API schemas Apple: Accessibility API schemas Dec 20, 2024
@nvmkuruc
Copy link
Collaborator

Would you guys have an example of wrapping method overloads for Python? I have two overloads for CreateDefaultAPI. One for each initializer:

  1. static UsdUIAccessibilityAPI CreateDefaultAPI(const UsdPrim& prim);
  2. static UsdUIAccessibilityAPI CreateDefaultAPI(const UsdSchemaBase& schemaObj);

Boost Python says RuntimeError: Boost.Python - All overloads must be exported before calling 'class_<...>("AccessibilityAPI").staticmethod("CreateDefaultAPI")'

In the meantime, I've only exposed the prim version of it to Python, because I couldn't find an incantation that allowed for both with the same name, and wasn't sure what the USDism is here

Here's an example from UsdSemantics in case it helps. https://github.com/PixarAnimationStudios/OpenUSD/blob/release/pxr/usd/usdSemantics/wrapLabelsAPI.cpp#L176

@dgovil
Copy link
Contributor Author

dgovil commented Dec 20, 2024

Split out the Localization PR here #3475

@dgovil
Copy link
Contributor Author

dgovil commented Dec 20, 2024

@nvmkuruc I don't see an overload of either custom method. Apologies if I'm missing it though.

@nvmkuruc
Copy link
Collaborator

nvmkuruc commented Dec 20, 2024

Apologies, I misunderstood the issue. How many times are you calling .staticmethod? Once or once per overload?

@dgovil
Copy link
Contributor Author

dgovil commented Dec 20, 2024

No worries! I tried once per overload, and just the once for one of the overloads, but both seem to make it unhappy. :-/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants