Code improvements: InOneWeekend §4.2

[dimitry-ishenko]

// Calculate the image height, and ensure that it's at least 1.
int image_height = static_cast<int>(image_width / aspect_ratio);

There is no need for static_cast. Assigning to an int variable will automatically do that:

// Calculate the image height, and ensure that it's at least 1.
int image_height = image_width / aspect_ratio;

---

// Viewport widths less than one are ok since they are real valued.
auto viewport_height = 2.0;
auto viewport_width = viewport_height * (static_cast<double>(image_width)/image_height);

I am not sure why the division has to be done first, but if you remove the parentheses, then static_cast becomes unnecessary since the first member of the expression is double and will cause other members to be promoted to double. So, you can simply write:

// Viewport widths less than one are ok since they are real valued.
auto viewport_height = 2.0;
auto viewport_width = viewport_height * image_width / image_height;

[dimitry-ishenko]

There is also a missing word just below Listing 8:

"If you're wondering why we don't just use aspect_ratio when computing viewport_width, it's because the value set to aspect_ratio is the ideal ratio, it may not be the actual ratio between image_width and image_height. If image_height was allowed to be real valued—rather than just an integer—then it would be fine to use aspect_ratio. ..."

And in the same paragraph image_height is called integer_height twice:

"... But the actual ratio between image_width and image_height can vary based on two parts of the code. First, integer_height is rounded down to the nearest integer, which can increase the ratio. Second, we don't allow integer_height to be less than one, which can also change the actual aspect ratio."

[hollasch]

Naked assignments from double to in get

warning C4244: '=': conversion from 'double' to 'int', possible loss of data  build\inOneWeekend.vcxproj]

Generally, any precision loss should get an explicit static_cast to indicate that the resulting precision loss is expected and accepted. You may or may not see this warning with your compiler/lint configuration, but others may.

[hollasch]

Dropping the static_cast for viewport_width is indeed fine. I think I originally had the subexpression image_width / image_height in parentheses, which requires the static_cast. Dropping the parentheses should be fine, though, so I'll do that and lose the cast.

[dimitry-ishenko]

Generally, any precision loss should get an explicit static_cast to indicate that the resulting precision loss is expected and accepted.

It makes sense if you want to express intent, but sometimes it can get in the way of readability. I am only bringing this up because static casts make expressions harder to read IMHO.

BTW C++ considers this a standard conversion which is done implicitly. See §7 Standard conversions and §7.1 Floating-integral conversions.

And, outside of msvc none of the other major compilers -- gcc, clang, icc (icx), nvc++, etc. with -Wall and -pedantic options -- complain about it.

[hollasch]

That may be, but keep in mind that we are explicitly not presenting production-quality or even standard C++ code. Readers of the books are expected to implement their own code, frequently in another language, and often with very little familiarity with the quirks of C++. The code must be legible across the entire audience, and we take pains to guard against easy mistakes when mentally transcribing examples into other implementations or languages.

If you look at the discussions, there are tons of examples of people running into situations where they inadvertently missed some small detail or nuance and lost a lot of time trying to track down where there the example code didn't translate exactly. The fewer implicit effects specific to the quirks of C++ we have, the better.

[hollasch]

The specific section you're commenting on has been a challenge. I originally thought of doing something like this:

    // Determine viewport dimensions.
    auto theta = degrees_to_radians(vfov);
    auto h = tan(theta/2);
    double actual_aspect_ratio = static_cast<double>(image_width) / image_height;
    auto viewport_height = 2 * h * focus_dist;
    auto viewport_width = viewport_height * actual_aspect_ratio;

making explicit the work to recapture the actual integer-based aspect ratio from the desired aspect ratio expressed in the aspect_ratio member. What would this look like in Erlang or Lisp or CoffeeScript or something else? I have no idea, but it's critical that the reader deeply understand the promotion & demotion inherent in this section.

Can you suggest an alternative approach, given real-valued aspect_ratio, integer-valued image_width & image_height, and which derives the real-valued viewport dimensions, with as little implicit C++ behavior as possible? Imagine you're handing this code to someone who has never written a line of C++ before, and using this as a recipe for some other language.

[dimitry-ishenko]

The specific section you're commenting on has been a challenge.

That's really strange, as it seems like basic math.

Can you suggest an alternative approach, given real-valued aspect_ratio, integer-valued image_width & image_height, and which derives the real-valued viewport dimensions, with as little implicit C++ behavior as possible?

IMHO there is no need for an alternative approach. What you've done in simplifying it is already great. For the heck of it, I've implemented the above snipped in different languages, and all implementation look nearly identical:

C++ https://www.ideone.com/Qdbzdx

auto aspect_ratio = 17.0 / 7.0;
int image_width = 400;

int image_height = image_width / aspect_ratio;

auto viewport_height = 2.0;
auto viewport_width = viewport_height * image_width / image_height;

Python https://www.ideone.com/SMwMiG

aspect_ratio = 17.0 / 7.0
image_width = 400

image_height = int(image_width / aspect_ratio)

viewport_height = 2
viewport_width = viewport_height * image_width / image_height

Java https://www.ideone.com/5iJgvy

double aspect_ratio = 17.0 / 7.0;
int image_width = 400;

int image_height = (int)(image_width / aspect_ratio);

double viewport_height = 2.0;
double viewport_width = viewport_height * image_width / image_height;

JavaScript https://www.ideone.com/ScSzEB

var aspect_ratio = 17.0 / 7.0;
var image_width = 400;

var image_height = Math.trunc(image_width / aspect_ratio);

var viewport_height = 2;
var viewport_width = viewport_height * image_width / image_height;

Julia https://www.ideone.com/i4qcie

aspect_ratio = 17.0 / 7.0;
image_width = 400;

image_height = trunc(Int, image_width / aspect_ratio);

viewport_height = 2;
viewport_width = viewport_height * image_width / image_height;

I've purposely picked random aspect ratio so that we don't get round numbers. Sorry, no Erlang. 😼

Anyway, it's up to you obviously how you want this code to be. You've done an amazing job so far. I am just another pair of 👀 offering my two 🪙.

[hollasch]

Thanks; I definitely value the second opinion. My paranoia just has me worried about the casual reader / beginning programmer who types

viewport_width = viewport_height * (image_width / image_height);

as it originally appeared. :smile:

[dimitry-ishenko]

viewport_width = viewport_height * (image_width / image_height);

This would definitely be an issue in C++. Python no. Java maybe. JS not sure. Actually, you've made me curious. I'm going to try int by int division and see how different languages behave.

[hollasch]

I'm quite surprised that Python doesn't have a problem with this. That notches it up a bit in my appreciation. Promotion dependent on operator precedence/order of evaluation is kind of nasty, I think. And now I'm questioning my own understanding, and curious about what you find.

[dimitry-ishenko]

Minimalist example: Print result of 5 / 2.

Results: C++ and Java return 2. Python, JS and Julia return 2.5

Links: C++ https://www.ideone.com/SzYDsB Python https://www.ideone.com/k37IdA Java https://www.ideone.com/AJG8yI JavaScript https://www.ideone.com/Vwcvir Julia https://www.ideone.com/Naurmn

[hollasch]

Ah, I at least understand the JS example, as JS has no natural integer types. I suppose that must be true of Python and Julia as well, so I guess it's not about order of evaluation after all.

[hollasch]

So I guess my Python appreciation notches back down to what it was before. :smile:

[hollasch]

But this all has me thinking. In standard production C++ code, the recommendation is to use static_cast liberally as that's a definite pattern you can search the codebase for when needed. However, the equivalent alternative is to use type constructors to get this explicit behavior (but never old-style C casts). So the code might look like this:

viewport_width = viewport_height * (double(image_width) / image_height);

That's a bit less noisy, so helps your concern about readability, but also signals to the reader that the promotion is required and/or at least something to be aware of. This would be an example of how we often use non-standard C++ in the interest of clarity for programmers of other languages.

I'm now wondering if this might be a tactic we should use across the example code instead of the admittedly awkward static_cast<type>(thing) syntax.

[hollasch]

For the record, our current codebase currently has 31 static casts to int, and 3 static casts to double.

For one thing, I will review these to make sure they can't be avoided somehow.

[hollasch]

(Heh. It just occurred to me that I would have to do a bit more work to find the casts in our code with old C-style casts.)

[hollasch]

Just recording what I discover on this issue. It turns out that

foo(x) ⇔ (foo)(x) ⇔ (foo)x ⇔ static_cast<foo>(x)

only when foo is a class type. So it's incorrect to say (as I did above) that double(x) calls doubles constructor. double has no constructor, only assignments and (built-in) conversions. It's a toss-up: C-style casts are fine with our current uses, and arguably more legible, if not recommended. But still not sure if that's enough to have us convert our code to this non-standard style.

[dimitry-ishenko]

From what I understand (type)x is equivalent to type(x) for basic types. So yes, it's still a C-style cast.

Ref: https://en.cppreference.com/w/cpp/language/explicit_cast

OK, one idea is to maybe introduce a pair of functions:

constexpr auto to_int(double x) { return static_cast<int>(x); }
constexpr auto to_double(int x) { return static_cast<double>(x); }

Then the expression becomes:

viewport_width = viewport_height * (to_double(image_width) / image_height);

It's still pretty readable. It shows the intent. And it follows C++ standard.

[dimitry-ishenko]

Another creative idea which I've used a few times to induce promotion to double:

viewport_width = viewport_height * (1.0 * image_width / image_height);

[hollasch]

Regarding the to_int and to_double conversions, there's a pro and a con.

con: I don't like it. pro: I kind of like it.

[dimitry-ishenko]

TBH I don't like it either. My slightly more likable version is to use a template:

template<typename T>
constexpr auto to(auto x) { return static_cast<T>(x); }

viewport_width = viewport_height * (to<double>(image_width) / image_height);

But I think some C++ new-comers might get overwhelmed by it.

[armansito]

I think I prefer where @hollasch landed with #1227, which uses constructor-style casts over a custom conversion utility. I think the latter would have obscured the code IMO.

Somewhat tangential to the discussion but related: I do wonder if replacing the use of auto with explicit type declarations would further improve clarity, for example the snippet from @dimitry-ishenko's original comment reads a lot clearer to me with explicit types:

// Viewport widths less than one are ok since they are real valued.
double viewport_height = 2.0;
double viewport_width = viewport_height * image_width / image_height;

auto is sometimes very useful for brevity (e.g. when the type involves highly verbose template expansions) but I think it unnecessarily obscures the code for a primitive type. Annotating the destination type also clarifies the intent when explicit and implicit type conversion is intermixed in an assignment (e.g. double(image_width) / image_height) as the reader may not be familiar with all the C++ type conversion rules.

I think this is even more valuable when different numerical type definitions are intermixed. For example, this block from src/TheNextWeek/perlin.h

auto u = p.x() - floor(p.x());
auto v = p.y() - floor(p.y());
auto w = p.z() - floor(p.z());
auto i = int(floor(p.x()));
auto j = int(floor(p.y()));
auto k = int(floor(p.z()));

would be clearer if it were rewritten as

double u = p.x() - floor(p.x());
double v = p.y() - floor(p.y());
double w = p.z() - floor(p.z());
int i = int(floor(p.x()));
int j = int(floor(p.y()));
int k = int(floor(p.z()));

In different programming languages, type inference is sometimes the idiomatic way to declare variables. For example, if I were implementing this book in Rust I would have written the above code as let u = p.x() - p.x().floor();. That said, if I were writing a Rust code snippet for pedagogical purposes, I would have preferred an explicit type annotation, like let u: f64 = p.x() - p.x.floor();.

Just food for thought.

[dimitry-ishenko]

for example the snippet from @dimitry-ishenko's original comment reads a lot clearer to me with explicit types:

// Viewport widths less than one are ok since they are real valued.
double viewport_height = 2.0;
double viewport_width = viewport_height * image_width / image_height;

@armansito but do you really care what type they are? Would it make a difference if they were float or long double? In some cases yes, of course; but in majority of the cases it wouldn't actually matter.

That's why I try to use auto whenever possible, unless I really need to know/control what type I want.

Also, things like this are IMHO redundant:

int i = int(floor(p.x()));

You are already saying on the right side of the equation that you want int, so to me this is just as readable:

auto i = int(floor(p.x()));

And, my preferred version would be even shorter:

int i = floor(p.x());

@hollasch so it seems you've made up your mind. Should I go ahead and close the issue?

NOTE: There were also a few typos which I've mentioned in my 2nd comment. Should I move them to a separate issue?

[dimitry-ishenko]

My overarching approach to programming is:

More code = more bugs.

And, by "code" I mean human-written code. So, I try to write less code or make compiler write code for me (eg, through templates, using auto, etc.)

[hollasch]

I think this issue is settled. We are now using the double(x) and int(x) syntax throughout.