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

[4.0] Add support for glow maps #54574

Merged
merged 1 commit into from
Jan 26, 2022
Merged

Conversation

Ansraer
Copy link
Contributor

@Ansraer Ansraer commented Nov 3, 2021

This PR is a minor enhancement for the glow post processing effect and allows users to provide a glow map (e.g. lens dust).
The influence of this glow map can be controlled with the glow_map_strength float variable, which defaults to 0 (=glow map turned off).

Here is an example screenshot I took while using a low res lens-dust jpg I found on google as the glow map:
Screenshot from 2021-11-04 00-01-55

Disclaimer: I originally wrote this for one of my projects, before deciding to turn it into a PR. I did my best to clean things up as much as I could, but in the processes ended up nearly doubling the number of tone mapping pipelines. Would greatly appreciate some feedback on that part in particular from someone with more rendering experience.
Nevermind, after discussing it on rocketchat that change was reverted. The new #ifdef blocks were removed and the glow map was moved to binding 1 of set 2.

@Ansraer Ansraer requested a review from a team as a code owner November 3, 2021 23:22
@clayjohn clayjohn added this to the 4.0 milestone Nov 3, 2021
@Calinou
Copy link
Member

Calinou commented Nov 4, 2021

The influence of this glow map can be controlled with the glow_map_strength float variable, which defaults to 0 (=glow map turned off).

Shouldn't it default to 1 or perhaps 0.5, with the effect being automatically nullified if no glow map texture is specified? This way, you can see an effect immediately as soon as a glow map texture is loaded.

@Ansraer
Copy link
Contributor Author

Ansraer commented Nov 4, 2021

As discussed with @clayjohn on rocketchat I removed the #define and moved the glow map into set 2 binding 1.

The influence of this glow map can be controlled with the glow_map_strength float variable, which defaults to 0 (=glow map turned off).

Shouldn't it default to 1 or perhaps 0.5, with the effect being automatically nullified if no glow map texture is specified? This way, you can see an effect immediately as soon as a glow map texture is loaded.

The effect already turns itself of if no glow map is provided, but I can also change the default value. I think I will set it to 0.8 instead of 1, that is what I have been using during testing so far.

@Ansraer
Copy link
Contributor Author

Ansraer commented Nov 4, 2021

Huh, must have lost the docs during the rebase. Should be fixed now.

@Calinou
Copy link
Member

Calinou commented Nov 6, 2021

I tested this feature locally, it's working pretty well overall:

image

Testing project: test_glow_map.zip

Two things I noticed:

  • When the glow Mix blend mode is used, the glow map is ignored. Ideally, it should be possible to use the glow map while the Mix mode is active.
  • The texture is stretched to fit the screen. This is usually fine, but the stretching may look unpleasant at ultrawide aspect ratios. This is especially the case if you're using a texture with a 1:1 aspect ratio as a glowmap (which I did in my testing project). There are two ways to alleviate this:
    • Recommend using a glow map with a 16:9 aspect ratio. This covers the most common aspect ratio without stretching, but it will still appear to stretch noticeably on 21:9 monitors. It'll still look better than a 1:1 glowmap by far 🙂
    • Only use the center of the glow map in a way similar to the CSS background-size: cover property. This means stretching will no longer occur on aspect ratios wider than the glow map, but the glow map will appear zoomed in instead. This is definitely a better choice when using a 21:9 glowmap, but I'm not sure if most developers will bother using a 21:9 glowmap to ensure it doesn't stretch or zoom in (since ultrawide players are not that common). When using a 16:9 glowmap, I'm not sure if this is actually a better choice, but it's something to keep in mind.

Edit: Colored glow maps work too!

image

@Ansraer
Copy link
Contributor Author

Ansraer commented Nov 7, 2021

Fixed the doc typo, mix mode (that was a stupid oversight, no clue how I missed that) and added braces to the if statements.

The texture is stretched to fit the screen. This is usually fine, but the stretching may look unpleasant at ultrawide aspect ratios. This is especially the case if you're using a texture with a 1:1 aspect ratio as a glowmap

In theory I could add another variable to the Params uniform to adjust the UV based on the texture-vs-screen ratio. However, I am not sure if that is an ideal solution. There is other stuff that will change on monitors with unusual resolution and aspect ratios as well, so it might be better if developers write their own logic to swap out the glow map depending on the resolution.
I am fairly certain I already exposed the necessary function to scripting for just that reason.

@Ansraer Ansraer requested review from Calinou and removed request for a team November 7, 2021 21:13
doc/classes/Environment.xml Outdated Show resolved Hide resolved
doc/classes/Environment.xml Outdated Show resolved Hide resolved
@Ansraer
Copy link
Contributor Author

Ansraer commented Nov 7, 2021

Updated the PR with the suggested documentation changes. And yeah, colored glow maps is something I really want to explore myself in the future. I suspect that using three different voronoi id (not distance) noise textures in the rgb channels of a glow map would make for a really trippy effect. Maybe even make those textures tiling and somehow animate them?

@Ansraer Ansraer requested a review from Calinou November 7, 2021 22:11
@akien-mga akien-mga requested a review from a team November 15, 2021 10:47
@akien-mga akien-mga merged commit 58324f4 into godotengine:master Jan 26, 2022
@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.

5 participants