Skip to content

No negative radius spheres #1428

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

Closed
wants to merge 4 commits into from
Closed

No negative radius spheres #1428

wants to merge 4 commits into from

Conversation

hollasch
Copy link
Collaborator

@hollasch hollasch commented Mar 11, 2024

[This change is stacked on top of the outstanding PR to normalize constructor parameter names. Ignore that commit, as I'll rebase this onto dev when that PR is merged.]

Implement bubbles with IOR, not negative radii

We were using negative radii on spheres to invert the sense of front vs
back faces of spheres, and using this to get an inverted index of
refraction (IOR) for bubbles. This method was used to model a hollow
glass sphere.

Over the past couple of years we've encountered a number of bugs induced
by negative radii spheres (see the list in the description for issue
1420).

Instead, this change interprets the dielectric parameter not as an IOR
relative to surrounding air, but instead as the ratio of the IOR of the
object material over the IOR of the surrounding medium. Interpreting the
parameter this way lets you model a sphere of air enclosed in glass --
just what we need to model a hollow glass sphere purely with the proper
IORs, and without resorting to inverted geometry with negative radii
spheres.

We also end up using this method to properly demonstrate total
internal/external reflection in another section.

Resolves #1420

In order to avoid confusion between constructor parameter names and
member variables, we either used alternate names for the parameters or
prefixed the parameter names with an underscore. For example:

    class foo {
      public:
        foo(int _i, bool semaphore, short bar) : i(_i), flag(semaphore)
        {
            baz = bar;
        }
       private:
         int i;
         bool flag;
         short baz;
    };

However, this is unnecessary. Constructor initializer lists are
unambiguous, as the assigned item always refers to the class member, and
the assigned value is always from the parameter (unless prefixed with
"this->").

Inside the constructor body, parameter names override member variables,
and "this->" can be used to indicate the class member in the case of
name collision.

Thus, the above code can be rewritten as this:

    class foo {
      public:
        foo(int i, bool flag, short baz) : i(i), flag(flag)
        {
            this->baz = baz;
        }
       private:
         int i;
         bool flag;
         short baz;
    };

I've taken advantage of this to clarify names used in constructors
throughout the codebase. Mostly for constructor parameter names,
sometimes for member variable names.
We were using negative radii on spheres to invert the sense of front vs
back faces of spheres, and using this to get an inverted index of
refraction (IOR) for bubbles. This method was used to model a hollow
glass sphere.

Over the past couple of years we've encountered a number of bugs induced
by negative radii spheres (see the list in the description for issue
1420).

Instead, this change interprets the dielectric parameter not as an IOR
relative to surrounding air, but instead as the ratio of the IOR of the
object material over the IOR of the surrounding medium. Interpreting the
parameter this way lets you model a sphere of air enclosed in glass --
just what we need to model a hollow glass sphere purely with the proper
IORs, and without resorting to inverted geometry with negative radii
spheres.

We also end up using this method to properly demonstrate total
internal/external reflection in another section.

Resolves #1420
@hollasch hollasch added this to the v4.0.0-alpha.2 milestone Mar 11, 2024
@hollasch hollasch requested a review from a team March 11, 2024 08:13
@hollasch hollasch self-assigned this Mar 11, 2024
Copy link
Contributor

@armansito armansito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like this change. Approved with some optional suggestions.

However, properly handling negative radii can be tricky. Recall the line from `sphere::hit()` in
listing [sphere-material] that calculates the outward normal:
The outer sphere is just modeled with a standard glass sphere, with an index of refraction of around
1.50. The inner sphere is a bit different, because _its_ index of refraction will now be relative to
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some optional small suggestions for this paragraph in bold:

The outer sphere is just modeled with a standard glass sphere, with an index of refraction of around 1.50 relative to the surrounding air. The inner sphere is a bit different because its index of refraction should be relative to the surrounding glass outer sphere and model a transition back to air. This is actually simple ...

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'll tweak the wording.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, I barely tweaked it:

The outer sphere is just modeled with a standard glass sphere, with an index of refraction of around
1.50. The inner sphere is a bit different, because its index of refraction should be relative to
the "glass atmosphere" of the surrounding outer sphere. This is actually simple to specify, as our

I don't want to add "relative to the surrounding air" since it's really a surrounding vacuum, (air = 1.000293), and most refractive indices are given with an implicit understanding that they're already relative to vacuum. Also, for the second change it's actually transitioning back into glass.

Either way, I'm going to add a bit more explanation earlier where we introduce refraction.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, vacuum is more correct. I only suggested air because it was used elsewhere in the book. My main point was to highlight that the refractive index is always relative to the surrounding medium (while a reader may assume its absolute meaning, i.e. relative to vacuum). I think mentioning "relative to the surrounding air/vacuum" in the first sentence sets things up for the second sentence but please use whichever phrasing you prefer.

... Also, for the second change it's actually transitioning back into glass.

Doesn't a transition from the inner sphere's exterior (as defined by the outward facing normals) to its interior represent a transition from glass to air/vacuum (or whatever interior/exterior medium pair that happens to have the same refraction ratio)? That's really what I meant but I may have misunderstood something.

Either way, I'm going to add a bit more explanation earlier where we introduce refraction.

Sounds good. As I said I think the main concern was to highlight that the refractive index is always relative. Even the absolute one is relative but readers may make assumptions about the definition of IOR and get confused about the meaning of dialectric::ref_index.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the next commit and let me know what you think.

Doesn't a transition from the inner sphere's exterior (as defined by the outward facing normals) to its interior represent a transition from glass to air/vacuum (or whatever interior/exterior medium pair that happens to have the same refraction ratio)? That's really what I meant but I may have misunderstood something.

Ah, yes, I misunderstood your suggested reading. I think it was the "back to air" that confused me. I'd rather explain this as a transition into a third medium that just happens in this example to be the same as the first medium. Anyway, I think the paragraph immediately preceding this explains this all in detail, and we don't need to repeat it in this paragraph.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This?

The outer sphere is just modeled with a standard glass sphere, with an index of refraction of around 1.50 relative to the surrounding air, modeling a refraction from air into glass. The inner sphere is a bit different because its index of refraction should be relative to the surrounding glass outer sphere and model a transition from glass into air. This is actually simple ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of wish we had three different materials to keep things straight, but the model itself would then be rather needlessly complicated. Perhaps use the terms "outside air" and "inside air"? ¯\_(ツ)_/¯

@hollasch
Copy link
Collaborator Author

Ugh. Too many PRs piled up — now this one has a bunch of conflicts. I'll have to create a second one for this, rebased.

@hollasch
Copy link
Collaborator Author

Superseded by #1435.

@hollasch hollasch closed this Mar 12, 2024
@hollasch hollasch deleted the no-negative-radius-spheres branch March 16, 2024 01:54
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.

2 participants