Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lgritz
Copy link
Collaborator

@lgritz lgritz commented May 7, 2018

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.

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));
Copy link
Contributor

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?

Copy link
Collaborator Author

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".

Copy link
Contributor

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));
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants