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.

Sign up for GitHub

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

Jump to bottom

Try fix invalid PVS index bug #5422

Merged
merged 9 commits into from
Sep 16, 2024
Merged

Try fix invalid PVS index bug #5422

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8aee15c
Try fix invalid PVS index bug
ElectroJr Sep 3, 2024
182654a
bounds check
ElectroJr Sep 3, 2024
6df7fcc
More Asserts
ElectroJr Sep 3, 2024
a7ba7d0
Merge branch 'master' of https://github.com/space-wizards/RobustToolb…
ElectroJr Sep 3, 2024
8cfbaab
fix assert?
ElectroJr Sep 3, 2024
1bd3557
remove deletion
ElectroJr Sep 3, 2024
945b525
a
ElectroJr Sep 3, 2024
23cb493
A!
ElectroJr Sep 16, 2024
34330ae
Merge branch 'master' of https://github.com/space-wizards/RobustToolb…
ElectroJr Sep 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion RELEASE-NOTES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ Don't change the format without looking at the script!

### Bugfixes

*None yet*
* Fixed a PVS error that could occur when trying to delete the first entity that gets created in a round.

### Other

Expand Down
9 changes: 3 additions & 6 deletions Robust.Server/GameStates/PvsSystem.DataStorage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,8 +245,6 @@ private static void FreeSessionDataMemory(PvsSession session)

private void OnEntityAdded(Entity<MetaDataComponent> entity)
{
DebugTools.Assert(entity.Comp.PvsData.Index == default);

AssignEntityPointer(entity.Comp);
}

Expand All @@ -255,6 +253,7 @@ private void OnEntityAdded(Entity<MetaDataComponent> entity)
/// </summary>
private void AssignEntityPointer(MetaDataComponent meta)
{
DebugTools.Assert(meta.PvsData == PvsIndex.Invalid);
if (_dataFreeListHead == PvsIndex.Invalid)
{
ExpandEntityCapacity();
Expand All @@ -267,8 +266,6 @@ private void AssignEntityPointer(MetaDataComponent meta)
ref var freeLink = ref Unsafe.As<PvsMetadata, PvsMetadataFreeLink>(ref metadata);
_dataFreeListHead = freeLink.NextFree;

// TODO: re-introduce this assert.
// DebugTools.AssertEqual(((PvsMetadata*) ptr)->NetEntity, NetEntity.Invalid);
DebugTools.AssertNotEqual(meta.NetEntity, NetEntity.Invalid);

meta.PvsData = index;
Expand All @@ -287,9 +284,9 @@ private void AssignEntityPointer(MetaDataComponent meta)
private void OnEntityDeleted(Entity<MetaDataComponent> entity)
{
var ptr = entity.Comp.PvsData;
entity.Comp.PvsData = default;
entity.Comp.PvsData = PvsIndex.Invalid;

if (ptr == default)
if (ptr == PvsIndex.Invalid)
return;

_incomingReturns.Add(ptr);
Expand Down
2 changes: 1 addition & 1 deletion Robust.Server/GameStates/PvsSystem.Dirty.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ private void OnEntityAdd(Entity<MetaDataComponent> e)

private void OnEntityDirty(Entity<MetaDataComponent> uid)
{
if (uid.Comp.PvsData != default)
if (uid.Comp.PvsData != PvsIndex.Invalid)
{
ref var meta = ref _metadataMemory.GetRef(uid.Comp.PvsData.Index);
meta.LastModifiedTick = uid.Comp.EntityLastModifiedTick;
Expand Down
2 changes: 1 addition & 1 deletion Robust.Server/GameStates/PvsSystem.Entity.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ private void AssertNullspace(EntityUid uid)

internal void SyncMetadata(MetaDataComponent meta)
{
if (meta.PvsData == default)
if (meta.PvsData == PvsIndex.Invalid)
return;

ref var ptr = ref _metadataMemory.GetRef(meta.PvsData.Index);
Expand Down
9 changes: 9 additions & 0 deletions Robust.Server/GameStates/PvsSystem.Overrides.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
using System.Runtime.InteropServices;
using Robust.Shared.GameObjects;
using Robust.Shared.Timing;
using Robust.Shared.Utility;

namespace Robust.Server.GameStates;

Expand All @@ -25,6 +26,10 @@ private void AddAllOverrides(PvsSession session)
foreach (ref var ent in CollectionsMarshal.AsSpan(_cachedGlobalOverride))
{
ref var meta = ref _metadataMemory.GetRef(ent.Ptr.Index);
DebugTools.AssertEqual(meta.NetEntity, ent.Meta.NetEntity);
DebugTools.AssertEqual(meta.LastModifiedTick, ent.Meta.EntityLastModifiedTick);
DebugTools.AssertEqual(meta.VisMask, ent.Meta.VisibilityMask);
DebugTools.AssertEqual(meta.LifeStage, ent.Meta.EntityLifeStage);
if ((mask & meta.VisMask) == meta.VisMask)
AddEntity(session, ref ent, ref meta, fromTick);
}
Expand All @@ -51,6 +56,10 @@ private void AddForcedEntities(PvsSession session)
foreach (ref var ent in CollectionsMarshal.AsSpan(_cachedForceOverride))
{
ref var meta = ref _metadataMemory.GetRef(ent.Ptr.Index);
DebugTools.AssertEqual(meta.NetEntity, ent.Meta.NetEntity);
DebugTools.AssertEqual(meta.LastModifiedTick, ent.Meta.EntityLastModifiedTick);
DebugTools.AssertEqual(meta.VisMask, ent.Meta.VisibilityMask);
DebugTools.AssertEqual(meta.LifeStage, ent.Meta.EntityLifeStage);
if ((mask & meta.VisMask) == meta.VisMask)
AddEntity(session, ref ent, ref meta, fromTick);
}
Expand Down
6 changes: 5 additions & 1 deletion Robust.Server/GameStates/PvsSystem.ToSendSet.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,10 @@ private void AddPvsChunk(PvsChunk chunk, float distance, PvsSession session)
foreach (ref var ent in span)
{
ref var meta = ref _metadataMemory.GetRef(ent.Ptr.Index);
DebugTools.AssertEqual(meta.NetEntity, ent.Meta.NetEntity);
DebugTools.AssertEqual(meta.LastModifiedTick, ent.Meta.EntityLastModifiedTick);
DebugTools.AssertEqual(meta.VisMask, ent.Meta.VisibilityMask);
DebugTools.AssertEqual(meta.LifeStage, ent.Meta.EntityLifeStage);
if ((mask & meta.VisMask) == meta.VisMask)
AddEntity(session, ref ent, ref meta, fromTick);
}
Expand All @@ -78,7 +82,7 @@ private void AddEntity(PvsSession session, ref PvsChunk.ChunkEntity ent, ref Pvs

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
Member

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 •
edited
Loading

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.

Expand Down
5 changes: 4 additions & 1 deletion Robust.Shared/GameObjects/Components/MetaDataComponent.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ private protected override void ClearTicks()
/// <summary>
/// Offset into internal PVS data.
/// </summary>
internal PvsIndex PvsData;
internal PvsIndex PvsData = PvsIndex.Invalid;
}

[Flags]
Expand Down Expand Up @@ -254,5 +254,8 @@ internal readonly record struct PvsIndex(int Index)
/// An invalid index. This is also used as a marker value in the free list.
/// </summary>
public static readonly PvsIndex Invalid = new PvsIndex(-1);
// TODO PVS
// Consider making 0 an invalid value.
// it prevents default structs from accidentally being used.
}
}
8 changes: 4 additions & 4 deletions Robust.Shared/Utility/ResizableMemoryRegion.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,8 +254,8 @@ public Span<TCast> GetSpan<TCast>() where TCast : unmanaged
public ref T GetRef(int index)
{
// If the memory region is disposed, CurrentSize is 0 and this check always fails.
if (index >= CurrentSize)
ThrowIndexOutOfRangeException();
if (index >= CurrentSize || index < 0)
ThrowIndexOutOfRangeException(CurrentSize, index);

return ref *(BaseAddress + index);
}
Expand Down Expand Up @@ -302,9 +302,9 @@ private static void ThrowNotInitialized()
}

[MethodImpl(MethodImplOptions.NoInlining)]
private static void ThrowIndexOutOfRangeException()
private static void ThrowIndexOutOfRangeException(int size, int index)
{
throw new IndexOutOfRangeException();
throw new IndexOutOfRangeException($"Index was outside the bounds of the memory region. Size: {size}, Index: {index}");
}

private void ReleaseUnmanagedResources()
Expand Down
Loading