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

Try fix invalid PVS index bug #5422

Merged
merged 9 commits into from
Sep 16, 2024

Conversation

ElectroJr
Copy link
Member

Some code was using the default value of PvsIndex as the invalid value, even though the 0th index actually gets assigned to entities. I think zero used to be invalid previously and the checks were just never updated once that changed?

Comment on lines 82 to 88

if (meta.LifeStage >= EntityLifeStage.Terminating)
{
Log.Error($"Attempted to send deleted entity: {ToPrettyString(ent.Uid)}, lifestage is {meta.LifeStage}.\n{Environment.StackTrace}");
Log.Error($"Attempted to send deleted entity: {ToPrettyString(ent.Uid)}, Meta lifestage: {ent.Meta.EntityLifeStage}, PVS lifestage: {meta.LifeStage}.\n{Environment.StackTrace}");
EntityManager.QueueDeleteEntity(ent.Uid);
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I mentioned in a conversation with PJB that the previous version of this removed the entity from the PvsSession's working set instead of actually calling delete on the entity. It's not clear to me what the consequences of that are. I suspect it's incidental to the current PVS issue though.

QueueDeleteEntity eventually just calls DeleteEntity which contains:

public virtual void DeleteEntity(EntityUid? uid)
   ...
if (meta.EntityLifeStage >= EntityLifeStage.Deleted)
	return;

if (meta.EntityLifeStage == EntityLifeStage.Terminating)
{
	var msg = $"Called Delete on an entity already being deleted. Entity: {ToPrettyString(e)}";
	_sawmill.Error($"{msg}. Trace: {Environment.StackTrace}");
}

If you're getting the Attempted to send deleted entity error and you see entities that aren't supposed to be being deleted that are being deleted do you also see this sawmill error from DeleteEntity? It would be further down in the logs since it's running at the end of the frame.

Might help confirm if there really is a desync between the value of ent.Meta.EntityLifeStage and meta.LifeStage.

Copy link
Member Author

@ElectroJr ElectroJr Sep 3, 2024

Choose a reason for hiding this comment

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

If you're getting the Attempted to send deleted entity error and you see entities that aren't supposed to be being deleted that are being deleted do you also see this sawmill error from DeleteEntity? It would be further down in the logs since it's running at the end of the frame.

Smug is the only one that has been running into these errors so far. and those errors did appear earlier, but I don't know if they're still happening https://discord.com/channels/310555209753690112/900426319433728030/1275994976114835468

Trying to re-delete the terminating entity was just added in the hopes of maybe improving exception tolerance in case it somehow manages to actually delete the entity and fix it. Ideally that code shouldn't ever actually need to run, its a sign that something else has already gone wrong somewhere else. Though looking at the code again, it shouldn't even be doing this because this code gets run in parallel.

@ElectroJr ElectroJr merged commit f81e30a into space-wizards:master Sep 16, 2024
3 checks passed
@ElectroJr ElectroJr deleted the pvs-index-invalid branch September 16, 2024 04:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants