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

Autodesk: Update the LineStyle proposal from suggestions #60

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

erikaharrison-adsk
Copy link
Contributor

@erikaharrison-adsk erikaharrison-adsk commented Apr 19, 2024

Description of Proposal

It is preferred to use a separate schema for the DashDotLines, because the dash dot patterns can only be applied for the lines. If we directly add style property to the BasisCurves, the style property will be only valid when the type is linear. This constraint is not convenient. We also add a separate rprim for the new primitive. In this proposal, I add a separate section talking about the extents of the DashDotLines, and a section talking about how to handle screen space pattern. At the end of the proposal, two examples are provided.

Link to Rendered Proposal

Supporting Materials

Previous PRs:

Contributing

It is preferred to use a separate schema for the DashDotLines, because the dash dot patterns can only be applied for the lines. If we directly add style property to the BasisCurves, the style property will be only valid when the type is linear. This constraint is not convenient.
We also add a separate rprim for the new primitive.
In this proposal, I add a separate section talking about the extents of the DashDotLines, and a section talking about how to handle screen space pattern.
At the end of the proposal, two examples are provided.
Copy link

@jcowles jcowles left a comment

Choose a reason for hiding this comment

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

Here are a couple comments, mostly just noodling the details.

The bigger question in my mind is still if this schema is needed - I totally understand the utility of it, but it's also yet another schema to support everywhere, in addition to the underlying existing curves...

- widths. Widths are now interpreted as the widths in screen space.

The DashDotLines has the following new properties:
- screenSpacePattern. A bool uniform. By default it is true, which means the dash-dot pattern will be based on screen unit. If we zoom in, the pattern on the line will change in the world space, so that the dash size and the dash gap size in the screen space will not change. If it is false, the pattern will be based on world unit. If we zoom in, the pattern on the line will not change in world space. The dash size and the dash gap size in the screen space will be larger.
Copy link

Choose a reason for hiding this comment

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

I think "screenSpace" should be one word, "screenspace"

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you are right.


![image of screenSpacePattern](screenSpacePattern.png)

If the curve style is "sketch", "dashDot" or "screenSpaceDashDot", the curve must bind to a specific material. The property of the style, such as the cap shape or the scale of the dash dot pattern, will be set in the material via the surface input.
### Extents of the DashDotLines
Different from the other Curves, the extents of the DashDotLines is only the bound box of the control points. The width of the line will not be considered, because it is screen spaced, that it is implemented via the shader.
Copy link

Choose a reason for hiding this comment

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

This is conditional on the screenspacePattern attribute - it seems when screenspacePattern is false, then the line does have an extent in worldspace, no?

On the other hand, if screenspacePattern should /only/ be considered for rendering, and DashDotLines have no extent, then perhaps it should exclusively be on the shader/material or be a primvar (like start/endCapType, patternScale)?

Copy link
Contributor

@PierreWang PierreWang Apr 23, 2024

Choose a reason for hiding this comment

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

It is not conditional on the screenspacePattern attibute.

"The width of the line will not be considered, because it is screen spaced, that it is implemented via the shader." In this sentence I mean the width is screenspaced. The attribute screenspacePattern only control if the pattern is screenspaced. But no matter whether it is true or false, the width of the line will always be screenspaced.

The default extents for a Curve is the bound box of the control points, and then be expanded with the width of the Curve. But the width for DashDotLines is screen spaced, and it is implemented in the fragment shader, we don't include the width in the extents calculation.

So DashDotLines will always have an extents, which is just the bound box of the control points.

@PierreWang
Copy link
Contributor

The bigger question in my mind is still if this schema is needed - I totally understand the utility of it, but it's also yet another schema to support everywhere, in addition to the underlying existing curves...

As @nvmkuruc pointed, the dash-dot patterned lines will interpret the width as screen-space width. This is different from the definition of a common Curve. To avoid schema slicing, it is better to use a separate typed schema.

We will use a separate Typed schema DashDotPattern to save the pattern, instead to use a texture to save the pattern. A DashDotLines primitive will not bind a material. Instead, it can bind a DashDotPattern.
@erikaharrison-adsk erikaharrison-adsk force-pushed the adsk/feature/LineStyle branch 4 times, most recently from f240736 to 01b1f0a Compare June 5, 2024 02:33
@PierreWang
Copy link
Contributor

In the new change, we create a new schema DashDotPattern. The pattern can be set via an array of float2. So we don't need to use a texture to provide the pattern. And we don't require that the DashDotLines primitive must binds a material. Instead, if the DashDotLines primitive has a pattern, it should bind a DashDotPattern.

Comment on lines 62 to 70

The DashDotPattern has the following new properties:
- patternPeriod. A float uniform. It is the length of a pattern. By default it is zero. If there is no pattern, it should be zero.
- pattern. An array of float2. It saves the dash-dot pattern. For each float2, the x value and the y value must be zero or positive. The x value saves the offset of the start of current symbol, from the end of the previous symbol. If the current symbol is the first symbol, the offset is from the start of the pattern to the start of current symbol. The y value saves the length of the current symbol. If it is zero, the current symbol is a dot. If it is larger than zero, the current symbol is a dash. As a result, the total sum of all the x value and y value will be the length from the start of the pattern to the end of the last symbol. This sum must be smaller than patternPeriod.

For example, assume the pattern is [(0, 10), (1, 4), (3, 0)]. It means the first symbol is a dash which is from 0 to 10. The second symbol is a dash which is from 11 to 15, and the third symbol is a dot which is at position 18. There are gaps between 10 and 11, and between 15 and 18. If the patternPeriod is 20, there is also a gap between 18 and 20.

An API schema DashDotPatternAPI is provided to bind the DashDotPattern to a DashDotLines primitive.
Copy link
Member

Choose a reason for hiding this comment

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

Specifying the Pattern as a separate prim that must be bound via relationship seems like overkill here, and imposes more complexity/cost on the scenegraph and Hydra efficiency than a simpler, API-only solution would. UsdShadeMaterial must be bound because it consists of multiple prims, and because the binding mechanism itself needs to be very "rich" (i.e. complicated).

As an alternative, could we get rid of the DashDotPattern Typed schema, instead putting all of its properties into DashDotPatternAPI? Then, the simple case is very simple: all the information for a single DashDotLines is contained on the prim, very efficiently processed by Hydra. In the mentioned case of wanting to "bind" the same Pattern onto multiple DashDotLines primitives, this seems like a CSS-inspired dream case for the use of class inheritance in USD.

Now we can't build inheritance into the schema itself (composition doesn't consult the UsdSchemaRegistry), but we can provide "strong guidance" in the spec (and helper functions in the schema classes) that DashDotLines primitives should minimally inherit from a "_class_DashDotPattern", and potentially also more specific/granular classes tailored to the asset/scene. So, each of the DashDotLine primitives in the "Diagram1" asset might look something like:

def DashDotLines "TopArc" (
    prepend inherits = [</_class_Diagram1_DD_Pattern>, </_class_DashDotPattern>]
)
{
}

so then anyone referencing that asset could affect all of its DashDotLines (and no others) by putting opinions for DashDotPatternAPI in a spec for </_class_Diagram1_DD_Pattern> .

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you. I will modify the proposal. And also for the text proposal, I will also merge the TextStyle into TextStyleAPI, and the same for ParagraphStyle and ColumnStyle.

@PierreWang
Copy link
Contributor

PierreWang commented Jun 20, 2024

Hi @spiffmon , I have updated the proposal. The properties of DashDotPattern is now in DashDotPatternAPI. I don't know if my sample is right:

def DashDotLines "StyledPolyline" (
    inherits = [</Pattern>]
){
    uniform bool screenspacePattern = true
    uniform token startCapType = "triangle"
    uniform token endCapType = "triangle"
    uniform float patternScale = 11
    int[] curveVertexCounts = [3, 4]
    point3f[] points = [(0, 0, 0), (10, 10, 0), (10, 20, 0), (0, 30, 0), (-10, 40, 0), (-10, 50, 0), (0, 60, 0)]
    float[] widths = [10] (interpolation = "constant")
    color3f[] primvars:displayColor = [(0, 0, 1)]
}
def "Pattern" (
    prepend apiSchemas = ["DashDotPatternAPI"]
)
{
    uniform float patternPeriod = 10
    uniform float2[] pattern    = [(0, 5), (2.5, 0)]
}

In practice, I found that I could not read the patternPeriod and pattern property from the "StyledPolyline" prim in the adapter of DashDotLines. I think as I used inheritance, the properties should be available in the prim.

@spiffmon
Copy link
Member

Yes, inherits should make the API schema be present on </StyledPolyline>, and you should be able to confirm that in usdview or any python shell. I don't really know what might be the problem in your Hydra adapter, though I know that in Hydra 2 the general pattern would be to have a SI specifically for DashDotPatternAPI... others here might be able to help more, if you can provide a bit more context on the code?

@PierreWang
Copy link
Contributor

Yes, inherits should make the API schema be present on </StyledPolyline>, and you should be able to confirm that in usdview or any python shell. I don't really know what might be the problem in your Hydra adapter, though I know that in Hydra 2 the general pattern would be to have a SI specifically for DashDotPatternAPI... others here might be able to help more, if you can provide a bit more context on the code?

Thank you. I made a further check today and found a bug in my code. There is no problem now.

@PierreWang
Copy link
Contributor

The implementation of this proposal is here:
PixarAnimationStudios/OpenUSD#3151

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Update
Development

Successfully merging this pull request may close these issues.

5 participants