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

[Port] Universal Upgrader #950

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
+78 −0
Open

[Port] Universal Upgrader #950

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
601905b
Create test
sanek31 Nov 26, 2024
9fad5b9
Add files via upload
sanek31 Nov 26, 2024
05e7e7c
Update UPSystem.cs
sanek31 Nov 30, 2024
489a054
Update UPComponent.cs
sanek31 Nov 30, 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
21 changes: 21 additions & 0 deletions Content.Server/_Special/UniversalUpgrader/Components/UPComponent.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@

namespace Content.Server._Special.UniversalUpgrader.Components;

[RegisterComponent]
public sealed partial class UPComponent : Component
{
[DataField, ViewVariables(VVAccess.ReadWrite)]
public string upgradeName;

[DataField, ViewVariables(VVAccess.ReadWrite)]
public string componentName;

[DataField, ViewVariables(VVAccess.ReadWrite)]
public float upgradeValue;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Требуется валидация полей и улучшение типобезопасности

Поля 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.

Suggested change
[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;


[DataField, ViewVariables(VVAccess.ReadWrite)]
public string ProtoWhitelist;

Comment on lines +16 to +18
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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;
}
Comment on lines +19 to +21
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Добавьте проверку значения 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.

Suggested change
[DataField, ViewVariables(VVAccess.ReadWrite)]
public int usable = 0;
}
/// <summary>
/// Количество оставшихся использований улучшения.
/// </summary>
[DataField]
[ViewVariables(VVAccess.ReadWrite)]
public int usable = 1;
}

48 changes: 48 additions & 0 deletions Content.Server/_Special/UniversalUpgrader/Systems/UPSystem.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@

using Content.Server._Special.UniversalUpgrader.Components;
using Content.Shared.Interaction;

namespace Content.Server._Special.UniversalUpgrader.Systems;

public sealed class UPSystem : EntitySystem
{

[Dependency] private readonly EntityManager _ent = default!;
public override void Initialize()
{
base.Initialize();
SubscribeLocalEvent<UPComponent, AfterInteractEvent>(OnInteract);
}

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);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WTF?

Copy link
Contributor Author

@sanek31 sanek31 Nov 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WTF?

GetType переводит строку в объект с которым можно работать, пример из видео:

_ent.GetComponent<Type.GetType("Conter.Shared.Weapons.Melee.MeleeWeaponComponent, Content.Shared")>(target);

равносилен

_ent.GetComponent<MeleeWeaponComponent>(target);

а

comp.GetType().GetField("AttackRate");

равносилен

comp.AttackRate;

Возможно есть простой способ, который я не нашёл

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ты мне говоришь что такое IComponentFactory? -_-


if (g != null && _ent.TryGetComponent(target, g, out var comp))
{

var h = comp.GetType().GetField(entity.Comp.upgradeName);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WTF?


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Небезопасное использование рефлексии

Прямое использование Type.GetType и GetField без валидации может привести к уязвимостям безопасности. Необходимо:

  1. Добавить белый список разрешенных компонентов
  2. Проверять namespace компонентов
  3. Валидировать имена полей

Пример безопасной реализации:

- 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 (h != null)
{

if (h.FieldType == typeof(int)) h.SetValue(comp,(int) entity.Comp.upgradeValue);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WTF?

if (h.FieldType == typeof(float)) h.SetValue(comp,(int) entity.Comp.upgradeValue);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Небезопасное приведение типов

Текущая реализация может привести к потере данных при конвертации:

  1. Для float всегда используется приведение к int
  2. Отсутствует проверка диапазона значений

Предлагаемое исправление:

- 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.

Suggested change
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}");


}
entity.Comp.usable -= 1;
if (entity.Comp.usable < 0) _ent.QueueDeleteEntity(entity);

}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Оптимизация производительности и обработка ошибок

Текущая реализация использует рефлексию при каждом взаимодействии, что может негативно влиять на производительность. Также отсутствует обработка исключений.

Рекомендации:

  1. Кэшировать информацию о типах и полях
  2. Добавить обработку исключений
  3. Логировать ошибки

Пример реализации:

+ 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.


}
1 change: 1 addition & 0 deletions Content.Server/_Special/test
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
1