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

Sphere Bounding Box: Artifact for negative radius. #532

Closed
DeltgenDavid opened this issue May 13, 2020 · 11 comments
Closed

Sphere Bounding Box: Artifact for negative radius. #532

DeltgenDavid opened this issue May 13, 2020 · 11 comments
Assignees
Milestone

Comments

@DeltgenDavid
Copy link

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
Copy link
Collaborator

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.

@hollasch hollasch added this to the v3.2.0 milestone May 13, 2020
@DeltgenDavid
Copy link
Author

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
Copy link
Collaborator

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
Copy link
Collaborator

hollasch commented May 14, 2020

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]));

@hollasch hollasch reopened this May 14, 2020
@hollasch hollasch modified the milestones: v3.2.0, Future May 14, 2020
@DeltgenDavid
Copy link
Author

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Author

DeltgenDavid commented May 15, 2020

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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.

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

No branches or pull requests

3 participants