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

Remove InstanceId when Scene Despawn #12778

Merged
merged 15 commits into from Mar 30, 2024
Merged

Remove InstanceId when Scene Despawn #12778

merged 15 commits into from Mar 30, 2024

Conversation

ghost
Copy link

@ghost ghost commented Mar 29, 2024

Objective

Solution

  • spawned_scenes field was never used, and I removed it
  • Add a component remove hook for Handle<DynamicScene>, and when the Handle<DynamicScene> component is removed, delete the corresponding InstanceId from spawned_dynamic_scenes

Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@Brezak
Copy link
Contributor

Brezak commented Mar 29, 2024

Fixes one leak. We still leak in spawned_instances. You'll have to pass the instance id into SceneSpawner::despawn_instance.

@ghost
Copy link
Author

ghost commented Mar 29, 2024

Fixes one leak. We still leak in spawned_instances. You'll have to pass the instance id into SceneSpawner::despawn_instance.

If calling SceneSpawner::despawn_instance, the user removing the Handle<DynamicScene> component would cause the entity to be despawned, and I'm not sure if this is appropriate.

@Brezak
Copy link
Contributor

Brezak commented Mar 29, 2024

It shouldn't. despawn_instance will eventually through despawn_queued_instances despawn the scene instance. It shouldn't despawn the parent entity itself. If you're removing the scene handle from an entity you're likely trying to despawn all the entities in said scene.

@ghost
Copy link
Author

ghost commented Mar 29, 2024

It shouldn't. despawn_instance will eventually through despawn_queued_instances despawn the scene instance. It shouldn't despawn the parent entity itself. If you're removing the scene handle from an entity you're likely trying to despawn all the entities in said scene.

Additionally, if the user calls despawn_recursive, it could lead to a repeated despawn action, resulting in a warning.

@Brezak
Copy link
Contributor

Brezak commented Mar 29, 2024

It shouldn't. despawn_instance will eventually through despawn_queued_instances despawn the scene instance. It shouldn't despawn the parent entity itself. If you're removing the scene handle from an entity you're likely trying to despawn all the entities in said scene.

Additionally, if the user calls despawn_recursive, it could lead to a repeated despawn action, resulting in a warning.

Then just remove the instance from spawned_instances. Don't forget to do the same for normal scenes.

@matiqo15 matiqo15 added C-Bug An unexpected or incorrect behavior A-Scenes Serialized ECS data stored on the disk labels Mar 29, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 30, 2024
@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Mar 30, 2024
@alice-i-cecile alice-i-cecile added this to the 0.13.2 milestone Mar 30, 2024
Comment on lines +71 to +72
if let Some(&SceneInstance(scene_instance)) = world.get::<SceneInstance>(entity) {
let Some(mut scene_spawner) = world.get_resource_mut::<SceneSpawner>() else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Extremely minor nit, but is there a reason that one of these is if let and the other one is let ... else, instead of both being let ... else?

Copy link
Author

Choose a reason for hiding this comment

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

Looks like using let ... else consistently would be better. The reason for the inconsistency between the two is that in a later commit, I changed resource_mut to get_resource_mut, but I didn't notice that the if let expression needed to correspond to it.

Merged via the queue into bevyengine:main with commit cdecd39 Mar 30, 2024
28 checks passed
@ghost ghost deleted the fix-#12746 branch March 31, 2024 15:28
mockersf pushed a commit that referenced this pull request Apr 1, 2024
# Objective

- Fix #12746
- When users despawn a scene, the `InstanceId` within `spawned_scenes`
and `spawned_dynamic_scenes` is not removed, causing a potential memory
leak

## Solution

- `spawned_scenes` field was never used, and I removed it
- Add a component remove hook for `Handle<DynamicScene>`, and when the
`Handle<DynamicScene>` component is removed, delete the corresponding
`InstanceId` from `spawned_dynamic_scenes`
@mockersf
Copy link
Member

mockersf commented Apr 1, 2024

removing from the 0.13.2 milestone as this use components hooks from #10756 which are not available in the 0.13

@mockersf mockersf removed this from the 0.13.2 milestone Apr 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Scenes Serialized ECS data stored on the disk C-Bug An unexpected or incorrect behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Memory Leak When Loading Scenes
7 participants