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

Basic fast filtering implementation #36588

Merged
merged 1 commit into from
Feb 28, 2020

Conversation

clayjohn
Copy link
Member

@clayjohn clayjohn commented Feb 27, 2020

This PR implements the fast filtering algorithm described here.

Fast filtering uses a two pass approach to blur environment maps to create radiance maps. It has incredible speed benefits over traditional importance sampling but comes at the cost of quality in certain scenarios. The implementation in this PR is both faster and higher quality than our current implementation of importance sampling, however, it can only be used to generate a 128x128 radiance map. So the current importance sampling implementation is left in place so users can have crisper environment reflections if they need.

comparison of fast filtering (top, 10ms) and importance sampling (below, 75ms)
Final Comparison

I have left the options structure more or less the same. Because there is no longer a low quality importance sampling I removed the "realtime_ggx" project setting and added a setting to choose the quality to use for the fast filtering algorithm. Low quality is default and provides excellent quality for most scenes. High quality can be used for use with especially high detail scenes.

Following this PR I would like to make another one improving the current importance sampling technique so that it is higher quality than the realtime option.

Speed Comparison

Current implementation using Importance Sampling

High Quality Low Quality
Array 75,000 us* 5,000 us*
Non-Array 20,000 us 670 us

Proposed implementation using Fast Filtering

High Quality Low Quality
Array 10,000 us* 4,000 us*
Non-Array 2,000 us 300 us

@akien-mga
Copy link
Member

Clang warning:

servers/visual/rasterizer_rd/rasterizer_scene_rd.cpp:829:6: error: logical not is only applied to the left hand side of this comparison [-Werror,-Wlogical-not-parentheses]
        if (!storage->reflection_probe_get_update_mode(rpi->probe) == VS::REFLECTION_PROBE_UPDATE_ONCE) {
            ^                                                      ~~
servers/visual/rasterizer_rd/rasterizer_scene_rd.cpp:829:6: note: add parentheses after the '!' to evaluate the comparison first
        if (!storage->reflection_probe_get_update_mode(rpi->probe) == VS::REFLECTION_PROBE_UPDATE_ONCE) {
            ^
             (                                                                                        )
servers/visual/rasterizer_rd/rasterizer_scene_rd.cpp:829:6: note: add parentheses around left hand side expression to silence this warning
        if (!storage->reflection_probe_get_update_mode(rpi->probe) == VS::REFLECTION_PROBE_UPDATE_ONCE) {
            ^
            (                                                     )

@clayjohn
Copy link
Member Author

@akien-mga Thanks!

Funny enough, that typo would have made the new code run 6 times every frame instead of once and I didn't even notice cause I still had > 60 FPS.

@clayjohn
Copy link
Member Author

@akien-mga I will push a version with docs later today. Please don't merge until then.

@clayjohn
Copy link
Member Author

@akien-mga Should be ready to merge pending the outcome of CI.
:)

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Docs reviewed.

doc/classes/Sky.xml Outdated Show resolved Hide resolved
doc/classes/Sky.xml Outdated Show resolved Hide resolved
doc/classes/ProjectSettings.xml Outdated Show resolved Hide resolved
doc/classes/ProjectSettings.xml Outdated Show resolved Hide resolved
doc/classes/ProjectSettings.xml Outdated Show resolved Hide resolved
doc/classes/ProjectSettings.xml Outdated Show resolved Hide resolved
doc/classes/ProjectSettings.xml Outdated Show resolved Hide resolved
doc/classes/ReflectionProbe.xml Outdated Show resolved Hide resolved
@clayjohn
Copy link
Member Author

The build failure looks like an issue with the CI itself.

@akien-mga
Copy link
Member

CI passed but there's a conflict with recent docs changes.

@clayjohn
Copy link
Member Author

Fixed :)

@akien-mga akien-mga merged commit 4ff84cb into godotengine:master Feb 28, 2020
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants