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

Set ambient light to avoid exactly black shadow. #7

Conversation

fidergo-stephane-gourichon
Copy link
Contributor

Makes limit between sphere and floor much clearer and cleaner.

Notice that the variable that was named "shadow_radius2" is actually compared to the square of distances, so I put it at 4 for a non-squared radius of 2, matching the radius of the sphere.
…rd_skew' and 'fix_shadow'

* fix_checkerboard_row_twice_as_large:
  Don't normalize ray, just copy.
  Fix one row of checkerboard being twice as large.

* fix_checkerboard_skew:
  Fix bug in checkerboard floor.

* fix_shadow:
  Fix bug in the (admitedly fake) shadow.
Makes limit between sphere and floor much clearer and cleaner.
@fidergo-stephane-gourichon
Copy link
Contributor Author

This is best appreciated after merging #4 #5 #6 and #7.

The viewer might be surprised that the shadow extends beyond the lower bound of the image. As far as I know, this is actually accurate given the geometry of the scene. "Fixing" this would imply to push the sphere further and reduce the angular width of view.

with_shadow_and_PR_4_5_6_7

@fidergo-stephane-gourichon fidergo-stephane-gourichon force-pushed the ambient_light_makes_more_appreciable_shadows branch from 381ebe2 to 9a8e4c2 Compare January 11, 2021 00:04
@fidergo-stephane-gourichon
Copy link
Contributor Author

Pushed branch that already combines #4 #5 #6 #7, to avoid conflict of standalone #7 when merging on top of the others.
This branch now represents all changes combined.

You can still access standalone change as 381ebe2 .

@fidergo-stephane-gourichon
Copy link
Contributor Author

fidergo-stephane-gourichon commented Jan 11, 2021

Here's result of latest push, compared with original:
with_shadow_and_PR_4_5_6_7_01
original image

@64
Copy link
Owner

64 commented Jan 11, 2021

Pushed branch that already combines #4 #5 #6 #7, to avoid conflict of standalone #7 when merging on top of the others. This branch now represents all changes combined.

Sorry, only noticed this after merging the others.

This is great work - thanks so much. My only concern, as you said, is that it's a bit surprising to the viewer, and for a project like this I'd rather have an image that's not surprising over something that's more physically accurate. I think it can be faked to look nicer by simply not painting the shadow when the depth is >= 2 (so we don't see it in the reflection of the sphere). I'll do this now and merge everything together.

@64 64 merged commit 9a8e4c2 into 64:master Jan 11, 2021
@64 64 mentioned this pull request Jan 11, 2021
@fidergo-stephane-gourichon
Copy link
Contributor Author

fidergo-stephane-gourichon commented Jan 11, 2021

Thanks for merging the PRs.

This is great work - thanks so much. My only concern, as you said, is that it's a bit surprising to the viewer, and for a project like this I'd rather have an image that's not surprising over something that's more physically accurate.

Well, you're not selling a Hollywood movie, it's a hacker project. So, if the viewer is surprised then educated and makes an actual progress, that's the option I would prefer.

I think it can be faked to look nicer by simply not painting the shadow when the depth is >= 2 (so we don't see it in the reflection of the sphere). I'll do this now and merge everything together.

I saw you implemented it. I saw the result and think it looks like a bug, making the program appear less capable than it actually is.

But hey, it's your project and if I disagree I can fork it and launch my own blender competitor. ;-)

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