Docs: Improve documentation of trace function

[AidanWelch]

Description

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

Tests

N/A

Checklist:

• I have read the contribution guidelines.
• I have previously submitted a Contributor License Agreement.
• I have updated the documentation, if applicable.
• [N/A] I have ensured that the change is tested somewhere in the testsuite (adding new test cases if necessary).
• My code follows the prevailing code style of this project.

[lgritz]

Is that true? If dir is not unit length, does that change the meaning of mindist and maxdist?

Or is dir required to be normalized?

Or is dir automatically normalized internally to the trace call?

[AidanWelch]

It's is true at least for implementation in Blender, and it led to weird behavior, that's how I discovered it.

[lgritz]

I agree that the spec does not spell it out, and should say more fully what the behavior should be.

Personally, I think that it's really confusing for the units of min/max to depend on whether or not the direction is normalized. My preference is to fix it in Blender rather than enshrine that odd behavior in the OSL spec itself. I think it's a much simpler explanation to say that min/max are in the canonical units of the scene. (and/or, that trace expects the dir argument to be unit length)

[sfriedmapixar]

This behavior is going to end up being entirely under the control of the renderer integration and is largely out of the hands of this code-base, so this is more of a "do we want to suggest a uniform policy across all renderers" or "do we want to leave it up to the renderer." The code interface that the renderer needs to implement uses Vec3 with no suggestion that they are normalized, and nothing in the current code JIT or calls will do any enforcement of normalization.

So with that, using the simplest math of P + mindist/maxdist * dir is likely what the first implementation anybody makes will do. Irregardless of whether dir starts out normalized or not, if the renderer has to transform dir to a different space, then the renderer needs to be mindful of the scale factor of that transform and apply it to the mindist or maxdist anyway, so we're not increasing any burden there.

It seems if we are to pick a policy to codify in the documentation, this is the most straightforward and least burdensome to implement from a renderer perspective...however the point of a domain specific language is generally to make it the least burdensome on the person developing code in that language. How surprising do shader authors find this kind of behavior?

FWIW, RenderMan treats the argument this way.

[sfriedmapixar]

I think Larry an my comments crossed paths -- Larry, there are different spaces, and the same OSL shader may run in different spaces. By "cannonical units of the scene" -- do you mean "world" space? Or are you saying "dir" must be normalized in "common" space... i.e. the shader must call normalize on dir right before trace()...or do you mean scene units, which would be "world" space for our renderer at least. If someone computes the ray direction in a convenient space for that, say the space defined by an orthonormal basis for the shaded point, and then transforms that to "current" it'll be non-normalized reasonably often in the trace() call, same for if it is normalized in "world" space depending on the renderer's "current" space.

I suppose then "most convenient" for setting min/maxdist will depend on if they wanted those in "world" units or if as aribitrarily set shader parameters, or of there is some geometric bounding box or something that the shader is trying to compute an intersection with and set them up that way. "camera" space is often convenient for setting up maxdist on depth-probe type rays.

[AidanWelch]

I think it would make sense to describe how its currently implemented, even if just noting that that is how it is done in some implementations- because a lot of implementations don't document this and just point to these specs. It also as its a standard function I think it should have somewhat portable behavior where it isn't too difficult to implement. Furthermore, if the behavior isn't described to a usable or implementable state what is the point in having a spec at all?

The issue comes in that I think intuitively as I discovered this writing shaders, I assumed that it was referring to world space, furthermore I think world space is more useful than some scalar, and I'm not entirely sure what scalar units would be useful for. But, to describe that would be breaking, and not match existing implementations. I wonder if what would be most clear is just stating that the direction vector should be normalized before passing it- or at least noting that there can be unexpected behavior if the direction vector isn't normalized.

[lgritz]

@sfriedmapixar If you pass normalize(Raydir) into the function, then obviously it is normalized in "common" space units, which is renderer-defined, but is the coordinate system of everything as far as OSL is concerned. I would (as a shader writer, I guess) assume that the Occam's Razor answer is that mindist and maxdist are also in those units.

The original idea was that not all renderers want to operate in the same space (historically, some have preferred camera and others world), but that it was probably a safe assumption that the renderer's geometric operations (like ray tracing) and shading would tend to be the same coordinate system.

Maybe the best route is not to over-prescribe renderer behavior, but rather to just explain. Something like:

It is up to the renderer whether the direction is subsequently normalized and the min/max distances are therefore units of "common" space, or whether distances involved in the trace operation are all proportional to the length of the vector provided. Therefore, the best way to write shaders that are guaranteed to be portable across different renderers is to ensure that you pass a unit-length ray direction and then you can assume that the mindist and maxdist are also referring to "common" space units.

If a renderer wants to allow the length of the vector to be semantically meaningful and/or to implicitly change the meanings of other arguments, it's up to them to warn their users of this unobvious behavior. (I suppose that this is already the status quo, except that Blender does this perhaps without spelling out the caveat.)

[lgritz]

If a renderer wants to allow the length of the vector to be semantically meaningful

what I really mean here is also "If a renderer wants to optimize by not checking nor taking into account that the vector may not be normalized..."

[lgritz]

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]

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]

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]

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]

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]

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

[lgritz]

LGTM, thanks for the addition