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

vchizhov · 2019-08-25T08:07:54Z

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.

Compiles with no warnings on MSVC 2019 with Warning Level: /W4. - provided CMakeLists.txt (tested only on Windows 10 with MSVC 2019 though, create the project in /build so that it would be ignored by the gitignore) - changed M_PI with a constexpr pi<T>() function - substituted random_double() and drand48() with my own random() function using the Mersenne twister from the standard library - use fstream rather than cout, so that redirecting would not be required - fixed double/float mismatch Removed random.h since it is redundant - I substituted random_double() with my own random() implementation using the Mersenne twister from the standard library. The random_double() in random.h used rand() % (RAND_MAX + 1.0); which is suboptimal (fails multiple statistical tests).

#15 - Clarification on sphere intersection (discriminant) #18 - Was actually fixed in the previous commit - uses fstream for writing out the image rather than cout #26 - Fixed the lambertian material to actually have a constant brdf and not a cos^2 brdf #54 - Same as #15

Adds very basic multi-threading with OpenMP. Requires /openmp command line option in MSVC. Also saves the image to a temporary buffer before writing to disk and adds a clock to count the rendering time.

Fixed the materials to use the correct normals for both lambertian and metal, which allows correct scattering for intersection on the inside. Added a small offset at each intersection along the normal in order to avoid self-intersection (no visible artifacts even with /fp:fast on the test scene). Added a comment for the equality: schlick(cosine, ref_idx) = schlick(cosine, ni_over_nt)

ronaldfw · 2019-08-25T14:12:39Z

@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 · 2019-08-25T14:17:59Z

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 · 2019-08-25T15:56:30Z

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 · 2019-08-25T16:10:33Z

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 ***@***.***> wrote: Reopened #79 <#79>. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#79?email_source=notifications&email_token=ACNZVKGTZPDTQ2B34BAIQZDQGKUTBA5CNFSM4IPIK3JKYY3PNVWWK3TUL52HS4DFWZEXG43VMVCXMZLOORHG65DJMZUWGYLUNFXW5KTDN5WW2ZLOORPWSZGOTHTEREA#event-2582005904>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACNZVKGIHVDCC56MKGHXFYDQGKUTBANCNFSM4IPIK3JA> .

vchizhov · 2019-08-25T16:20:51Z

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 · 2019-08-25T16:41:18Z

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 ***@***.***> 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 <#79?email_source=notifications&email_token=ACNZVKE7AVMY2FSPDZRGY6TQGKWOPA5CNFSM4IPIK3JKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5CWVTI#issuecomment-524643021>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACNZVKHCDYULHWAWO6NHB33QGKWOPANCNFSM4IPIK3JA> .

vchizhov added 5 commits August 23, 2019 19:39

gitignore

5844ef0

Addresses issue: #58

84b1338

Adds very basic multi-threading with OpenMP. Requires /openmp command line option in MSVC. Also saves the image to a temporary buffer before writing to disk and adds a clock to count the rendering time.

vchizhov closed this Aug 25, 2019

vchizhov reopened this Aug 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

vchizhov commented Aug 25, 2019 •

edited

Loading

ronaldfw commented Aug 25, 2019 •

edited

Loading

ronaldfw commented Aug 25, 2019

vchizhov commented Aug 25, 2019 •

edited

Loading

trevordblack commented Aug 25, 2019 via email

vchizhov commented Aug 25, 2019 •

edited

Loading

trevordblack commented Aug 25, 2019 via email

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

Are you sure you want to change the base?

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

Conversation

vchizhov commented Aug 25, 2019 • edited Loading

ronaldfw commented Aug 25, 2019 • edited Loading

ronaldfw commented Aug 25, 2019

vchizhov commented Aug 25, 2019 • edited Loading

trevordblack commented Aug 25, 2019 via email

vchizhov commented Aug 25, 2019 • edited Loading

trevordblack commented Aug 25, 2019 via email

vchizhov commented Aug 25, 2019 •

edited

Loading

ronaldfw commented Aug 25, 2019 •

edited

Loading

vchizhov commented Aug 25, 2019 •

edited

Loading

vchizhov commented Aug 25, 2019 •

edited

Loading