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

Determination of normal direction in geometry hit function #270

Closed
trevordblack opened this issue Nov 30, 2019 · 16 comments
Closed

Determination of normal direction in geometry hit function #270

trevordblack opened this issue Nov 30, 2019 · 16 comments
Assignees
Milestone

Comments

@trevordblack
Copy link
Collaborator

On an unrelated note - I also agree that the determination of the normal direction should probably happen inside sphere:hit.

Originally posted by @vchizhov in #137 (comment)

@trevordblack
Copy link
Collaborator Author

We have a code philosophy question at hand here. At present we have an implicit determination of inside/outside of geometric primitives within the material scatter function.

  1. Does it make sense to make this determination explicit?
  • e.g. metal, diffuse, and dielectric have to explicitly account for interiority of rays?
  1. Should this interiority check moved into the geometric primitive?

@vchizhov
Copy link

vchizhov commented Dec 1, 2019

As far as I am aware there is no determination in the code - if you start rays on the "inside" of an object it will produce wrong results. As an example imagine enclosing your camera inside a sphere, putting a light source inside - then your render will be black, even though it should not be. I think the only determination happens for dielectrics (since you cannot get away with not doing it). If anything doing the same thing for other materials will be only consistent.

As for the check - technically it could happen anywhere where you have access to the intersection ray direction and the normal, but as you pointed out in the previous thread it makes sense to have it happen inside the primitive. Having it there is useful, since this is the very first time where you produce the normal - so if you return the correct facing normal the moment you compute it, you don't have to be worried and reason about anything down the line. The code is as simple as:
normal = dot(normal, ray.dir) < 0 ? normal : -normal;
For performance reasons the correct dot may be kept in the intersection structure, but such a micro-optimization is probably overkill for an introductory ray tracer.
An added benefit is that for dielectrics, you will always get the correct facing normal so your logic will not differ much from the other materials.

TL;DR: Moving the computation of where the normal is facing to the geometry, saves you from logic leaking outside of where the normal is actually computed. It's easier to reason about, prevents bugs, and makes sense.

@hollasch hollasch added this to the v3 milestone Dec 2, 2019
@hollasch
Copy link
Collaborator

hollasch commented Dec 3, 2019

There are arguments on both sides (ha! See what I did there?). Some surfaces may have distinct front/back sides (say, blue on the outside, red on the inside). Others have no "inside" (think of a triangle). Some objects have an implicit inside, with respect to index of refraction (glass inside, whatever else outside).

Does material know about inside vs. outside? It's up to the object to know if it has an inside, right? A sphere has an inside, a torus has an inside, a flat quad may or may not have an inside (it can present itself as an infinitely thin box).

Of course, we've given the reader enough for them to play with variants, and we should just go with simplicity where it doesn't matter.

@vchizhov
Copy link

vchizhov commented Dec 3, 2019

I disagree. It seems we are conflating several uses of the normal here. I am not aware of a reasonable scenario, where you want to return the "incorrect" normal, since you should use the "correct" normal for both shading and scattering (if you are aware of such a scenario please inform me of it). If you want to distinguish between a front and back face, this can be easily achieved by adding a bool indicating the "sidedness" to the intersection structure (which would be filled out once again in the intersection with the geometry routine) without coupling this to the normal. This effectively decouples the material from the geometry (while still allowing one to query front/back face).

I'd argue that keeping the correct normal is both simpler and more consistent, especially when comparing dielectrics to other materials. This is obviously a subjective opinion, I just cannot find good enough arguments for the opposite.

@trevordblack
Copy link
Collaborator Author

trevordblack commented Dec 3, 2019 via email

@vchizhov
Copy link

vchizhov commented Dec 3, 2019

The front/back notion will be useful if you want to texture the two sides differently as hollasch mentioned. Considering that this isn't used in the book (and that it can be also achieved by duplicating and scaling the geometry), I am not sure whether adding it is necessary. We need the bool for the ior vs 1/ior choice in the dielectric.

Also as an added benefit the correct normal will be consistent with most (all) rendering equation formulations the readers will find (Kajiya, Veach, Dutre, Lessig).

@hollasch
Copy link
Collaborator

hollasch commented Dec 3, 2019

I think Vassillen makes a good argument. So, would this work?

  • Hit record has new value, Boolean inside property, true if the geometry has a concept of inside and the intersection was from the inside.
  • The hit record always returns the normal such that N⋅Ro < 0.

Note that we should avoid saying "same side", because a ray outside a sphere needs the outward normal for the front-face intersection, and the inward normal for the back-face intersection.

Any objections?

@vchizhov
Copy link

vchizhov commented Dec 3, 2019

I would recommend having the bool be something like front_face/front_facing or something along those lines. For instance triangles do not have an "inside" but they can have an orientation defined through the order in which the vertices were given. It's not so much that I want to emphasize the naming, but rather that even if a notion of inside is ill-defined, it should still probably produce a reasonable bool for the two sides - for example if you want to render differently based on the side, as you mentioned previously. It's also consistent with the surfaces that have an inside.

And ultimately you need it for refraction. If you have a triangle mesh that has a dielectric material, you need this bool, even if triangles have no inside, but you just rely on the user to provide correctly oriented meshes with no holes (also makes the refraction code more readable I guess).

Just note that this change will induce a change in the dielectric code.

@hollasch
Copy link
Collaborator

hollasch commented Dec 3, 2019

front?

@vchizhov
Copy link

vchizhov commented Dec 3, 2019

Whatever would make the most sense for people unfamiliar with graphics, I am not good with naming. Obviously this should be explained in more details in the text, and in comments.

@trevordblack
Copy link
Collaborator Author

trevordblack commented Dec 4, 2019

It might be valuable for us to explicitly declare the inside of Metal and Diffuse as (0.0, 0.0, 0.0) black. While not terribly useful it sets a good precedent for the user to take notice of the front/front_face/inside bool, is physically sound (to a first degree), and removes that aspect of "leaking" state

There's a possibility that this fix will remove the ability to make bubbles.

@vchizhov
Copy link

vchizhov commented Dec 4, 2019

There should be no issues with bubbles I think, the only difference is, that rather than give the second sphere a negative radius, you will just give it a reciprocal index of refraction (which makes more sense physically).

I don't think that extra definitions for color are required, that should rather be left as an exercise to the reader or for the next books. Anyways with spheres only you cannot see it without a light source (that is not the background). So this will be mostly useful if a reader introduces a different primitive (plane for example), has an emissive sphere inside a sphere, or introduces new light sources. I advise against defining extra colors just with regards to keeping the complexity low.

@trevordblack
Copy link
Collaborator Author

trevordblack commented Dec 15, 2019

Question:

I want to rewrite the refract function to return a vec3, instead of a bool.

At present, the code calls refract and refract returns whether or not the ray should be refracted, and if it should, a vec3 parameter.

I believe that the logic for whether a ray is refracted should remain entirely within dielectric::scatter and refract only refracts the ray.

This is the preexisting pattern for reflect, and I've always thought that having a different pattern for refract was confusing. It would certainly simplify understanding of the Dielectric class.

Which, as far as I can tell, is the cause for most problems by new users in the first book

@vchizhov
Copy link

vchizhov commented Dec 15, 2019

In that case it will be consistent with glsl for example: https://www.khronos.org/registry/OpenGL-Refpages/gl4/html/refract.xhtml

On the other hand it is less explicit. I don't think the confusion arises so much from this specific part, as from the lack of details about refraction in general. The text just skips over the details, effectively just presenting the code and relying on the reader to "trust it".

I usually refer people to: https://graphics.stanford.edu/courses/cs148-10-summer/docs/2006--degreve--reflection_refraction.pdf
for more details. I realize that it is problematic to add maybe what would be considered too advanced mathematics in the text, but I think the whole confusion stems exactly from this. That is not to say that I have a solution to this problem.

@trevordblack
Copy link
Collaborator Author

Hmm.
That paper is Ace.

I'm going to rewrite the Dielectric chapter the way that makes sense in my head, I'll have a PR and we an discuss it holistically.

@trevordblack
Copy link
Collaborator Author

Changes ported to PR #312

Provide FB over there.

hollasch added a commit that referenced this issue May 17, 2020
With the changes made to handle surface intersection from either side
(refer to #270), we no longer have need for the `flip_face` class. This
change removes the class and the associated text in the books.

Resolves #482
Resolves #588
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

No branches or pull requests

3 participants