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

Using HitRecord::t before declaring it #428

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
Milestone

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

@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

@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
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
Projects
None yet
Development

No branches or pull requests

4 participants