[Port] Universal Upgrader #950
Conversation
WalkthroughВ данном запросе на изменение добавлены два новых класса: Changes
Sequence Diagram(s)sequenceDiagram
participant Player
participant UPSystem
participant Entity
participant Component
Player->>UPSystem: Взаимодействие с сущностью
UPSystem->>Entity: Проверка валидности
UPSystem->>Component: Получение имени компонента
UPSystem->>Component: Установка значения обновления
UPSystem->>UPComponent: Уменьшение счетчика usable
alt usable < 0
UPSystem->>Entity: Удаление сущности
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🔇 Additional comments (7)Content.Server/_Special/UniversalUpgrader/Components/UPComponent.cs (3)
Исправьте именование поля Поле
Проверьте значение поля Поле
Добавьте проверки на Поля Content.Server/_Special/UniversalUpgrader/Systems/UPSystem.cs (4)
Добавьте валидацию имени компонента перед использованием Использование
Оптимизируйте разбиение строк и проверьте соответствие длины массивов Методы Примените следующий дифф для исправления: - var t = entity.Comp.upgradeName.Split(' ').Length;
+ var upgradeNames = entity.Comp.upgradeName.Split(' ');
+ var upgradeValues = entity.Comp.upgradeValue.Split(' ');
+ if (upgradeNames.Length != upgradeValues.Length)
+ {
+ // Обработка ошибки или выход из метода
+ Logger.Error("Количество элементов в upgradeName и upgradeValue не совпадает.");
+ return;
+ }
- for (int i = 0; i < t; i++)
+ for (int i = 0; i < upgradeNames.Length; i++)
{
- var un = entity.Comp.upgradeName.Split(' ')[i];
- var uv = entity.Comp.upgradeValue.Split(' ')[i];
+ var un = upgradeNames[i];
+ var uv = upgradeValues[i];
Исправьте использование метода Метод Примените следующий дифф для исправления: - h.SetValue(h.FieldType, uv );
+ var convertedValue = Convert.ChangeType(uv, h.FieldType);
+ h.SetValue(comp, convertedValue);
Добавьте обработку ошибок и улучшите безопасность при использовании рефлексии Прямое использование рефлексии без валидации может привести к уязвимостям и исключениям. Рекомендуется:
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (4)
Content.Server/_Special/UniversalUpgrader/Components/UPComponent.cs (2)
4-6: Добавьте документацию к компонентуНеобходимо добавить XML-документацию, описывающую назначение компонента и его использование.
Предлагаемые изменения:
[RegisterComponent] +/// <summary> +/// Компонент, отвечающий за функциональность универсального улучшения предметов. +/// Позволяет модифицировать другие компоненты в соответствии с заданными параметрами. +/// </summary> public sealed partial class UPComponent : Component
1-21: Рекомендации по архитектуре компонентаТекущая реализация может быть улучшена для обеспечения большей надежности и расширяемости.
Рекомендации:
- Рассмотрите использование enum для определения типов улучшений вместо строковых полей
- Добавьте события для отслеживания изменений состояния компонента
- Реализуйте интерфейс ISerializationHooks для валидации данных при десериализации
Content.Server/_Special/UniversalUpgrader/Systems/UPSystem.cs (2)
42-43: Некорректная логика удаления сущностиУдаление сущности происходит сразу после достижения отрицательного значения usable, что может привести к проблемам синхронизации.
Рекомендуется:
- Добавить проверку на неотрицательное значение при инициализации
- Использовать DeleteEntity вместо QueueDeleteEntity для немедленного удаления
1-48: Архитектурные рекомендацииТекущая реализация универсального апгрейдера имеет несколько архитектурных проблем:
- Отсутствует синхронизация с клиентом
- Нет валидации значений обновления
- Нет системы отката изменений
- Отсутствует интерфейс IUpgradeable для компонентов
Рекомендуется:
- Добавить SharedUPSystem для синхронизации
- Внедрить интерфейс IUpgradeable для компонентов
- Реализовать систему валидации значений
- Добавить события до/после обновления
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
Content.Server/_Special/UniversalUpgrader/Components/UPComponent.cs(1 hunks)Content.Server/_Special/UniversalUpgrader/Systems/UPSystem.cs(1 hunks)Content.Server/_Special/test(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- Content.Server/_Special/test
🔇 Additional comments (1)
Content.Server/_Special/UniversalUpgrader/Systems/UPSystem.cs (1)
7-15: Структура класса соответствует шаблонам SS14
Базовая структура реализована правильно, включая:
- Наследование от EntitySystem
- Внедрение зависимости EntityManager
- Подписку на событие AfterInteractEvent
| [DataField, ViewVariables(VVAccess.ReadWrite)] | ||
| public string ProtoWhitelist; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Исправьте именование и добавьте документацию для ProtoWhitelist
Поле ProtoWhitelist нарушает конвенцию именования (Pascal Case для публичных полей).
Предлагаемые изменения:
- public string ProtoWhitelist;
+ /// <summary>
+ /// Список разрешенных прототипов для улучшения.
+ /// </summary>
+ [DataField]
+ [ViewVariables(VVAccess.ReadWrite)]
+ public string? protoWhitelist;Committable suggestion skipped: line range outside the PR's diff.
| [DataField, ViewVariables(VVAccess.ReadWrite)] | ||
| public int usable = 0; | ||
| } |
There was a problem hiding this comment.
Добавьте проверку значения usable
Поле usable инициализируется нулем, что может привести к неправильному поведению компонента.
Рекомендуемые изменения:
- public int usable = 0;
+ /// <summary>
+ /// Количество оставшихся использований улучшения.
+ /// </summary>
+ [DataField]
+ [ViewVariables(VVAccess.ReadWrite)]
+ public int usable = 1;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [DataField, ViewVariables(VVAccess.ReadWrite)] | |
| public int usable = 0; | |
| } | |
| /// <summary> | |
| /// Количество оставшихся использований улучшения. | |
| /// </summary> | |
| [DataField] | |
| [ViewVariables(VVAccess.ReadWrite)] | |
| public int usable = 1; | |
| } |
| [DataField, ViewVariables(VVAccess.ReadWrite)] | ||
| public string upgradeName; | ||
|
|
||
| [DataField, ViewVariables(VVAccess.ReadWrite)] | ||
| public string componentName; | ||
|
|
||
| [DataField, ViewVariables(VVAccess.ReadWrite)] | ||
| public float upgradeValue; |
There was a problem hiding this comment.
Требуется валидация полей и улучшение типобезопасности
Поля upgradeName, componentName и upgradeValue не имеют проверок на null и отрицательные значения.
Рекомендуемые изменения:
- public string upgradeName;
+ [DataField(required: true)]
+ public string upgradeName = default!;
- public string componentName;
+ [DataField(required: true)]
+ public string componentName = default!;
- public float upgradeValue;
+ [DataField(required: true)]
+ [ViewVariables(VVAccess.ReadWrite)]
+ public float upgradeValue = 1.0f;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [DataField, ViewVariables(VVAccess.ReadWrite)] | |
| public string upgradeName; | |
| [DataField, ViewVariables(VVAccess.ReadWrite)] | |
| public string componentName; | |
| [DataField, ViewVariables(VVAccess.ReadWrite)] | |
| public float upgradeValue; | |
| [DataField(required: true), ViewVariables(VVAccess.ReadWrite)] | |
| public string upgradeName = default!; | |
| [DataField(required: true), ViewVariables(VVAccess.ReadWrite)] | |
| public string componentName = default!; | |
| [DataField(required: true), ViewVariables(VVAccess.ReadWrite)] | |
| public float upgradeValue = 1.0f; |
| if (h.FieldType == typeof(int)) h.SetValue(comp,(int) entity.Comp.upgradeValue); | ||
| if (h.FieldType == typeof(float)) h.SetValue(comp,(int) entity.Comp.upgradeValue); |
There was a problem hiding this comment.
Небезопасное приведение типов
Текущая реализация может привести к потере данных при конвертации:
- Для float всегда используется приведение к int
- Отсутствует проверка диапазона значений
Предлагаемое исправление:
- if (h.FieldType == typeof(int)) h.SetValue(comp,(int) entity.Comp.upgradeValue);
- if (h.FieldType == typeof(float)) h.SetValue(comp,(int) entity.Comp.upgradeValue);
+ if (h.FieldType == typeof(int))
+ h.SetValue(comp, Convert.ToInt32(entity.Comp.upgradeValue));
+ else if (h.FieldType == typeof(float))
+ h.SetValue(comp, Convert.ToSingle(entity.Comp.upgradeValue));
+ else
+ Logger.Warning($"Unsupported field type {h.FieldType}");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (h.FieldType == typeof(int)) h.SetValue(comp,(int) entity.Comp.upgradeValue); | |
| if (h.FieldType == typeof(float)) h.SetValue(comp,(int) entity.Comp.upgradeValue); | |
| if (h.FieldType == typeof(int)) | |
| h.SetValue(comp, Convert.ToInt32(entity.Comp.upgradeValue)); | |
| else if (h.FieldType == typeof(float)) | |
| h.SetValue(comp, Convert.ToSingle(entity.Comp.upgradeValue)); | |
| else | |
| Logger.Warning($"Unsupported field type {h.FieldType}"); |
| private void OnInteract(Entity<UPComponent> entity, ref AfterInteractEvent args) | ||
| { | ||
| if (!args.CanReach || args.Target is not { Valid: true } target) | ||
| return; | ||
| if (entity.Comp.ProtoWhitelist != null && HasComp<MetaDataComponent>(target)) | ||
| { | ||
| var z = _ent.GetComponent<MetaDataComponent>(target); | ||
| if (z.EntityPrototype!.ID != entity.Comp.ProtoWhitelist) | ||
| return; | ||
| } | ||
|
|
||
| Type? g = Type.GetType(entity.Comp.componentName); | ||
|
|
||
| if (g != null && _ent.TryGetComponent(target, g, out var comp)) | ||
| { | ||
|
|
||
| var h = comp.GetType().GetField(entity.Comp.upgradeName); | ||
|
|
||
| if (h != null) | ||
| { | ||
|
|
||
| if (h.FieldType == typeof(int)) h.SetValue(comp,(int) entity.Comp.upgradeValue); | ||
| if (h.FieldType == typeof(float)) h.SetValue(comp,(int) entity.Comp.upgradeValue); | ||
|
|
||
| } | ||
| entity.Comp.usable -= 1; | ||
| if (entity.Comp.usable < 0) _ent.QueueDeleteEntity(entity); | ||
|
|
||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Оптимизация производительности и обработка ошибок
Текущая реализация использует рефлексию при каждом взаимодействии, что может негативно влиять на производительность. Также отсутствует обработка исключений.
Рекомендации:
- Кэшировать информацию о типах и полях
- Добавить обработку исключений
- Логировать ошибки
Пример реализации:
+ private readonly Dictionary<string, (Type type, FieldInfo field)> _componentCache = new();
+ private bool TryGetComponentInfo(string componentName, string fieldName,
+ out (Type type, FieldInfo field) info)
+ {
+ var key = $"{componentName}.{fieldName}";
+ if (_componentCache.TryGetValue(key, out info))
+ return true;
+
+ try
+ {
+ // ... кэширование информации о компоненте
+ return true;
+ }
+ catch (Exception e)
+ {
+ Logger.Error($"Failed to get component info: {e}");
+ info = default;
+ return false;
+ }
+ }Committable suggestion skipped: line range outside the PR's diff.
| Type? g = Type.GetType(entity.Comp.componentName); | ||
|
|
||
| if (g != null && _ent.TryGetComponent(target, g, out var comp)) | ||
| { | ||
|
|
||
| var h = comp.GetType().GetField(entity.Comp.upgradeName); | ||
|
|
There was a problem hiding this comment.
Небезопасное использование рефлексии
Прямое использование Type.GetType и GetField без валидации может привести к уязвимостям безопасности. Необходимо:
- Добавить белый список разрешенных компонентов
- Проверять namespace компонентов
- Валидировать имена полей
Пример безопасной реализации:
- Type? g = Type.GetType(entity.Comp.componentName);
+ if (!ValidateComponentName(entity.Comp.componentName))
+ return;
+ Type? g = Type.GetType($"Content.Server.{entity.Comp.componentName}");Committable suggestion skipped: line range outside the PR's diff.
| if (g != null && _ent.TryGetComponent(target, g, out var comp)) | ||
| { | ||
|
|
||
| var h = comp.GetType().GetField(entity.Comp.upgradeName); |
| if (h != null) | ||
| { | ||
|
|
||
| if (h.FieldType == typeof(int)) h.SetValue(comp,(int) entity.Comp.upgradeValue); |
| return; | ||
| } | ||
|
|
||
| Type? g = Type.GetType(entity.Comp.componentName); |
There was a problem hiding this comment.
WTF?
GetType переводит строку в объект с которым можно работать, пример из видео:
_ent.GetComponent<Type.GetType("Conter.Shared.Weapons.Melee.MeleeWeaponComponent, Content.Shared")>(target);
равносилен
_ent.GetComponent<MeleeWeaponComponent>(target);
а
comp.GetType().GetField("AttackRate");
равносилен
comp.AttackRate;
Возможно есть простой способ, который я не нашёл
There was a problem hiding this comment.
ты мне говоришь что такое IComponentFactory? -_-
Rxup
added
DONT MERGE
|
Спасибо, не знал про это. Перепишу |
Инструкцию по использованию скину чуть позже
universalupgrader.mp4
Summary by CodeRabbit
UPComponent, который позволяет управлять и применять улучшения к сущностям.UPSystem, обрабатывающая взаимодействия с сущностями, обладающими компонентомUPComponent.