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

Vulkan: Toggling the SSAO Half Size option in Project Settings causes memory leak #63202

Closed
xiaowangxu opened this issue Jul 19, 2022 · 12 comments · Fixed by #65738
Closed

Vulkan: Toggling the SSAO Half Size option in Project Settings causes memory leak #63202

xiaowangxu opened this issue Jul 19, 2022 · 12 comments · Fixed by #65738

Comments

@xiaowangxu
Copy link

Godot version

v4.0.alpha11.official [afdae67]

System information

Windows 11, Vulkan, Nvida RTX 2080 Ti

Issue description

when ssao is enabled and turn off the ssao/half_size option in Project Setting will cause memory leak
turn on half_size:
HalfSizeOn
turn off half_size:
HalfSize
HalfSizeOff

Steps to reproduce

Open any scene in godot. Enable SSAO in the WorldEnvironment, and toggle the ssao/half_size option in Project Setting and you will see the memory difference in the debugger

Minimal reproduction project

SSAO_Bug.zip

@xiaowangxu xiaowangxu changed the title turn off the ssao/half_size option in Project Setting will cause memory leak [Godot 4] turn off the ssao/half_size option in Project Setting will cause memory leak Jul 19, 2022
@clayjohn
Copy link
Member

Does the memory usage continue increasing each frame or does it only increase once and then stay static?

@Calinou Calinou changed the title [Godot 4] turn off the ssao/half_size option in Project Setting will cause memory leak Vulkan: Toggling the SSAO Half Size option in Project Settings causes memory leak Jul 19, 2022
@SmartySmart702
Copy link

SmartySmart702 commented Jul 19, 2022

Does the memory usage continue increasing each frame or does it only increase once and then stay static?

Tested for ~3 minutes and it increased continuously.

@SmartySmart702
Copy link

SmartySmart702 commented Jul 19, 2022

Same for SSIL.
Even for this:
AO: on, half size on
SSIL: off, half size off

Note that SSIL is disabled but "Half Size" on for SSIL still causes a memory leak (because AO is enabled?)

@Calinou
Copy link
Member

Calinou commented Jul 19, 2022

It might be worth checking whether #62478 fixes this.

cc @BastiaanOlij

@BastiaanOlij
Copy link
Contributor

Hmmm, yeah would be good to check if my PR already deals with it or else see if I can fix it in there as any fix will probably be lost due to the merge conflicts not picking this up properly.

That said, the code for allocating and freeing buffers used by these effects was all over the place and should be more readable in my PR. It should be easier to check and see if we forget to free something.

@allenwp
Copy link
Contributor

allenwp commented Jul 27, 2022

I just tested with revision 0a420c4, which should include @BastiaanOlij's PR, on a local debug build of the editor and was able to reproduce the memory leak with the minimal reproduction project. The leak happened when either SSAO or SSIL was enabled. When both were disabled, no measurable memory leak was shown in he Debugger Monitor.

@BastiaanOlij
Copy link
Contributor

Thanks @allenwp , I'll have a closer look as soon as I can, most of my PR was restructuring, not changing how it works but it should be easier now to compare allocations and whether they are being freed up.

@BastiaanOlij
Copy link
Contributor

Not sure if this could be related to this issue, it may be a flow on effect, but I couldn't find any memory leak in SSAO (yet, I'll do more testing).
I did find a leak in VoxelGI: #64139 which could be triggered as SSAO is triggered.

@BastiaanOlij
Copy link
Contributor

Can this be re-tested on the latest master?

@allenwp
Copy link
Contributor

allenwp commented Sep 12, 2022

@BastiaanOlij This time I tested using a release_debug build, built with MSVS 14.3 x86_64 and the latest official alpha release.

Memory leak reproduced using the reproduction project that is attached to initial comment of this issue on 0a420c4, v4.0.alpha16.official [86dd3f3], and 79b21e9 (latest master).

@Calinou Calinou added this to the 4.0 milestone Sep 12, 2022
@Calinou Calinou moved this to To Assess in 4.x Priority Issues Sep 12, 2022
@BastiaanOlij
Copy link
Contributor

Thanks Allen, I'm going to try and see if I can reproduce it at this end.

@BastiaanOlij
Copy link
Contributor

I think I fixed it, see PR #65738

Repository owner moved this from To Assess to Done in 4.x Priority Issues Sep 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

6 participants