Addresses issues: #9, #15, #18, #26, #54, #58, #65, #67, #69

[vchizhov]

Addresses issues: #9, #15, #18, #26, #54, #58, #65, #67, #69.

Haven't tested compiling it on anything except MSVC 2019. Though I have tried to keep it compiler agnostic.

[ronaldfw]

@vchizhov Thanks for submitting this pull request. Unfortunately, I think this will be difficult to get merged as-is. In general, I would encourage you to pick single individual issues and then addressing them one-by-one. As in many places in software engineering, changes (thus pull requests) should adhere to the rule of doing exactly one thing (thus, address one issue). Otherwise it's almost impossible to review and have a focused discussion.

I still want to give you some feedback on this change to incorporate for the future:

• Make sure you stay consistent with the existing style of the code (example: you name headers ".hpp" while existing headers are named ".h").
• You have behavior changes in your code (replacing MAXFLOAT with infinity). This requires careful discussion and would also require changes to the book.
• Code comments shouldn't include discussion of issues and how they are resolved in the code. This should be done on github issues.
• OpenMP is difficult to get working on macOS and I'd bet that your cmake file as-is is not sufficient. Thus your OpenMP changes break macOS portability, which I consider important to maintain.

Let me know if you want more specific feedback on the code and I'd be happy to give the code a more thorough pass. But again, I think the important part is to pick a single issue to address and then work towards that.

[ronaldfw]

One more thing: When you pick an issue to work on, make sure you assign it to yourself (if it is already assigned, please ask the assignee if this is okay with them!). You try to address multiple issues with this pull request for issues that are already assigned to other people. This makes collaboration difficult.

[vchizhov]

I was tempted to rename all '.h' to '.hpp' but I stopped in the middle, thanks for mentioning it. The maxfloat to infinity was suggested in the issue thread. The code comments with the issue # are there for the reviewer, so it would be easier to figure out what the change is referring to, they can be deleted after the review is done. Granted working on one issue at a time would have solved this. I have no macOS machine, so I have no way of testing whether this works on macOS (neither have I tested it on linux). The package is not set to REQUIRED so it should result in a soft error afaik, and the cmakelists should still work out. The openmp part is not even required with MSVC either way, but /openmp has to be set manually (or idk a way to set it through cmake).

P.S. Closed the pull request by mistake.

[trevordblack]

For the next week or so, we're slowing the pace of code changes so that we can focus on getting the book on the web. The v2 is all about just getting it out there.

There are a lot of changes here that we've discussed in github and in the slack.

I'm sure we'll get to use a lot of your changes. For the time being, I would recommend storing this in your own repo. We'll have the slack link up ASAP

On Sun, Aug 25, 2019, 09:05 Vassillen Chizhov [email protected] wrote:

Reopened #79 https://github.com/RayTracing/InOneWeekend/pull/79.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/RayTracing/InOneWeekend/pull/79?email_source=notifications&email_token=ACNZVKGTZPDTQ2B34BAIQZDQGKUTBA5CNFSM4IPIK3JKYY3PNVWWK3TUL52HS4DFWZEXG43VMVCXMZLOORHG65DJMZUWGYLUNFXW5KTDN5WW2ZLOORPWSZGOTHTEREA#event-2582005904, or mute the thread https://github.com/notifications/unsubscribe-auth/ACNZVKGIHVDCC56MKGHXFYDQGKUTBANCNFSM4IPIK3JA .

[vchizhov]

The most important of the changes in terms of theory is the Lambertian material fix, but that needs to be fixed in the book also, I have posted a pdf write up on the issue page, there's a less math heavy and more palatable version in one of the cited sources. The most important issue in practice is the wrong normals for scattering on the inside (metal and lambertian materials), as well as the offset to avoid self-intersection, but those also need to be fixed in the book (if at all).

[trevordblack]

We've discussed internally the incorrect Lambertians. We don't currently have a plan on that, so suffice it so say it won't be making v2.

On Sun, Aug 25, 2019, 09:20 Vassillen Chizhov [email protected] wrote:

The most important of the changes in terms of theory is the Lambertian material fix, but that needs to be fixed in the book also, I have posted a pdf write up on the issue page, there's a less math heavy and more palatable version in one of the cited sources. The most important issue in practice is the wrong normals for scattering on the inside (metal and lambertian materials), as well as the offset to avoid self-intersection.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/RayTracing/InOneWeekend/pull/79?email_source=notifications&email_token=ACNZVKE7AVMY2FSPDZRGY6TQGKWOPA5CNFSM4IPIK3JKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5CWVTI#issuecomment-524643021, or mute the thread https://github.com/notifications/unsubscribe-auth/ACNZVKHCDYULHWAWO6NHB33QGKWOPANCNFSM4IPIK3JA .