8000 Using HitRecord::t before declaring it · Issue #428 · RayTracing/raytracing.github.io · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Using HitRecord::t before declaring it #428

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.

Sign up for GitHub

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

Jump to bottom
Closed
Krenodeno opened this issue Apr 2, 2020 · 9 comments
Closed

Using HitRecord::t before declaring it #428

Krenodeno opened this issue Apr 2, 2020 · 9 comments
Assignees
@trevordblack
Labels
book 1: One Weekend
Milestone
v3.2.0

Comments

@Krenodeno
Copy link

In Chapter 6, at listing 13, hit_record definition is

struct hit_record {
    vec3 p;
    vec3 normal;
};

and then in listing 14, there is reference to a t member that do not exist... yet.

bool sphere::hit(const ray& r, double t_min, double t_max, hit_record& rec) const {
[...]
            rec.t = temp;
            rec.p = r.at(rec.t);
            rec.normal = (rec.p - center) / radius;

hit_record's member t is defined later as the time:
I know that we’ll also want motion blur at some point, so I’ll also add a time input variable.
Therefore I cannot see any relation to time in the calculations of temp listing 18:

auto a = r.direction().length_squared();
auto half_b = dot(oc, r.direction());
auto root = sqrt(discriminant);
auto temp = (-half_b - root)/a;

If I'm correct, at this point t is more like a depth buffer, as to see listing 19.
@trevordblack
Copy link
Collaborator

This is a bug. Thank you for catching it.

Unfortunately, rec.t refers not to the time, but the parameterization of the ray. Namely:

orig + t*dir.

t needs to be put back into the initial definition of hit_record. And we need to consider renaming it since it's overloading with the time variable later on

This was referenced Apr 2, 2020
Closed
Merged
@trevordblack trevordblack self-assigned this Apr 2, 2020
@trevordblack trevordblack added this to the v3.1.0 milestone Apr 2, 2020
@trevordblack
Copy link
Collaborator

Just use

struct hit_record {
    vec3 p;
    vec3 normal;
    double t;
    bool front_face;

    inline void set_face_normal(const ray& r, const vec3& outward_normal) {
        front_face = dot(r.direction(), outward_normal) < 0;
        normal = front_face ? outward_normal :-outward_normal;
    }
};

for hit_record, where t is the parameterization of the ray (not time). Neglect the comment I know that we’ll also want motion blur at some point, so I’ll also add a time input variable. as it was a brain fart on my part.

There is a new PR to fix this issue.

Thank you for bringing it up

@Krenodeno
Copy link
Author

Glad I could help !

@hollasch
Copy link
Collaborator
hollasch commented Apr 2, 2020

Resolved in PR #429

@hollasch hollasch closed this as completed Apr 2, 2020
@halfhorst halfhorst mentioned this issue Apr 19, 2020
Closed
@junkimu
Copy link
junkimu commented May 17, 2020

t still needs to be added back to listing 18: "The hittable class with time and side"

@hollasch
Copy link
Collaborator

Thank you for catching this!

@hollasch hollasch reopened this May 17, 2020
@hollasch
Copy link
Collaborator

It would be good to use a better name, perhaps ray_parameter.

@hollasch hollasch modified the milestones: v3.1.0, v3.1.2 May 17, 2020
@hollasch hollasch modified the milestones: v3.1.2, v3.2.0 May 17, 2020
@hollasch
Copy link
Collaborator

See also #418 regarding our nomenclature for time0 and time1. This should be considered as well when making these changes.

hollasch added a commit that referenced this issue May 24, 2020
@hollasch
Prior commit fixes 428 (hit_record param t)
94360de 
Resolves #428
@hollasch
Copy link
Collaborator

Resolved in PR #625

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@trevordblack trevordblack

Labels
book 1: One Weekend
Projects
None yet
Milestone
v3.2.0
Development

No branches or pull requests
4 participants
@hollasch @Krenodeno @trevordblack @junkimu
0