-
Notifications
You must be signed in to change notification settings - Fork 359
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
OSL built-in displace() and bump() were documented but never implemented #887
base: main
Are you sure you want to change the base?
Conversation
Add them now. Also, an additional variety that takes a vector offset in a non-common space. There's no good reason why these were never implemented. I think that people just used the `P += ...; N = calculatenormal(...)` idiom directly. But they were always documented in the OSL Language Spec, so I see no reason not to add them now.
displace (amp * N); | ||
} | ||
void displace (string space, float amp) { | ||
float len = length (transform (space, N)); |
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.
Does this do the right thing if the scale is not uniform?
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. But it might depend on what you think is "right".
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.
:) Yeah... I've been using this idiom forever. It makes intuitive sense, and it does give the user some form of non-uniform control over the displacement magnitude. However, lately I've switched to using the cube-root of abs(determinant(matrix(space,"common"))) as the scale factor. This change of heart is due to my experience using non-uniformly scaled coordinate systems to control displacement. With the simple length(transform(N)) method, the results are never quite what I expect or desire, and so the presumed control it allows doesn't, in my recent opinion, live up to the expectation. As a result, I've concluded that preserving a volumetric sense of relative scale is more useful in the rare instance when a user must use a non-uniformly scaled space to control displacement magnitude. Since displacement can be viewed as a volumetric operation, I now see this as making more intuitive sense. But, all that said, the simple approach works perfectly in the case of uniformly scaled coordinates, and is cheaper to compute. And since that's by far the most likely scenario, it may be the right implementation choice.
} | ||
|
||
void bump (vector offset) { | ||
N = normalize (calculatenormal (P+offset)); |
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.
Isn't it discouraged to change N
in a surface shader? Most closures take a normal as an argument.
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 I would prefer for bump
and displace
to return a normal
and point
respectively, and let the shader writer manage the assignment to global variables.
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.
The idea is that you should call one of these in the "root" shader, which is passed in the accumulated offset or amount.
If you're randomly calling it in mid-graph nodes, perhaps multiple times, you're probably going to do weird things.
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.
Also, the idea was, some day (or in some renderers) we might want displace() to have additional side effects. For example, maybe it modifies dPdu, dPdv? Or do other renderer-specific things? I wanted a function that does the "assignment to global variables" part, which may someday grow to have per-renderer trickiness. The "accumulate and return total" part is already trivial.
Add them now. Also, an additional variety that takes a vector offset in
a non-common space.
There's no good reason why these were never implemented. I think that
people just used the
P += ...; N = calculatenormal(...)
idiomdirectly. But they were always documented in the OSL Language Spec, so I
see no reason not to add them now.