Weak Entity Reference

[EmoGarbage404]

Basic weak entity reference implementation based on ElectroSr and Tayrtahn's implementation from #5577 combined with PJB's review from #6114

Apologies for not preserving git history as my PR process was a bit scattershot and not exactly the cleanest. The majority of this PR is the work of others which I've done my best to correct according to review and my own beliefs.

Weak Entity Reference is simply an EntityUid wrapper that requires the TryGetEntity function to be used to get the contained entity. The check ensures that the referenced entity always exists, hopefully preventing content errors due to deleted references. Additionally, the struct networks as invalid if the referenced entity is no longer valid, preventing errors from attempting to get the metadata component.

[EmoGarbage404]

I would prefer to just have these be private members and then have the TryGetEntity function be on the struct. I think being internal is a good compromise that at least prevents content from accessing the entityUid directly.

[EmoGarbage404]

I'm not sure if this actually checks if nothing caused errors on the client or server. If there's a better way to do this, let me know.

[EmoGarbage404]

Got around to fixing the tests and adding in some missing API stuff (ToPrettyString, resolves, etc.)

Should be good to go now

[PJB3005]

Why do WeakEntityReferences still have a concept of invalid? Especially in the serialization code, from my reading, deserialization of a broken weak entity reference would cause an error.

[EmoGarbage404]

Why do WeakEntityReferences still have a concept of invalid?

Invalid is just meant to serve as the default/uninitialized state of the WeakEntityReference. In the prior PR this was being done with both null as well as invalid, which was kinda unnecessarily confusing. I opted to use Invalid for this value since, in the context of usage of a WeakEntityReference, there really shouldn't be a meaningful distinction between an entity being missing or being unable to be retrieved (deleted).

Especially in the serialization code, from my reading, deserialization of a broken weak entity reference would cause an error.

I've added proper reading for invalid values now. I must've missed it, since the values were properly validated in other parts of the serialization.

Also as an aside I think the github tests exploded for some unrelated reason to the PR.

[FairlySadPanda]

I was interested in this as a comparison so I compared this to Unreal Engine's concept of a TWeakRef, and there their default state is to wrap a nullptr of type Object*, and then you check IsValid() to check if not null and then that it hooks up to a valid object via their implementation's weak reference manager. So a TWeakRef can be "invalid" whilst also not being nullptr.

I took a look and I do think that public bool TryGetEntity(WeakEntityReference weakRef, [NotNullWhen(true)] out EntityUid? entity) looks off. If the internal value can either be EntityUid.Invalid or a reference, then maybe it should be signatured as public bool TryGetEntity(WeakEntityReference weakRef, out EntityUid entity), because otherwise it's introducing nullability rather than orienting consistently around EntityUid.Invalid being empty/invalid.

Thoughts? Happy to be wrong here.

[EmoGarbage404]

I took a look and I do think that public bool TryGetEntity(WeakEntityReference weakRef, [NotNullWhen(true)] out EntityUid? entity) looks off. If the internal value can either be EntityUid.Invalid or a reference, then maybe it should be signatured as public bool TryGetEntity(WeakEntityReference weakRef, out EntityUid entity), because otherwise it's introducing nullability rather than orienting consistently around EntityUid.Invalid being empty/invalid.

Thoughts? Happy to be wrong here.

The TryGetEntity method uses a nullable out parameter to just mirror the general convention of "TryGet" functions which return null when failing. I think this makes using them more intuitive, as it prevents a situation where the TryGetEntity function is handled incorrectly and the invalid EntityUid is passed into some function. Nullables additionally have significantly better linting in C# than our bespoke invalid default value, so I think orienting the API like this results in a much better end user experience.

[FairlySadPanda]

That is true, thanks for the response.