Cleans up tag system

[Tornado-Technology]

About the PR

Clean code and update the tag system

Why / Balance

All checks now take place only in Debug build, which will reduce the load during game and tag operations. Deprecated methods have been removed, as well as their usage, method signatures have been updated for convenient usage. Removed garbage overloads

Technical details

All overloads for IEnumerable methods have been removed for their uselessness. Also the function accepts only ProtoId<TagPrototype>, not a string. The system now does not create the component state itself, it now does so via AutoGenerateComponentState

Media

• I have added screenshots/videos to this PR showcasing its changes ingame, or this PR does not require an ingame showcase

[ElectroJr]

The main reason for having the single-parameter override was to avoid having the method allocate a new array unnecessarily. Unless there's some analyser or something else to prevent people from misusing it, I'd rather keep these overrides.

Similarly, IIRC the list & array specific overrides exist because they are faster than the overrides that use IEnumerables. I don't recall if I benchmarked them or not, but I think they existed for a reason. Tags get checked often enough that performance probably matters,

[Tornado-Technology]

Does casting a class to the IEnumerable type create anything? Almost any list implements an iteration method of this interface, and when we cast an instance to the IEnumerable interface, we limit the ability to interact with the class to only the methods and properties described in IEnumerable. I don't think the type conversion operation consumes a lot of resources.

[Tornado-Technology]

Ok, I seem to get the point, but what prevents us from using HasTag, because we still input a collection, and it simply will not cause an overload even if it has one element?

[ElectroJr]

Ok, I seem to get the point, but what prevents us from using HasTag, because we still input a collection, and it simply will not cause an overload even if it has one element?

I'd be fine with just using hasTag if HasTags were given a debug assert that checks that people don't accidentally use it when passing in only one tag (which they were doing, which is why the override exists). But I don't think theres a way to differentiate between that and someone just passing in an actual array that happens to only have one element.

Having the override doesn't hurt anything, its only a few lines of codes and stops some trivial mistakes from accidentally slowing things down a bit.

Does casting a class to the IEnumerable type create anything?

Using IEnumerable and uneccesarily using params [] methods allocates and slows things down. IIRC for IEnumerable its because it has to create an enumerator class. For example, running these benchmarks ElectroJr/RobustToolbox@fd62c6d yields:

Method	Mean	Error	StdDev	Ratio	Gen0	Allocated	Alloc Ratio
HasTagsArray	21.39 ns	0.129 ns	0.121 ns	1.00	-	-	NA
HasTagsList	21.70 ns	0.087 ns	0.081 ns	1.01	-	-	NA
HasTagsIEnumerableArray	29.08 ns	0.099 ns	0.093 ns	1.36	0.0038	32 B	NA
HasTagsIEnumerableList	30.13 ns	0.098 ns	0.092 ns	1.41	0.0048	40 B	NA

Method	Mean	Error	StdDev	Ratio	Gen0	Allocated	Alloc Ratio
HasTag	10.35 ns	0.043 ns	0.040 ns	1.00	-	-	NA
HasTagsParams	16.23 ns	0.105 ns	0.082 ns	1.57	0.0038	32 B	NA

They're generally 1.3-1.6 times slower and allocate. In most cases it usually doesn't matter and I wouldn't be picky, but I think that tags get used frequently enough by other systems that it would make a not insignificant difference. E.g., there are systems that iterate over many entities and check for some tags, or things like InRangeUnobstructed() which does some tag checks.

It might be premature optimization, but I'd rather just keep the faster methods around. Though they look like bloat & duplicate code, they're not really hard to maintain and just help make things a bit faster.

[ElectroJr]

To clarify, this really only matters for the has-tag methods. Add/remove tags don't get used often enough for it to matter

[deltanedas]

for these will ProtoId have a compile time / initialization stage error if you do this or will it just silently have an invalid id

[mirrorcult]

yes, electro made a linter pass for static protoid variables

[Tornado-Technology]

/// <summary>
/// This attribute should be used on static string or string collection fields to validate that they correspond to
/// valid YAML prototype ids. This attribute is not required for static <see cref="ProtoId{T}"/> and
/// <see cref="EntProtoId"/> fields, as they automatically get validated.
/// </summary>
[AttributeUsage(AttributeTargets.Field)]
public sealed class ValidatePrototypeIdAttribute<T> : Attribute where T : IPrototype
{
}

[ElectroJr]

AFAICT should be fine to merge after re-adding some List<ProtoId<TagPrototype>> overrides to avoid IEnumerables

[Partmedia]

According to git bisect, this PR broke air alarms not alarming.

[osjarw]

According to git bisect, this PR broke air alarms not alarming.

Not sure if there's better place to mention this..
This cast seems to be failing on master.

space-station-14/Content.Shared/DeviceNetwork/NetworkPayload.cs

Lines 6 to 24 in b0b76a1

	public sealed class NetworkPayload : Dictionary<string, object?>
	{
	/// <summary>
	/// Tries to get a value from the payload and checks if that value is of type T.
	/// </summary>
	/// <typeparam name="T">The type that should be casted to</typeparam>
	/// <returns>Whether the value was present in the payload and of the required type</returns>
	public bool TryGetValue<T>(string key, [NotNullWhen(true)] out T? value)
	{
	if (this.TryCastValue(key, out T? result))
	{
	value = result;
	return true;
	}
	value = default;
	return false;
	}
	}

called from line 89

space-station-14/Content.Server/Atmos/Monitor/Systems/AtmosAlarmableSystem.cs

Lines 81 to 94 in b0b76a1

	private void OnPacketRecv(EntityUid uid, AtmosAlarmableComponent component, DeviceNetworkPacketEvent args)
	{
	if (component.IgnoreAlarms) return;
	if (!EntityManager.TryGetComponent(uid, out DeviceNetworkComponent? netConn))
	return;
	if (!args.Data.TryGetValue(DeviceNetworkConstants.Command, out string? cmd)
	|| !args.Data.TryGetValue(AlertSource, out HashSet<string>? sourceTags))
	{
	return;
	}
	var isValid = sourceTags.Any(source => component.SyncWithTags.Contains(source));

https://github.com/space-wizards/space-station-14/pull/28272/files#diff-cec0ecbf90ebef61830db8b0fb530d66d855cf94262cb6d8e11e2fac42e0bb32L11-R10

Replacing that HashSet<string> with HashSet<ProtoId<TagPrototype>> seems to fix the alarms, I think I can try making it into a PR if I can't find any other big issues relating to it

[Partmedia]

Thanks for the investigation and proposed fix. Would you mind putting that into a PR and linking it here? Thanks!