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

Update book 1 renders plus many listing fixes #618

Closed
wants to merge 5 commits into from

Conversation

hollasch
Copy link
Collaborator

  • Reduced code duplication in dielectric::scatter()
  • Change standard render dimensions to 400x225
  • Change image 6 to a before-and-after pair to show antialiasing
  • Refactored material and geometry declarations (multiple listings)
  • Refactored assignment of etai_over_etat
  • Fix multiple listing highlighting misses
  • Fix multiple listing include errors
  • Add missing double t member of struct hit_record
  • Add missing scene code changes at various points
  • Show final scene render parameters plus highlighting

See #179
Resolves #547
Resolves #548
Resolves #549
Resolves #550
Resolves #551
Resolves #552
Resolves #553
Resolves #554
Resolves #555
Resolves #556
Resolves #557
Resolves #560
Resolves #561
Resolves #562
Resolves #563
Resolves #564
Resolves #565
Resolves #566

- Reduced code duplication in dielectric::scatter()
- Change standard render dimensions to 400x225
- Change image 6 to a before-and-after pair to show antialiasing
- Refactored material and geometry declarations (multiple listings)
- Refactored assignment of `etai_over_etat`
- Fix multiple listing highlighting misses
- Fix multiple listing include errors
- Add missing `double t` member of struct `hit_record`
- Add missing scene code changes at various points
- Show final scene render parameters plus highlighting

See #179
Resolves #547
Resolves #548
Resolves #549
Resolves #550
Resolves #551
Resolves #552
Resolves #553
Resolves #554
Resolves #555
Resolves #556
Resolves #557
Resolves #560
Resolves #561
Resolves #562
Resolves #563
Resolves #564
Resolves #565
Resolves #566
@hollasch hollasch added this to the v3.2.0 milestone May 24, 2020
@hollasch hollasch requested review from trevordblack and a team May 24, 2020 00:42
@hollasch hollasch self-assigned this May 24, 2020
`camera::get_ray()` parameters renamed from `u,v` to `s,t`.

Resolves #616
@trevordblack
Copy link
Collaborator

Really like the before/after shot with antialiasing

@@ -2364,11 +2405,10 @@
solution does not exist, the glass cannot refract, and therefore must reflect the ray:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trailing comma on line 2401

@trevordblack
Copy link
Collaborator

trevordblack commented May 24, 2020

I'm seeing really weird artifacting on some all of the material renders.
See:
img-1.11-metal-shiny.png
img-1.12-metal-fuzz.png
img-1.15-glass-sometimes-refract.png
img-1.16-glass-hollow.png
img-1.18-view-distant.png
img-1.19-view-zoom.png
img-1.21-book1-final.png

We need to figure out where those are coming from

@trevordblack
Copy link
Collaborator

trevordblack commented May 25, 2020

So, I'm rerendering the images and i'm not getting artifacts, I'll have them up as a commit in this branch.
I'm also fixing:
trailing comma (see above)
bubble thickness

  • is 0.4 and 0.45 in different parts of the book
  • Am standardizing

@trevordblack
Copy link
Collaborator

I'm wrapping a bunch of small changes into my commit

@hollasch
Copy link
Collaborator Author

Why would your images look different? Like you say, we should figure this out.

Also, if we end up with better images, we'll need to construct a pristine pull request. I don't want to bloat the repo database with the unused images.

@hollasch
Copy link
Collaborator Author

I presume you're using the progression branch to do the renders, yes?

@trevordblack
Copy link
Collaborator

I'm not sure why my images are different. I'm running ubuntu w clang9.

I'm actually rerendering almost all of the images

@trevordblack
Copy link
Collaborator

Nope. I'm adding directly to render-updates.
Mistake?

@hollasch
Copy link
Collaborator Author

I'll retry with the modern C++ RNG — there's no good reason for us to use the old one.

@hollasch
Copy link
Collaborator Author

Yes — use the progression branch. This will also ensure that you're not accidentally slipstreaming in any future or personal modifications, and are following the progression as the book presents.

@trevordblack
Copy link
Collaborator

Okay, go ahead and try. BUT, I don't think your artifacting is the RNG

@trevordblack
Copy link
Collaborator

ahhh, shucks
I'll move over to progression

@trevordblack
Copy link
Collaborator

Okay, I'm seeing now that progression/dev-minor/render-updates is a rats nest.
I'm going to create a branch off of progression that has ONLY the image updates, and create a PR.
I'm rendering the final scene rn, which could take another hour.

Everything is rendered at 400x225 (100spp), except for the final scene, which is 1200x800 (500spp)

@hollasch
Copy link
Collaborator Author

The book stipulates 50spp. The intent is to enable fast turnaround while developing, not good-looking pictures.

@trevordblack
Copy link
Collaborator

Wait. What should I be rendering at?

@hollasch
Copy link
Collaborator Author

400×225, 50spp. But all of this is coded/written in the progression branch. So you just need to visit the individual commits and run. For example, git checkout progression~20 to get to the codebase reset. progression~19 is img-1.01, progression~18 is img-1.02, progression~17 is img-1.03, and so on.

@hollasch
Copy link
Collaborator Author

@trevordblack helped me figure out that I'd missed the surface-acne epsilon, which caused all sorts of subtle rendering artifacts. Walking through the progression uncovered more small issues. Going to wad this one and recreate for a second pass.

@hollasch hollasch closed this May 26, 2020
@hollasch hollasch deleted the render-updates branch May 26, 2020 09:17
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

Successfully merging this pull request may close these issues.

None yet

2 participants