Closed mrmcsoftware closed 5 years ago
mrmcsoftware
commented
5 years ago I see it has already been reported and closed but not fixed. I felt that at some point someone mentioned being off by a factor of 2, but I couldn't remember what it was referring to (or who) - I guess it was this.
mrmcsoftware
commented
5 years ago Correction to my comment: pdf.h has been fixed on github. However this correction isn't in the zip file that can be downloaded on github (v1.123.0). And cosine_density isn't fixed on either.
hollasch
commented
5 years ago (Note: prior issue on this topic: issue RayTracing/TheRestOfYourLife#9 and PR RayTracing/TheRestOfYourLife#8.)
hollasch
commented
5 years ago We wouldn't go back and modify an old already-released version (v1.123.0). The new code release is still under development.
If this is still an issue in the latest source or book, please let me know where specifically the error is.
mrmcsoftware
commented
5 years ago I don't know if I'm looking at the latest source, and I'm not sure how the book directory is involved, but in the file book/ch06.md.html the error is in lines #148 and #149 and in the file src/cosine_density.cc the error is in lines RayTracing/TheRestOfYourLife#21 and RayTracing/TheRestOfYourLife#22. (as mentioned before, pdf.h has already been fixed). But I do understand the need to wait till the next release is finished.
hollasch
commented
5 years ago Ok, I see them now; thanks.
hollasch
commented
5 years ago Please review PR RayTracing/TheRestOfYourLife#70. Thanks.
mrmcsoftware
commented
5 years ago I've reviewed and approved PR RayTracing/TheRestOfYourLife#70. Looks good! Thanks! If there's anything else I need to do let me know (I don't know all the ins and outs of github, so there could be something I'm supposed to do but not know it).
hollasch
commented
5 years ago Nope; that's it. If you're interested in helping out further, I can send you an invite to the RT contributors group for this org, so you can do more formal code reviews. Also, let me know if you'd like an invite to the Slack workspace for this org.
mrmcsoftware
commented
5 years ago What would be involved in being in the group? Is it a "just contribute when/where you feel like it" or is it "review/approve/contribute/etc. to every issue/change"? After the invitation, I looked through the open issues (and for the other books as well). I don't envy your job in dealing with the requested fixes, clarifications/rewordings, enhancements, etc. I do wonder if all of those changes to the code are going to be put in the book as well (that's a lot of work). BTW, I did make several changes to NextWeek's main.cc and awhile ago I thought of tweeting Peter Shirley saying "do with it what you will" but that seemed like too much for twitter (though I am relieved that DM's don't seem to have the 280 character limit): biggest changes: 1. I don't like ASCII ppm's so I changed to binary ppm, 2. I made it compile on Windows as well (already compilable on Linux), 3. And since it's binary ppm, can no longer output to stdout on Windows (CR/LF conversion), so create file, and 4. Created a menu to select which scene you want (including my attempt to figure out which lines of commented code went with which scene, not a perfect match in the case of at least one image). I see some of my changes are already addressed in the issues (but some would conflict with the issue solutions (like random.h RNG for example)). I don't know if I know the books and code well enough (I'm more of a skimmer than a reader) to be a valued contributor, but if it's just contribute where you feel like it (and as time permits), I guess I could.
hollasch
commented
5 years ago Very much a "contribute as much/little" as you want. In this case, it would have meant that I could assign the PR to you to review, since you were involved. There's a ton going on right now, but I'm hoping it will simmer down quite a bit. We're doing what we can to put as much on autopilot as possible. This is GitHub; we all have side projects we'd like to get back to. :D
We've already discussed this informally, but I figured I would also go through the official reporting procedure...
I think there's an error in book 3 (TheRestOfYourLife): the code segment at the end of Chapter 5 doesn't match the equations right before it (x and y are multiplied by 2 in the code (...(phi)2sqrt(r2))) and the code on github has the same error. I believe the equations are right (code should be ...(phi)*sqrt(r2)). This affects "cosine_density.cc" and "pdf.h"