-
Notifications
You must be signed in to change notification settings - Fork 356
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
Docs: Improve documentation of trace
function
#1671
Conversation
Signed-off-by: Aidan Welch <aidan@freedwave.com>
src/doc/languagespec.tex
Outdated
\end{code} | ||
|
||
Furthermore, \qkw{mindist} and \qkw{maxdist} should be viewed as scalars, | ||
which multiplied by the magnitude of the direction vector is the length of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's is true at least for implementation in Blender, and it led to weird behavior, that's how I discovered it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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..."
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 |
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:
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. |
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:
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
The alternative syntax 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. |
I agree completely, and prefer : syntax for clarity- I was just giving an example from Python |
Signed-off-by: Aidan Welch <aidan@freedwave.com>
Okay, I've implemented the changes suggested. Also, I think clarification on what the |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the addition
…ation#1671) Improve the documentation of optional parameters of trace to more accurately describe behavior. Signed-off-by: Aidan Welch <aidan@freedwave.com>
Description
Improve the documentation of optional parameters of
trace
to more accurately describe behavior.Tests
N/A
Checklist: