AcademySoftwareFoundation / OpenShadingLanguage

Advanced shading language for production GI renderers
BSD 3-Clause "New" or "Revised" License
2.07k stars 350 forks source link

Docs: Improve documentation of `trace` function #1671

Closed AidanWelch closed 1 year ago

AidanWelch commented 1 year ago

Description

Improve the documentation of optional parameters of trace to more accurately describe behavior.

Tests

N/A

Checklist:

lgritz commented 1 year ago

Thanks, I really appreciate help in making the documentation more clear.

Before we go with this wording and format, please look at the explanation in texture() (circa line 4086 of languagespec.tex), it is yet another way to explain how the optional token/value pairs work, and I think maybe the way the parameters are shown directly in pairs makes it more clear. So as you look at it, consider (a) regardless of which style of explanation we ultimately prefer, perhaps it's best if all the places we are talking about optional token/value argument lists should be explained with the same style and wording? and (b) is the texture presentation more clear and we should adopt it here, or should we change that in some way, too?

AidanWelch commented 1 year ago

Before we go with this wording and format, please look at the explanation in texture() (circa line 4086 of languagespec.tex), it is yet another way to explain how the optional token/value pairs work, and I think maybe the way the parameters are shown directly in pairs makes it more clear. So as you look at it, consider (a) regardless of which style of explanation we ultimately prefer, perhaps it's best if all the places we are talking about optional token/value argument lists should be explained with the same style and wording? and (b) is the texture presentation more clear and we should adopt it here, or should we change that in some way, too?

I agree, the definition of those pairs is more clear. But, I don't think it is that clear without prior knowledge of calling the function with those pairs how to actually pass them, for example I don't think the correct syntax is any more intuitive than:

trace(vec, dir, mindist=0.5);

or passing a struct- based on what is described anywhere in the existing spec.

I wonder if maybe this should be clarified in the spec as the way to pass optional params, or if it should just be clarified for these two functions, and left up to implementations/users how they pass them for other functions.

lgritz commented 1 year ago

Yes, it should be clarified in the spec with an example. Glancing at the document, I think that in section 6.4, which describes the syntax of function declarations and calls, is the right place. I would recommend:

  1. Swapping the current order of 6.4.1 (function calls) and 6.4.2 (function definitions) so that explaining the definition comes first (since in a shader, it also comes first).

  2. Then either at the end of the "function calls" (repositioned as 6.4.2) part, or with a new part 6.4.3/optional arguments, we can explain how optional arguments work, including an example. Then all the built-in arguments can refer to this, with language similar to:

Following the required arguments, the texture() call can take optional
arguments in the form of token/value pairs (as explained in section
\ref{sec:syntax:functioncalls}), which may include any of the following:

    "blur", <float>
         blah blah
    "width", <float>
         blah blah

And of course trace can make a similar reference, and structure the list of token/value pairs similarly (whereas now it doesn't quite mirror the same structure, but it should).

By the way, this whole optional argument passing scheme was inherited from the way people were used to it working in RenderMan Shading Language, and one thing that's always bugged me is that it's not possible to write a function definition in OSL itself that allows for optional arguments in this manner (or defaulted arguments). I think it would be a great thing to extend the OSL syntax to allow both for defaulted arguments as well as a more modern syntax such as

C = texture(texturename, s, t, width: 3, blur: 0.1);

The alternative syntax width = 3 also is attractive, but I slightly fear how it might interact with the fact that (inherited from C) "foo = bar" is considered an expression, so if there is a local variable called foo as well as a parameter named foo, there is some semantic ambiguity about whether it change the value of the local variable, which isn't a problem if we used : for passed parameter naming.

But I digress. Right now, we just want to clarify the docs about what OSL does. Later, I would like to return to adding this feature somehow to the language syntax, I think it will make shader writers happy.

AidanWelch commented 1 year ago

but I slightly fear how it might interact with the fact that (inherited from C) "foo = bar" is considered an expression, so if there is a local variable called foo as well as a parameter named foo, there is some semantic ambiguity about whether it change the value of the local variable, which isn't a problem if we used : for passed parameter naming.

I agree completely, and prefer : syntax for clarity- I was just giving an example from Python

AidanWelch commented 1 year ago

Okay, I've implemented the changes suggested. Also, I think clarification on what the shade argument does should be added, but I don't know myself so I can't add that.

AidanWelch commented 1 year ago

Also, I would've liked to link directly the normalize function instead of the geom section, but I don't know if that's possible