Combine predicted deletion - #7059
maciejwalendziuk wants to merge 5 commits into
Conversation
| while (QueuedDeletions.TryDequeue(out var uid)) | ||
| { | ||
| // The deletion may have been canceled since it was queued. | ||
| if (!QueuedDeletionsSet.Remove(uid)) | ||
| continue; | ||
|
|
There was a problem hiding this comment.
This would already be modifying the queue which in itself should not be touched so remove? At best make it a debug assert but these should always match 1-1.
There was a problem hiding this comment.
they are intentionally not 1-1 anymore. ClearPredictedDeletion cancels a queued deletion by removing it from the set (since we cant remove from a queue) and this check discards the stale queue entry. otherwise a rolled back deletion redetaches the entity after state application restores it - assert would fire on every rollback
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| [ProxyFor(typeof(EntityManager), nameof(EntityManager.PredictedDeleteEntity))] | ||
| [Obsolete("Use Del")] | ||
| protected void PredictedDel(Entity<MetaDataComponent?, TransformComponent?> ent) |
There was a problem hiding this comment.
The ProxyFor is still useful to keep the interim I would think, just direct it at the new method?
There was a problem hiding this comment.
Del already has ProxyFor(DeleteEntity) and the analyzer will just suggest the first matching proxy so pointing it at DeleteEntity could make DeleteEntity calls warn "use PredictedDel". PredictedDeleteEntity callers still get the obsolete warning pointing at DeleteEntity. I could be missing something though...
| meta.Flags &= ~MetaDataFlags.Detached; | ||
| _entities.ClearPredictedDeletion(uid); | ||
| if (isEnteringPvs) |
There was a problem hiding this comment.
If anything is touching entities outside of PVS it should really be an assert instead.
There was a problem hiding this comment.
its the rollback for predicted deletions. PredictedDetachNetworkedEntity sets Detached flag and when prediction reset doesnt run (for example prediction is disabled) reentry here is the only restore path. assert would fire on every one of those
There was a problem hiding this comment.
That still doesn't explain why a nullspace entity would ever be predicted deleted?
|
|
||
| await client.WaitPost(() => | ||
| { | ||
| var entMan = (ClientEntityManager) client.EntMan; | ||
| EntityUid? raised = null; | ||
| void OnQueueDeleted(EntityUid uid) => raised = uid; | ||
|
|
||
| entMan.EntityQueueDeleted += OnQueueDeleted; | ||
| try | ||
| { | ||
| entMan.QueueDeleteEntity(clientTarget); | ||
| } | ||
| finally | ||
| { | ||
| entMan.EntityQueueDeleted -= OnQueueDeleted; | ||
| } | ||
|
|
||
| // Physics relies on this to purge contacts before the entity gets detached. | ||
| Assert.That(raised, Is.EqualTo(clientTarget)); | ||
| }); |
2 participants
DeleteEntityandQueueDeleteEntitynow predict deletions of networked entities by detaching them to nullspace which is whatPredictedDeleteEntityandPredictedQueueDeleteEntitydid. They're obsolete aliases now along with thePredictedDelandPredictedQueueDelTryQueueDeleteEntityignoring the predicted deletion queues and returning true for entities that were already going awayClient-side
DelandQueueDelon a networked entity used to log an error and do nothing - now it detaches. The obsoletion warnings cover thePredictedDel->Delrename but not this so anything that was quietly relying on the noop will start visibly deleting for a tickI hope that is what metalgearsloth meant.
Solves: #7043