RayTracing / raytracing.github.io

Main Web Site (Online Books)
https://raytracing.github.io/
Creative Commons Zero v1.0 Universal
8.78k stars 866 forks source link

Sphere Bounding Box: Artifact for negative radius. #532

Closed DeltgenDavid closed 3 years ago

DeltgenDavid commented 4 years ago

Bounding box calculation for spheres should take the absolute value of the radius in order to take into account "bubble" like dielectrics where sphere with negative radius is placed inside another one.

hollasch commented 4 years ago

Makes sense. I don't know if negative radii will cause other issues as well, though. Let us know what happens in your own code.

DeltgenDavid commented 4 years ago

Thank you. So far, except for the bounding box, negative radii do not have any other visible side effects. Code behaves as expected even though it should only be used in the context of creating thickness for dielectrics sharing the same index.

hollasch commented 4 years ago

Looking at the code, the bounding box method is

output_box = aabb(
    center - vec3(radius, radius, radius),
    center + vec3(radius, radius, radius));   

Looks like this should handle a negative radius just fine.

hollasch commented 4 years ago

Wait, I think the better fix here is to handle this in aabb. The constructor should really look like this:

_min = point3(fmin(a[0],b[0]), fmin(a[1],b[1]), fmin(a[2],b[2]));
_max = point3(fmax(a[0],b[0]), fmax(a[1],b[1]), fmax(a[2],b[2]));
DeltgenDavid commented 4 years ago

Indeed you are right. It will take all geometries into account that way. I did not think about that, i still only manage spheres in my implementation.

hollasch commented 4 years ago

I should also point out that the better approach to this (nested materials) is to implement a refracted index stack, assuming that shapes fully nest each other. That would be a fun little appendix project.

hollasch commented 4 years ago

Oh, and while I'm here, I neglected to point out that my proposed code above is due to the fact that the aabb constructor assumes that the first point contains all of the minimum values, and the second point contains all of the maximums. My update would have that enforced at construction, so any two extreme points would suffice (they'd just be considered arbitrary opposite corners of an axis-aligned box).

DeltgenDavid commented 4 years ago

I should also point out that the better approach to this (nested materials) is to implement a refracted index stack, assuming that shapes fully nest each other. That would be a fun little appendix project.

Can be fun yes, or having a stack of "thickness" values associated with different refractive index directly inside the sphere object (if all bubbles are concentric).

trevordblack commented 4 years ago

I do a rather thorough investigation of this when porting the code to Optix (RTX), and found that negative radii had no visible effect except for BVH construction.

hollasch commented 3 years ago

There's also the question here about whether negative radii (a geometric attribute) should be used as a hack to invert the index of refraction (a material attribute). Instead of creating a glass (inside air) sphere turned inside out, the model should be that of an air inside glass sphere, by supplying the dielectric constructor with an inverted index of refraction.

Going with this approach, the change could (should) be as simple as explaining how to model a bubble in a glass sphere in the text.

hollasch commented 3 years ago

I believe that this is fixed now with the latest change to treat aabb constructor parameters as any two extreme points, rather than specifically the point of all minimums and the point of all maximums. Please re-open if this is not true.