tonemap.glsl and tonemap_mobile.glsl seem to have many duplicate code. Should there be a tonemap_inc.glsl? which can make it easier for other PRs (like #106940) to rebase.

No, I intentionally duplicated the code since I expect these shaders to diverge even more as time goes on. Trying to share code between them is what led us to leaving a huge amount of performance on the table. This is a case where having optimized code for each platform is worthwhile and saves us literally milliseconds per frame.

tonemap.glsl and tonemap_mobile.glsl seem to have many duplicate code. Should there be a tonemap_inc.glsl? which can make it easier for other PRs (like #106940) to rebase.

No, I intentionally duplicated the code since I expect these shaders to diverge even more as time goes on. Trying to share code between them is what led us to leaving a huge amount of performance on the table. This is a case where having optimized code for each platform is worthwhile and saves us literally milliseconds per frame.

Why aren't specialization constants an option for this case? I suspect that specialization constants will be added at some point in the near future anyway to address performance issues from branching, even if we are using two entirely separate shaders. (In the past, I've seen a measurable performance gain when using the Linear tonemapper from simply turning:

	if (params.tonemapper == TONEMAPPER_LINEAR) {
		return color;
	} else if (params.tonemapper == TONEMAPPER_REINHARD) {
		return tonemap_reinhard(max(vec3(0.0f), color), white);
	} else if (params.tonemapper == TONEMAPPER_FILMIC) {
		return tonemap_filmic(max(vec3(0.0f), color), white);
	} else if (params.tonemapper == TONEMAPPER_ACES) {
		return tonemap_aces(max(vec3(0.0f), color), white);
	} else { // TONEMAPPER_AGX
		return tonemap_agx(color);
	}

into:

		return color;

(which is exactly how this would compile with specialization constants, if I understand correctly)

tonemap.glsl and tonemap_mobile.glsl seem to have many duplicate code. Should there be a tonemap_inc.glsl? which can make it easier for other PRs (like #106940) to rebase.

No, I intentionally duplicated the code since I expect these shaders to diverge even more as time goes on. Trying to share code between them is what led us to leaving a huge amount of performance on the table. This is a case where having optimized code for each platform is worthwhile and saves us literally milliseconds per frame.

Why aren't specialization constants an option for this case?

Whoops, I hadn't looked into this PR thoroughly enough: I see now that specialization constants were added AND tonemap.glsl was split into two.

I presume there's a substantial reason that specialization constants weren't an option for optimizations that prompted splitting tonemap.glsl into two files?

With the Mobile renderer, the first level seems to add a lot of jaggies. This is especially the case with HDR 2D enabled:

Settings	Image

MRP: glow-test.zip

I'm struggling to review the code changes of this PR because I'm unfamiliar with both the existing and new glow gathering code. This is a "me" problem and not a problem with this PR.

...But I mention it, because it leads me to some thoughts that I figure are worth mentioning just in case it's helpful:

This PR seems to be a combination of:

• Swapping out the Mobile glow gather implementation and changing how the rendering passes work in relation to this
• Separating tonemap.glsl into two files
• Adding specialization constants

This is pie-in-the-sky thinking, but an alternative approach may be to add specialization constants to the tonemap process in a separate PR. This PR could explore specialization constants for both Forward+ and Mobile on different mobile and desktop hardware.

There is also a part of the Godot PR process that I struggle with in general: although a single commit is best for merging, it makes things harder to review for a person not very familiar with the code when a PR combines multiple changes, like this one.

Anyway, not sure if my thoughts are helpful. I understand people only have so much time to work on these things and there is some efficiency to be gained in the development process by bulking multiple changes into one commit. I'm not sure if the extra complexity during the review process balances that or if there is risk introduced by bugs that couldn't be caught during review due to the extra complexity of the single commit during the review process. (This is likely a discussion that is beyond this PR?)

Oh! I've been so focused on finding any outstanding issues that I failed to voice my positive thoughts about this PR:

• Changes the way that glow is gathered and passes are handled on Mobile to minimize texture reads and greatly improve performance on devices that can't read these textures quickly: 👍👍😄😄
• Adds specialization constants to resolve issues with insane amounts of branching that saturates ALUs: 👍👍😄😄

These changes in this PR are extremely valuable and I'm really looking forward to having these merged in! It will be a game changer in performance on mobile!

With the Mobile renderer, the first level seems to add a lot of jaggies. This is especially the case with HDR 2D enabled

MRP: glow-test.zip

What do people think of getting rid of Level 1 for Mobile and Forward+ altogether? Even if it's not as expensive on desktop GPUs, I suspect it still has a cost that almost nobody benefits from?

Just spit-balling ideas here. Other than this, I'm not sure how this issue can be addressed...

Here's a quick comparison of this PR with Godot 4.5 for all levels, 1 through 7:

Mobile HDR 2D (this PR)	Mobile HDR 2D (4.5)	Forward+ HDR 2D (4.5)

Mobile (this PR)	Mobile (4.5)	Forward+ (4.5)

* Note: #110671 will help fix some of the issues shown when HDR 2D is disabled.

The tool is fully automated, so it's easy to generate more images. Takes no time at all to try other configurations, just ask if you want to see.

This test does highlight that level 1 is the only regression in this PR...

This test does highlight that level 1 is the only regression in this PR...

I don't intend to make any improvements to glow level 1. I searched Godot projects on Github and couldn't find any that relied on level 1. In practice it is kind of useless because the kernel is so narrow that it doesn't look like anything. Ironically level 1 is the most expensive to compute because it is reading from the highest res source, and writing out the highest number of pixels.

What this PR does to avoid that cost is simply downsample instead of blur level 1. So what you are comparing is a raw downsampled image to one that has already had an expensive layer of blur applied. They are not intended to match at all

What this PR does to avoid that cost is simply downsample instead of blur level 1. So what you are comparing is a raw downsampled image to one that has already had an expensive layer of blur applied. They are not intended to match at all

This makes sense and matches my understanding.

This is why I suggested we maybe remove level 1 entirely, since it adds jaggies and doesn't work as expected. To do this, I guess we could hide level 1 in the editor and force its intensity to always 0.0, even if a script tries to set it to something else?

My thought is that if level 1 is left in this state, it will both appear as a bug and also introduce risk of games being made to look worse if upgraded from a previous version that uses level 1 in the blur effect.

There is also a possibility that in some scenes it looks acceptable, but other scenes show the jaggies... So glow is made more difficult to use because jaggies might appear in scenes other than the one the glow was initially configured for. An example is when you have rectangular, axis-aligned objects in your setup scene that only rotate off axis in other scenes.

(Said differently, I'm concerned that this level 1 behaviour produces a bad user experience.)

This is why I suggested we maybe remove level 1 entirely, since it adds jaggies and doesn't work as expected. To do this, I guess we could hide level 1 in the editor and force its intensity to always 0.0, even if a script tries to set it to something else?

I have no problem doing that!

This is why I suggested we maybe remove level 1 entirely, since it adds jaggies and doesn't work as expected. To do this, I guess we could hide level 1 in the editor and force its intensity to always 0.0, even if a script tries to set it to something else?

I have no problem doing that!

Let's go with that approach then! One other wrinkle is how the "normalize" function works: level 1 should not contribute to the calculation of normalized levels, in case it's been set through script or by a resource that was saved with an earlier version of Godot. I think that should handle all the edge cases to make this a good user experience...

Thanks!

I just finished bisecting and it looks like this PR broke foveated rendering with OpenXR and the Vulkan Mobile renderer. The project I'm testing doesn't have glow enabled, so it's probably some random collateral damage, although, from looking at the diff I'm not entirely sure the cause yet

@dsnopek Could it be a hardcoded framebuffer format somewhere?

No difference for me. Still a poor performance on mobile.

But more importantly, if you look at it overlaid on actual game content with multiple layers the difference is much more apparent. The jaggies line up better with the underlying geometry and blend into the other layers better. On larger geometry its not even noticeable at reasonable settings.

Ah, I think I might have misunderstood the change that happened between the Oct 10 and Oct 24 versions of this PR. The change that was made was to do with how level 1 was blended with the rest and not to do with the generation of the level 1 texture?

If you swap between your images you can see a huge difference!

This is the part where I think the misunderstanding presented itself, as there is only a minor difference between the two versions of the PR:

Mobile (this PR Oct 10)	Mobile (this PR Oct 24)

But more importantly, if you look at it overlaid on actual game content with multiple layers the difference is much more apparent. The jaggies line up better with the underlying geometry and blend into the other layers better. On larger geometry its not even noticeable at reasonable settings.

Ah, I think I might have misunderstood the change that happened between the Oct 10 and Oct 24 versions of this PR. The change that was made was to do with how level 1 was blended with the rest and not to do with the generation of the level 1 texture?

Cleared this up in the rendering meeting: it was the change of threshold default that was supposed to help with some scenes' jaggies, but this change of default has been reverted so we're back where we started with jaggies generated from level 1.

clayjohn and I both don't have strong feelings regarding the behaviour of level 0 after this PR, so we're leaving it as-is and see if people report it as a serious usability issue.