PixarAnimationStudios / OpenUSD-proposals

Share and collaborate on proposals for the advancement of USD
106 stars 27 forks source link

Autodesk: Add LineStyle proposal #20

Closed erikaharrison-adsk closed 11 months ago

erikaharrison-adsk commented 1 year ago

Description of Proposal

This is a proposal on supporting line styles within OpenUSD.

Supporting Materials

N/A

Contributing

meshula commented 1 year ago

Could you revise the commit to include the signing requirement, then force push your branch? That should unblock the Merging workflow flag.

erikaharrison-adsk commented 12 months ago

@meshula - sorry for the delay, I didn't notice it was approved. Do we go ahead and merge it in ourselves? I know with the USD repo, we don't have permissions to do so, and here only those with write access can merge pull requests.

Please advise on next steps for this work, thanks!

asluk commented 12 months ago

These are all great features; thanks! I'd be interested in opening a discussion around the tradeoffs of having all of these properties live directly on the BasisCurves schema. Would it be worth considering other approaches like putting screen space and styling properties into API schemas? My concern is that the basis curves schema is already a bit muddled in its domain usage, and carving up its domain usages across API schemas as mixins might promote clarity around its role in a given scene? Thanks for considering!

spiffmon commented 12 months ago

These are all great features; thanks! I'd be interested in opening a discussion around the tradeoffs of having all of these properties live directly on the BasisCurves schema. Would it be worth considering other approaches like putting screen space and styling properties into API schemas? My concern is that the basis curves schema is already a bit muddled in its domain usage, and carving up its domain usages across API schemas as mixins might promote clarity around its role in a given scene? Thanks for considering!

+1 to everything @asluk said. Factoring these concerns into an applied schema will simplify the proposal, because there will no longer be a need for a "none" value of style - style is only considered when the schema is present on a Curves primitive. Plus, that opens the door to applying the styling to NurbsCurves, as well as any other curve primitives that may come along.

I also think it will really strengthen the proposal to review and consider/contrast related art and use-cases. For example, we already know there is interest in encoding Blender's Grease Pencil drawings in USD. What about Tiltbrush? And I also believe Adobe is interested in line-art.

It's great that you discuss the Hydra screenspace-related primvars, and I would love to see part of this proposal be either appropriately formalizing them in USD, or declaring they are no longer needed.

This leads to another question: should "line styling" and "screen-space rendering" be formalized as separate concerns? Thinking about Tiltbrush or wanting to put vector drawing on the side of a 3D building... if rich styling of lines only makes sense for 2D lines, let's state that, but even if so, it need not be limited to screen-space, but to any surface. Does that argue for a 2D styled curve encoding, or are styled 3D curves still a needed thing?

Last thing is about the longer-term vision for styling: the proposal states the current implementation only supports linear curves. Would you plan to extend it to handle all the supported bases? This proposal is a great treatment of some needed features, but it would really helpful to discuss, in the ways described above, how it fits into the future-looking needs of the USD ecosystem.

Thank you!

meshula commented 12 months ago

Hi @erikaharrison-adsk "approving" is a github workflow term in this case. We don't approve things for implementation into OpenUsd here! It's github-workflow-approved in order that we can land the proposal into this repo, to make it easier for others to help work on it.

Without approve and merge, if others want to work on it, they need to fork YOUR repo, they would make requests onto it, and then you'd have to land on your repo, and propagate those changes here.

So, if we want, we can merge the proposal as we have done for other proposals, in order to facilitate collaboration...

erikaharrison-adsk commented 11 months ago

Thanks for the comments here - sorry for the delay responding. We'll follow up on the responses from @asluk and @spiffmon shortly.

@meshula re: "merge the proposal to facilitate collaboration" -- ??? I'm sorry, I'm not following. I thought that this repo was about discussing features on the theoretical side (and practical if implementation is visible) via comments in PRs, and then once "approved" to be going ahead with getting the implementation out to OpenUSD. Is this not how it works? I'm not sure how approve & merge in this proposal repo impacts collaborations -- do you mean for others to be making changes to this branch and proposal files? Given there's feedback above, I'd assume we'll continue to update this proposal before merging this branch in. Thanks for your patience in helping me understand this better.

meshula commented 11 months ago

@erikaharrison-adsk Please refer to this section on the front page https://github.com/PixarAnimationStudios/OpenUSD-proposals/tree/main#relationship-to-other-forums which tries to lay out that this forum is for discussion. In this section https://github.com/PixarAnimationStudios/OpenUSD-proposals/tree/main#discuss-the-proposal we try to explain that this is a discussion and collaboration forum for proposals, but doesn't make any suggestion that an accepted proposal is also planned for implementation in OpenUSD.

Could you take a look over here? https://github.com/PixarAnimationStudios/OpenUSD-proposals/blob/main/proposals/Readme.md

That table shows the status of the proposals. As you can see the statuses are "Implemented", "Published", and "Draft".

LineStyles is not in the table! If we approve the PR here, we are specifically approving it for inclusion in that table, and assigning it a status.

"Implemented" means that it was implemented in OpenUSD itself.

"Published" means that it has achieved stability, and further work on the proposal is not expected. It does not mean that it will be implemented into OpenUSD, and also, it does not mean that will not be implemented. It just means that everyone has agreed that they think they've said everything they want to say about it. Presumably, interested parties are working on them, and will send them to the OpenUSD repo as a PR when they think they they are ready.

"Draft" means the proposal is in its working stage.

So an approval here on this page means, the proposal will appear on that page with a status. If everyone has agreed "hey LineStyle is ready to implement, there's nothing else to say" it would bear a "Published Status", and if there is ongoing conversation it would be marked as "Draft", meaning that there is still ongoing conversation, and cooperation and feedback is requested and encouraged.

As long as your proposal is here, as a PR, we can all comment, and you can update your PR as you wish. At the point you are ready to others proposing their own PRs on the proposal (as opposed to updating this PR yourself), then we can hit the Merge Pull Request button, and then everyone can collaborate directly on the button. We can't hit the Merge Pull request without also clicking the "github Approve" button, it's regrettable that that one word is so overloaded with unrelated meanings.

So... my hitting approval meant that I think we're ready to move to the "community collaboration" phase, and we'd move the status in the table to "Draft" or "Published" depending on consensus.

I hope this straightens things up. If it would help to improve the wording on the landing page to make it more understandable, suggestions are welcome.

PierreWang commented 11 months ago

@asluk @spiffmon Thank you for your advice. The design was written by me and Erika helped put the design into github. Yes, in our general design we hoped that the style could be applied to any curve. But in the first implementation we only considered our usage, so it is only applied to lines. And we used the material network as a help to input the line style texture into the shader. We can move the line style into API schemas. And there are still two issues I am considering. First we sample in the line style texture to decide the position of a fragment in the style. Is that the material network the only way to enable texture sampling in shader? It will be better to put the line style texture in the API schema. And even better if we can create line style texture on the fly. The second issue is that the current algorithm requires pre-calculate the length of the curve on the screen (in every frame). So if we want to extend it to any curve, I need to find a way to calculate the length of a parameterized curve fast.

I think screen space line style and screen space lines are different and unrelated. A line is screen-spaced means that when camera changes, the line keeps its size, or keeps its direction on the screen. And a line style is screen-spaced means that, when camera changes, the line can change, but the line width will not change, and dash-dot pattern on the screen will not change. For example, each dash is still 10 pixels on the screen, even if the line itself rotates or extends. So a screen-spaced line style can be applied to non screen-spaced lines. And on the other hand, if the line has screen space attribute such as camera facing, the style can be non screen-spaced. The line could be wider when we zoom in. Maybe screen-space is not accurate for a line style. It is better to use "keepWidth" and "keepPatternLength" to describe how the pattern changes when camera changes.

erikaharrison-adsk commented 11 months ago

@meshula -- sorry to be a bother, but based on your comment, and that you've approved this, should we now be listed as Draft on the readme.md page? Or that only happens once this PR is merged?

Or does this PR need to be updated to include the row in the Readme.md page?

meshula commented 11 months ago

@erikaharrison-adsk The listing on on the readme.md page gets updated when merged. You can add the row in the PR if you like, or we can do it for you upon merging. Just let us know when you'd like to merge :)

erikaharrison-adsk commented 11 months ago

Thanks @meshula -- we're good to merge this proposal in. @PierreWang will follow up in the new year with edits for this proposal based on the comments above.

meshula commented 11 months ago

Awesome :D

erikaharrison-adsk commented 9 months ago

@meshula (not sure how else to connect/tag you) - I'm not sure exactly what the next steps are at this stage. From our end, we're continuing to work on implementation and will get out a PR to OpenUSD in the coming weeks. From the proposal perspective, please advise what next we need to do, if there are specific things we ought to do (eg. from Draft to Published?).

meshula commented 9 months ago

Hi Erika, are you on the ASWF Slack? The easiest thing would be to connect there via a DM, and we can get a more direct conversation going. https://join.slack.com/t/academysoftwarefdn/shared_invite/zt-2at1248ps-UzGbBrikPTGS6TsODfYphg