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

Jump to bottom

Light paths recording annotations and tweaks #2629

Merged
merged 2 commits into from May 30, 2019
Merged

Light paths recording annotations and tweaks #2629

merged 2 commits into from May 30, 2019

Conversation

fu5ha
Copy link
Contributor

@fu5ha fu5ha commented May 28, 2019

  • Annotates light path creation code
  • Uses reinhard tonemapping (x/(1+x)) for displaying the radiance as color when drawing. This gives much more helpful color results than previously when displaying light paths, since it gives a sense of intensity as well as just hue:

old:
image
image

new:
image
image

  • Takes into account secondary emitter radiance in the middle of a light path. Previously if a path intersected with an emitter that was also a reflector that was not the origin of the path, the emitted radiance of said emitter would not be added to the path's total radiance.

@fu5ha
annotate light path creation code, use reinhard tonemapping for light…
cd85f05 
… path display colors, take into account secondary emitter radiance in light paths
@LZaw LZaw requested review from LZaw and removed request for LZaw May 28, 2019 08:50
@LZaw
Copy link
Member

LZaw commented May 28, 2019

Looks good,
One (you?) could possibly refactor the three functions

void LightPathStream::create_path_from_hit_emitter(const size_t emitter_event_index)
void LightPathStream::create_path_from_sampled_emitter(const size_t emitter_event_index)
void LightPathStream::create_path_from_sampled_environment(const size_t env_event_index)

since they got a lot of overlapping code.

dictoon
dictoon reviewed May 29, 2019
View reviewed changes
Copy link
Member

@dictoon dictoon left a comment

Choose a reason for hiding this comment

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

Nice! You seem to already have a good understand of how light path recording works.

src/appleseed/renderer/kernel/lighting/lightpathstream.cpp Outdated Show resolved Hide resolved
src/appleseed/renderer/kernel/lighting/lightpathstream.cpp Outdated Show resolved Hide resolved
src/appleseed/renderer/kernel/lighting/lightpathstream.cpp Outdated Show resolved Hide resolved
@fu5ha
Copy link
Contributor Author

fu5ha commented May 30, 2019

Ready for review again :)

dictoon
dictoon approved these changes May 30, 2019
View reviewed changes
Copy link
Member

@dictoon dictoon left a comment

Choose a reason for hiding this comment

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

Looks good!

@dictoon dictoon changed the title Light path creation annotation and tweaks Light paths recording annotations and tweaks May 30, 2019
@dictoon dictoon merged commit 09d8370 into appleseedhq:master May 30, 2019
oktomus
oktomus reviewed May 31, 2019
View reviewed changes
src/appleseed.studio/mainwindow/rendering/lightpathswidget.cpp
@@ -385,7 +385,7 @@ void LightPathsWidget::render_light_path(const size_t light_path_index) const
light_path_recorder.get_light_path_vertex(i, v1);

auto radiance = Color3f::from_array(v1.m_radiance);
radiance /= sum_value(radiance);
radiance /= radiance + Color3f(1.0);
Copy link
Member

Choose a reason for hiding this comment

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

Should be Color3f(1.0f)

src/appleseed/renderer/kernel/lighting/lightpathstream.cpp
@@ -269,9 +269,18 @@ void LightPathStream::create_path_from_hit_emitter(const size_t emitter_event_in

// Update current radiance.
const auto& throughput = event_data.m_path_throughput;
// Multiply by previous throughput to attenuate by the next hit
Copy link
Member

Choose a reason for hiding this comment

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

End your comments with a dot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oktomus oktomus oktomus left review comments

@dictoon dictoon dictoon approved these changes

@LZaw LZaw LZaw approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@fu5ha @LZaw @dictoon @oktomus