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

[3.x] Implement glow map effect #93133

Merged
merged 1 commit into from
Sep 24, 2024
Merged

Conversation

Chaosus
Copy link
Member

@Chaosus Chaosus commented Jun 13, 2024

This will make it possible even on 3.x:

glow_map_3 x

Adds glow_map and glow_map_strength properties to Environment and a new method called environment_set_glow_map to VisualServer (I did not add additional parameter environment_set_glow because then we need to add a 14 parameter support to D_METHOD and several other internal method for bindings). Yes, I understand that this in incompatible with 4.x, but it's better than expand the existed bindings too much (If not, I will try to fix it) ?

You can test this change with this asset library template:
изображение

@Chaosus Chaosus requested review from a team as code owners June 13, 2024 15:11
@Chaosus Chaosus added this to the 3.x milestone Jun 13, 2024
@Chaosus Chaosus changed the title Implement glow map effect to 3.x [3.x] Implement glow map effect Jun 13, 2024
@Calinou Calinou removed the topic:2d label Jun 13, 2024
Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

Documentation itself is fine

@lawnjelly
Copy link
Member

This looked good to me from preliminary look last week, however we are in feature freeze for 3.6 so will bump to 3.7 (I don't think we have a 3.7 milestone yet but leaving as 3.x).

@KYRYAMBA
Copy link

KYRYAMBA commented Jul 4, 2024

Great! I hope it will be included in 3.6, there is still a lot of time between beta -> RC1-2 -> release.

@lawnjelly
Copy link
Member

Would be nice to have a review from @Calinou or @clayjohn before merging, as you guys are familiar with the 4.x PR.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally (rebased on top of 3.x 26405db), it works as expected.

Testing project: truck_town_at_night_3.x.zip

image

@lawnjelly
Copy link
Member

@clayjohn

The only thing I'm slightly wary of here is whether the glowmap is having a cost even when not used in the shader. It seems to create the sampler and use an if statement to branch into the texture lookup, even if the texture is not being used.

I don't know whether the shaders on mobile especially will be able to optimize this out. An alternative might be to add a USING_GLOWMAP_DEFINE or some such and only pull in the extra code when in use.

@clayjohn
Copy link
Member

@clayjohn

The only thing I'm slightly wary of here is whether the glowmap is having a cost even when not used in the shader. It seems to create the sampler and use an if statement to branch into the texture lookup, even if the texture is not being used.

I don't know whether the shaders on mobile especially will be able to optimize this out. An alternative might be to add a USING_GLOWMAP_DEFINE or some such and only pull in the extra code when in use.

You can see the impact on various Mali devices using the Mali Shader Compiler. I've set up a demo in ShaderPlayground with this code https://shader-playground.timjones.io/8878ac907e47866464793e70c46da22d. You can just delete the lines to see the impact.

On the default Mali G78, this code doesn't harm "best case performance" which measures the case where the branch fails. It does increase typical case texture fetchs by 25% and texture fetches are the bottleneck here, so I would expect some performance impact.

Looking at older devices (Mali G31, T720), the texture fetches are impacted more, however, on such low end devices, we become instruction bound on the arithmetic unit instead. So the impact of the extra texture might be minimal.

@lawnjelly
Copy link
Member

lawnjelly commented Sep 24, 2024

We decided that #define may be overkill for now, so will merge, and watch for any performance regressions.

Actually, @Chaosus would you be able to rebase just in case as it's been a few months since this was updated?

@Chaosus
Copy link
Member Author

Chaosus commented Sep 24, 2024

Actually, @Chaosus would you be able to rebase just in case as it's been a few months since this was updated?

Done

@lawnjelly lawnjelly merged commit f70472f into godotengine:3.x Sep 24, 2024
14 checks passed
@lawnjelly
Copy link
Member

Thanks!

@Chaosus Chaosus deleted the glow_map_3x branch September 25, 2024 06:06
@lawnjelly lawnjelly modified the milestones: 3.x, 3.7 Oct 27, 2024
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.

6 participants