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

In use-after-despawn situation, make despawning system name available #2949

Closed
wants to merge 3 commits into from

Conversation

payload
Copy link
Contributor

@payload payload commented Oct 10, 2021

This draft sketches a debugging help in a situation where a command like Insert acts upon an entity which was already despawned. When the command is applied it will panic with the message

thread 'main' panicked at 'Could not add a component (of type `alloc::string::String`) to entity 0v0 because it doesn't exist in this World.
If this command was added to a newly spawned entity, ensure that you have not despawned that entity within the same stage.
This may have occurred due to system order ambiguity, or if the spawning system has multiple command buffers

This tells which component (String) should have been added to which Entity (0v0) together with a hint that some system order dependency exist and is off.
To debug a situation like this one question would be "Which system was despawning this entity?" but this question might not be easy to answer.

A related issue and PR is

Solution

The panic message should contain the name of the system which despawned the entity, so when debugging I can immediately jump to this system, schedule it in a different stage or delay the despawn:

thread 'main' panicked at 'Could not add a component (of type `alloc::string::String`) to entity 0v0 because it doesn't exist in this World.
Entity was despawned in use_after_despawn::do_despawn.
If this command was added to a newly spawned entity, ensure that you have not despawned that entity within the same stage.
This may have occurred due to system order ambiguity, or if the spawning system has multiple command buffers'

In this case use_after_despawn::do_despawn is the fully qualified name of the system do_despawn.

The implementation extents Commands with a system name (if it is a static str).
When Despawn (or DespawnRecursive) is added to the queue the struct carries on this system name.
When Despawn::write is run, it is setting some crude system name state on the world.
When EntityRef::despawn is run, it looks up the system name from the world and adds this entity and this name to a World::despawned vec.
This vec is cleared at the beginning of every stage (ignoring use cases without stages).
When a panic happens due to a missing entity during a Command::write, you can now look up the entity in the World::despawned vec and put the system name in the panic message.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Oct 10, 2021
std::borrow::Cow::Borrowed(s) => s,
std::borrow::Cow::Owned(_) => "owned",
};
Commands::new(state, world).with_name(name)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should just pass the Cow here 🐄 . It is in both cases cheap, right?

std::borrow::Cow::Borrowed(s) => s,
std::borrow::Cow::Owned(_) => "owned",
};
Commands::new(state, world).with_name(name)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe it would be more appropriate to pass the whole SystemMeta, but well, I can't figure out the lifetime specifiers. And maybe the lifetime is not even guaranteed since the Despawn command, lands in a queue and the later the SystemMeta would land in some kind of buffer for the whole stage.

}

impl Command for Despawn {
fn write(self, world: &mut World) {
println!("despawn from {}", self.name);
world.set_system_name(Some(self.name));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This set name thing is crude. I am sorry. It may could be a separate despawn* function on world instead which takes the despawn origin name.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed S-Needs-Triage This issue needs to be labelled labels Oct 11, 2021
@alice-i-cecile
Copy link
Member

Closing as #3851 was merged :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants