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

Memory leak when using get_last_slide_collision() #57073

Open
BeayemX opened this issue Jan 22, 2022 · 3 comments
Open

Memory leak when using get_last_slide_collision() #57073

BeayemX opened this issue Jan 22, 2022 · 3 comments

Comments

@BeayemX
Copy link
Contributor

BeayemX commented Jan 22, 2022

Godot version

v4.0.dev.custom_build [b5f524d]

System information

Linux Mint - Vulkan API 1.2.131 - Using Vulkan Device #0: NVIDIA - NVIDIA GeForce GTX 1080

Issue description

Using get_last_slide_collision() causes object to be created but never deleted.

This can be seen in the object count in the profiler.

Steps to reproduce

Use a CharacterBody2D and use get_last_slide_collision() when a collision happenes.
Check with Performance.get_monitor(Performance.OBJECT_COUNT) or the profiler in the editor that the object count is only increasing.

OR

Run the attached project to see the issue with everything prepared.

Minimal reproduction project

  1. The player (white square) will fall and collide with the static collider (white rectangle).
  2. In the UI you can see the current number of existing objects
  3. Press space to jump
  4. Notice that the number of objects will not increase as long as the player is NOT colliding with the ground

MemoryLeak_get_last_slide_collision.zip

@markdibarry
Copy link
Contributor

I'm guessing the root of the problem is right here where we create a new copy of the MotionResult and never free old ones.
image

@LeaoLuciano
Copy link
Contributor

GET_INSTRUCTION_ARG(ret, argc + 1);
VariantInternal::initialize(ret, Variant::OBJECT);
Object **ret_opaque = VariantInternal::get_object(ret);
method->ptrcall(base_obj, argptrs, ret_opaque);
VariantInternal::object_assign(ret, *ret_opaque); // Set so ID is correct too.

Looking to backtraces on gdb, ret_opaque is a pointer to a Ref<KinematicCollision2D> that references count increases in method->ptrcall(base_obj, argptrs, ret_opaque); but isn't unreferenced therefore. The references count also increases in VariantInternal::object_assign(ret, *ret_opaque);, but is unferenced at the end of the gdscript scope:
for (int i = 0; i < _stack_size; i++) {
stack[i].~Variant();
}
.

@YuriSizov YuriSizov modified the milestones: 4.0, 4.1 Feb 27, 2023
@YuriSizov YuriSizov modified the milestones: 4.1, 4.2 Jun 22, 2023
@Lazy-Rabbit-2001
Copy link
Contributor

Lazy-Rabbit-2001 commented Oct 26, 2023

Sorry, but I don't know if it's fittable to talk it here:
I opened a test project with a CharacterBody2D (with a template script for CharacterBody2D) and a TileMap, and no get_last_slide_collision() used (even no get_slide_collision()), but the amount of objects got increased every time i made the body collide with a piece of tile(Caution: it's A PIECE of tile, not the whole tile)

I think a probable fix is to clear all old elements in Vector<Ref<KinematicCollision2D>> slide_colliders in the beginning part of code of move_and_slide()

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

No branches or pull requests

7 participants