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

Improve names for time, ray values in hittable and derived classes #746

Closed
hollasch opened this issue Oct 3, 2020 · 11 comments
Closed

Improve names for time, ray values in hittable and derived classes #746

hollasch opened this issue Oct 3, 2020 · 11 comments
Assignees
Milestone

Comments

@hollasch
Copy link
Collaborator

hollasch commented Oct 3, 2020

We need better names that distinguish between these, instead of using generic t names.

@hollasch hollasch added this to the v3.2.2 milestone Oct 3, 2020
@hollasch hollasch assigned hollasch and unassigned hollasch Oct 3, 2020
@trevordblack trevordblack self-assigned this Oct 4, 2020
@trevordblack
Copy link
Collaborator

trevordblack commented Oct 4, 2020

We have t for the parametric distance (ray values), time for time

We already have a diagram that has t for parametric distance (figure 2).

What do we change hittable and it's children to?

There's also this egregious example in moving_sphere.h:

point3 cen0, point3 cen1, double t0, double t1, double r, shared_ptr<material> m)
: center0(cen0), center1(cen1), time0(t0), time1(t1), radius(r), mat_ptr(m)

@trevordblack
Copy link
Collaborator

trevordblack commented Oct 4, 2020

t_min, tmin, t_max, tmax

time0, time_0, time1, time_1

t0, tm0, t1, tm1

???

@trevordblack
Copy link
Collaborator

I propose:

t_min, t_max, time0, time1, tm0, tm1

@trevordblack
Copy link
Collaborator

Or, find a new letter for the parametric distance and start redoing figures, code, and text.

@hollasch
Copy link
Collaborator Author

hollasch commented Oct 4, 2020

I think t_min and t_max for t parameters (like with rays) are good, as are time0, time1 for time intervals.

Where would you use tm0 and tm1? Not wild about those.

@trevordblack
Copy link
Collaborator

         camera(
             point3 lookfrom,
             point3 lookat,
             vec3   vup,
             double vfov, // vertical field-of-view in degrees
             double aspect_ratio,
             double aperture,
             double focus_dist,
             double t0 = 0,
             double t1 = 0
         ) {
             auto theta = degrees_to_radians(vfov);
             auto h = tan(theta/2);
             auto viewport_height = 2.0 * h;
             auto viewport_width = aspect_ratio * viewport_height;
 
             w = unit_vector(lookfrom - lookat);
             u = unit_vector(cross(vup, w));
             v = cross(w, u);
 
             origin = lookfrom;
             horizontal = focus_dist * viewport_width * u;
             vertical = focus_dist * viewport_height * v;
             lower_left_corner = origin - horizontal/2 - vertical/2 - focus_dist*w;
 
             lens_radius = aperture / 2;
             time0 = t0;
             time1 = t1;
         }

@trevordblack
Copy link
Collaborator

Basically in function signatures where we don't want to use time0 time1 because those are already member variables.

Did we standardize on how we wanted to accomplish this?

_time0?

@trevordblack
Copy link
Collaborator

we also have member variables _max, and _min in aabb.

So our notation here is inconsistent

@trevordblack
Copy link
Collaborator

Oh, and src/*/bvh.h is a mess

@hollasch
Copy link
Collaborator Author

hollasch commented Oct 5, 2020

Following up on Slack.

@trevordblack
Copy link
Collaborator

trevordblack commented Oct 6, 2020

Outcome:

  • _min -> minimum
  • _max -> maximum
  • tmax -> t_max
  • tmin -> t_min
  • t0
  • t1

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

No branches or pull requests

2 participants