Explain sphere uv #762

hollasch · 2020-10-12T05:52:25Z

Resolves #533

Also, get_sphere_uv() is in sphere.h for book 2, but hittable.h for book 3. This change also moves the function to sphere.h for book 3. Resolves #533

The sphere::hit() function calls get_sphere_uv() with the intersection point transformed to its position on a unit sphere centered at the origin. But this calculation was already performed for the normal vector, so we should just reuse that result instead of calculating it twice.

The prior sphere::hit() methods duplicated the main update block with a single change in variable value. This change solves and selects the intersection root first, then updates the hit record for a good hit, simplifying the code and the text.

CHANGELOG.md

…sphere-uv

trevordblack · 2020-10-15T21:42:12Z

src/TheNextWeek/sphere.h

@@ -43,10 +43,17 @@ bool sphere::bounding_box(double time0, double time1, aabb& output_box) const {
 }


-void get_sphere_uv(const point3& p, double& u, double& v) {
+static void get_sphere_uv(const point3& p, double& u, double& v) {


If you want to make the source static, you'll need to make the inline src in the text static.

This really should be private, but everything's awkward with our approach of doing everything in a header.

Made private static.

books/RayTracingInOneWeekend.html

trevordblack · 2020-10-15T21:48:50Z

books/RayTracingTheNextWeek.html

@@ -1215,7 +1215,7 @@
                vec3 outward_normal = (rec.p - center) / radius;
                rec.set_face_normal(r, outward_normal);
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ C++ highlight
-                get_sphere_uv((rec.p-center)/radius, rec.u, rec.v);
+                get_sphere_uv(outward_normal, rec.u, rec.v);


This has some interesting implications.
The functionality is identical before and after change.

A texture shaded on the inside is reversed. Which is what you'd expect.

trevordblack · 2020-10-15T21:52:14Z

Uhhh, Steve...

Source:

    ...
    auto phi = atan2(p.z(), p.x());
    auto theta = asin(p.y());
    ...

Text:

x=cos(ϕ)cos(θ)
y=sin(ϕ)cos(θ)
z=sin(θ)

The z and y in the latex equations in the text need be flipped. The source is correct

trevordblack · 2020-10-15T21:52:43Z

I'm pretty sure this is what the original issue was bringing up

hollasch · 2020-10-15T21:57:19Z

Interesting. Later comments suggest that there's no issue. I wasn't aware there was a disagreement between the source and the text; I'll revisit.

…tracing.github.io into explain-sphere-uv

trevordblack · 2020-10-20T18:16:32Z

I added another comment that was what I think @D-K-E was referring to.

hollasch · 2020-10-20T18:43:56Z

Thanks. Having gone over it, I still think there are issues. What I'm wrestling with right now is a better explanation for the atan2 arguments, which are juggled to move past the sawtooth discrepancy in the return values (0→π / -π→0). I understand that it works, but am trying to find a way to develop it naturally.

rjkilpatrick · 2020-10-21T22:41:15Z

The convention in ce97f02 is taking θ as the angle from the x-z plane and then ϕ as the polar angle from the z-axis, is this intended?
i.e.

x=cos(ϕ)cos(θ)
z=sin(ϕ)cos(θ)
y=sin(θ)

Relates to the geometry:

Which is atypical for spherical coordinates.
I would suggest following the original definition of θ being from the pole along the z-axis [0, π) and ϕ being the angle from the x-axis, in the x-y plane [0, 2π).

trevordblack · 2020-10-22T01:53:51Z

I think you might be slurring your variables a bit.
Our y axis points up.

Do you propose we make the z axis point up?

If you propose making the θ as the angle coming down from the up axis (in this case y), then, yes, we are considering this (and will probably do it). It's an old bug that no one caught cause there isn't a test for it

hollasch · 2020-10-22T02:00:59Z

I've always seen θ going around the equator (for example, longitude), and φ indicating the angle from one pole to the other (like latitude). Like John, I was all set to assert that the book was "nonstandard". Then I did an image search on {spherical coordinates}, and realized it was just another item on the list of

left-handed versus right-handed coordinates
clockwise vs counter-clockwise winding order
row major vs column major matrix memory layout
column vs row vectors
+Y up vs +Y down
and on and on and on and on and on and on and on and on and on and on and on and on and on and on and on and on and on and on and on and on and on and on and on and on and on and on and on and on and on and on and on and on and on and on and on and on and on and on

What's really needed here is a proper illustration (in the same manner that John posted above) and a bit more explanation.

trevordblack · 2020-10-22T02:03:52Z

I have an immense urge to go through your list and explain how there's EXACTLY one right option for each...

hollasch · 2020-10-25T22:08:28Z

This change has been split up into and is superseded by #774 and #775

hollasch added 3 commits October 11, 2020 15:55

Add comments for get_sphere_uv() function

6f408f2

Also, get_sphere_uv() is in sphere.h for book 2, but hittable.h for book 3. This change also moves the function to sphere.h for book 3. Resolves #533

hollasch added this to the v3.2.2 milestone Oct 12, 2020

hollasch requested review from trevordblack and a team October 12, 2020 05:52

hollasch self-assigned this Oct 12, 2020

hollasch added status: in PR and removed status: in PR labels Oct 12, 2020

Merge branch 'dev-patch' into explain-sphere-uv

9b7aa22

hollasch commented Oct 12, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

hollasch added 3 commits October 11, 2020 23:14

Tweaks for get_sphere_uv()

f9e2c1c

Merge remote-tracking branch 'origin/explain-sphere-uv' into explain-…

b0a1293

…sphere-uv

Merge branch 'dev-patch' into explain-sphere-uv

5e36388

trevordblack reviewed Oct 15, 2020

View reviewed changes

books/RayTracingInOneWeekend.html Show resolved Hide resolved

trevordblack reviewed Oct 15, 2020

View reviewed changes

hollasch and others added 5 commits October 16, 2020 23:34

sphere.h: made get_sphere_uv() private static

f54f76a

Merge branch 'dev-patch' into explain-sphere-uv

cb61894

Merge branch 'dev-patch' into explain-sphere-uv

4014e6e

Merge branch 'explain-sphere-uv' of https://github.com/RayTracing/ray…

f482039

…tracing.github.io into explain-sphere-uv

swapped the y and z in latex

ce97f02

This was referenced Oct 25, 2020

Refactor sphere hit method to use common code #774

Merged

Corrected sphere UV coordinates #775

Merged

hollasch closed this Oct 25, 2020

hollasch deleted the explain-sphere-uv branch August 20, 2021 23:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Explain sphere uv #762

Explain sphere uv #762

hollasch commented Oct 12, 2020

trevordblack Oct 15, 2020

hollasch Oct 15, 2020

hollasch Oct 17, 2020

trevordblack Oct 15, 2020

trevordblack commented Oct 15, 2020

trevordblack commented Oct 15, 2020

hollasch commented Oct 15, 2020

trevordblack commented Oct 20, 2020

hollasch commented Oct 20, 2020

rjkilpatrick commented Oct 21, 2020 •

edited

Loading

trevordblack commented Oct 22, 2020

hollasch commented Oct 22, 2020

trevordblack commented Oct 22, 2020

hollasch commented Oct 25, 2020

Explain sphere uv #762

Explain sphere uv #762

Conversation

hollasch commented Oct 12, 2020

trevordblack Oct 15, 2020

Choose a reason for hiding this comment

hollasch Oct 15, 2020

Choose a reason for hiding this comment

hollasch Oct 17, 2020

Choose a reason for hiding this comment

trevordblack Oct 15, 2020

Choose a reason for hiding this comment

trevordblack commented Oct 15, 2020

trevordblack commented Oct 15, 2020

hollasch commented Oct 15, 2020

trevordblack commented Oct 20, 2020

hollasch commented Oct 20, 2020

rjkilpatrick commented Oct 21, 2020 • edited Loading

trevordblack commented Oct 22, 2020

hollasch commented Oct 22, 2020

trevordblack commented Oct 22, 2020

hollasch commented Oct 25, 2020

rjkilpatrick commented Oct 21, 2020 •

edited

Loading