-
Notifications
You must be signed in to change notification settings - Fork 32
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.
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
Поправка для спонсорки #303
Conversation
WalkthroughВ данном пулл-реквесте внесены изменения в несколько классов, связанных с управлением персонажами и их загрузками, с акцентом на поддержку функциональности спонсорства. В классе Changes
Sequence Diagram(s)sequenceDiagram
participant Player
participant LobbyUIController
participant LoadoutSystem
participant TraitSystem
participant CharacterRequirementsSystem
Player->>LobbyUIController: Request Character Loadout
LobbyUIController->>LoadoutSystem: ApplyCharacterLoadout(sponsorTier, uuid)
LoadoutSystem->>TraitSystem: OnPlayerSpawnComplete(userId)
TraitSystem->>CharacterRequirementsSystem: CheckRequirementsValid(sponsorTier, uuid)
CharacterRequirementsSystem-->>TraitSystem: Valid/Invalid
LoadoutSystem-->>LobbyUIController: Loadout Applied
LobbyUIController-->>Player: Character Loaded
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 8
🧹 Outside diff range comments (2)
Content.Shared/Clothing/Loadouts/Systems/LoadoutSystem.cs (1)
Line range hint
62-71
: Обновите документацию метода!XML-документация метода не содержит информации о новых параметрах
sponsorTier
иuuid
. Это затруднит использование метода другими разработчиками.Добавьте следующую документацию:
/// <param name="playTimes">Playtime for the player for use with playtime requirements</param> /// <param name="whitelisted">If the player is whitelisted</param> + /// <param name="sponsorTier">The sponsorship tier of the player</param> + /// <param name="uuid">The unique identifier of the player</param> /// <returns>A list of loadout items that couldn't be equipped but passed checks</returns>Content.Client/Preferences/UI/HumanoidProfileEditor.xaml.cs (1)
Line range hint
1420-1425
: Проверка на наличиеLocalUser
может быть упрощенаТекущая проверка инициализирует
uuid
следующим образом:var uuid = _playerManager.LocalUser != null ? _playerManager.LocalUser.ToString() ?? "" : "";Можно упростить код, использовав оператор
?
и??
вместе:var uuid = _playerManager.LocalUser?.ToString() ?? "";Это улучшит читаемость кода.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (7)
- Content.Client/Lobby/LobbyUIController.cs (4 hunks)
- Content.Client/Preferences/ClientPreferencesManager.cs (1 hunks)
- Content.Client/Preferences/UI/HumanoidProfileEditor.xaml.cs (1 hunks)
- Content.Server/Clothing/Systems/LoadoutSystem.cs (3 hunks)
- Content.Server/Traits/TraitSystem.cs (4 hunks)
- Content.Shared/Clothing/Loadouts/Systems/LoadoutSystem.cs (3 hunks)
- Content.Shared/Customization/Systems/CharacterRequirementsSystem.cs (2 hunks)
🔇 Additional comments (12)
Content.Server/Clothing/Systems/LoadoutSystem.cs (3)
10-12
: Правильное использование условной компиляции для директивы using.Хорошее решение использовать условную компиляцию для включения зависимости спонсорского функционала. Это поможет избежать проблем при отключенной функции спонсорства.
46-50
: Проверьте совместимость параметров в вызывающем коде.Убедитесь, что все вызовы метода
ApplyCharacterLoadout
в других частях кода учитывают новые параметры при активированной функции спонсорства.✅ Verification successful
Based on the search results, I can now generate the final response since I have enough information to verify the compatibility of the
ApplyCharacterLoadout
method calls.Все вызовы метода ApplyCharacterLoadout корректно учитывают условную компиляцию для спонсорских параметров
Проверка показала, что все места вызова метода
ApplyCharacterLoadout
правильно обрабатывают дополнительные параметрыsponsorTier
иuuid
при активированной директивеLPP_Sponsors
:
- В Content.Client/Lobby/LobbyUIController.cs
- В Content.Shared/Clothing/Loadouts/Systems/LoadoutSystem.cs
- В Content.Server/Clothing/Systems/LoadoutSystem.cs
Везде используется одинаковая структура условной компиляции
#if LPP_Sponsors
для передачи дополнительных параметров.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Поиск всех вызовов ApplyCharacterLoadout rg "ApplyCharacterLoadout\(" -A 3Length of output: 2622
23-25
: Проверьте инициализацию CheckSponsorSystem.Зависимость помечена как
default!
, что может привести к NullReferenceException, если система не будет правильно инициализирована. Рекомендуется добавить проверку в Initialize().Content.Shared/Customization/Systems/CharacterRequirementsSystem.cs (2)
Line range hint
19-24
: Корректное добавление параметров для спонсорской функциональности!Сигнатуры методов обновлены последовательно с сохранением единообразия параметров sponsorTier и uuid.
Also applies to: 43-48
Line range hint
19-24
: Необходимо проверить использование параметров sponsorTier и uuidДавайте убедимся, что новые параметры корректно передаются во всех местах вызова.
Also applies to: 43-48
✅ Verification successful
Let me analyze the results. I can see that the
CheckRequirementValid
andCheckRequirementsValid
methods are called from multiple places, and there are conditional compilation blocks (#if LPP_Sponsors
) showing proper handling of the sponsor parameters. Let me generate the final response in Russian:Параметры sponsorTier и uuid корректно используются в коде
Проверка показала, что:
- В файле CharacterRequirements.Logic.cs параметры sponsorTier и uuid правильно передаются при условной компиляции с флагом LPP_Sponsors
- Остальные вызовы не требуют изменений, так как параметры опциональные и имеют значения по умолчанию
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Поиск всех вызовов методов CheckRequirementValid и CheckRequirementsValid rg -A 3 "CheckRequirement(s)?Valid\("Length of output: 10720
Content.Client/Preferences/ClientPreferencesManager.cs (2)
88-89
: 🧹 Nitpick (assertive)Проверьте необходимость явного получения сессии
Получение локальной сессии и её использование для валидации профиля выглядит избыточным, так как
_playerManager.LocalSession
уже доступен в условном блоке выше. Рекомендуется переиспользовать существующую переменную для улучшения читаемости кода.Предлагаемые изменения:
- var session = _playerManager.LocalSession!; - profile.EnsureValid(session, collection, allowedMarkings); + profile.EnsureValid(_playerManager.LocalSession!, collection, allowedMarkings);Likely invalid or redundant comment.
Line range hint
1-150
: Рекомендации по архитектуре спонсорской системыТекущая реализация спонсорской системы тесно связана с системой предпочтений персонажа. Рекомендуется рассмотреть следующие архитектурные улучшения:
- Вынести логику работы со спонсорскими маркировками в отдельный сервис
- Использовать систему событий для обновления спонсорских данных
- Добавить кэширование разрешенных маркировок для оптимизации производительности
Давайте проверим текущую архитектуру спонсорской системы:
Content.Shared/Clothing/Loadouts/Systems/LoadoutSystem.cs (2)
43-54
: Корректная реализация перегрузки метода!Добавление опциональных параметров для спонсорства выполнено правильно с использованием условной компиляции. Передача параметров в основную реализацию метода осуществляется корректно.
44-46
: 🧹 Nitpick (assertive)Рекомендация по тестированию условной компиляции
Использование директивы
LPP_Sponsors
усложняет тестирование кода. Рекомендуется добавить модульные тесты для обоих случаев компиляции (с и безLPP_Sponsors
).Проверим использование директивы в других файлах:
Also applies to: 68-70
Content.Client/Lobby/LobbyUIController.cs (1)
19-23
: Корректное добавление зависимостей!Добавление
IPlayerManager
и условная компиляция для спонсорского функционала реализованы согласно принятым практикам.Also applies to: 36-36
Content.Server/Traits/TraitSystem.cs (1)
Line range hint
48-70
: Рекомендуется выделить логику проверки спонсора в отдельный методТекущая реализация смешивает логику проверки спонсора с основной логикой метода. Рекомендуется выделить проверку спонсора в отдельный приватный метод для улучшения читаемости и поддерживаемости кода.
+#if LPP_Sponsors + private (int, string) GetSponsorInfo(ICommonSession player) + { + var sponsorTier = _checkSponsor.CheckUser(player.UserId).Item2 ?? 0; + var uuid = player.UserId.ToString(); + return (sponsorTier, uuid); + } +#endif private void OnPlayerSpawnComplete(PlayerSpawnCompleteEvent args) { #if LPP_Sponsors - var sponsorTier = _checkSponsor.CheckUser(args.Player.UserId).Item2 ?? 0; - var uuid = args.Player.UserId.ToString(); + var (sponsorTier, uuid) = GetSponsorInfo(args.Player); #endifПроверьте обработку null значения спонсорского уровня
При получении уровня спонсора используется null-coalescing оператор для установки значения по умолчанию. Убедитесь, что значение 0 является корректным для неспонсоров.
✅ Verification successful
Let me search for more context about the sponsor check implementation to verify the null handling.
Based on the search results, I can now provide the final response since we have found consistent usage of
CheckUser().Item2 ?? 0
pattern across multiple files in the codebase.Предложенный рефакторинг и проверка обработки null подтверждены
- Выделение логики проверки спонсора в отдельный метод является обоснованным, так как аналогичный код повторяется в нескольких файлах:
- Content.Server/Clothing/Systems/LoadoutSystem.cs
- Content.Server/Players/PlayTimeTracking/PlayTimeTrackingSystem.cs
- Content.Server/Traits/TraitSystem.cs
- Использование
?? 0
для обработки null значений является согласованным паттерном во всех местах использованияCheckUser().Item2
, что подтверждает корректность такого подхода.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Поиск других мест использования CheckUser для проверки согласованности обработки null rg -A 3 "CheckUser.*Item2"Length of output: 1904
Script:
#!/bin/bash # Search for CheckSponsor interface/class definition ast-grep --pattern 'interface ICheckSponsor { $$$ }' # Also search for any class implementing it ast-grep --pattern 'class $_ : $$$ICheckSponsor$$$' # Search for any constants or enums related to sponsor tiers rg -g '*.cs' '(enum|const).*Sponsor'Length of output: 110
Content.Client/Preferences/UI/HumanoidProfileEditor.xaml.cs (1)
1477-1477
: Убедитесь в корректности вызова метода CheckRequirementsValidПри условной компиляции с
#if LPP_Sponsors
в метод_characterRequirementsSystem.CheckRequirementsValid
передаются дополнительные параметры0, sponsorTier, uuid
. Пожалуйста, проверьте, соответствует ли количество и тип аргументов определению этого метода при разных конфигурациях компиляции.
|
||
validation = requirement.IsValid(job, profile, playTimes, whitelisted, prototype, | ||
entityManager, prototypeManager, configManager, | ||
out reason, depth); | ||
out reason, depth | ||
#if LPP_Sponsors | ||
, sponsorTier, uuid | ||
#endif | ||
); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Предлагаю убрать лишние пустые строки
Пустые строки до и после вызова requirement.IsValid
не улучшают читаемость кода.
- FormattedMessage? reason;
-
validation = requirement.IsValid(job, profile, playTimes, whitelisted, prototype,
entityManager, prototypeManager, configManager,
out reason, depth
#if LPP_Sponsors
, sponsorTier, uuid
#endif
);
-
// Set valid to false if the requirement is invalid and not inverted
+ FormattedMessage? reason;
+ validation = requirement.IsValid(job, profile, playTimes, whitelisted, prototype,
+ entityManager, prototypeManager, configManager,
+ out reason, depth
+#if LPP_Sponsors
+ , sponsorTier, uuid
+#endif
+ );
+ // Set valid to false if the requirement is invalid and not inverted
Committable suggestion was skipped due to low confidence.
#if LPP_Sponsors | ||
var sys = IoCManager.Resolve<SponsorsManager>(); | ||
var sponsorTier = 0; | ||
if (sys.TryGetInfo(out var sponsorInfo)) | ||
sponsorTier = sponsorInfo.Tier; | ||
var uuid = _playerManager.LocalUser != null ? _playerManager.LocalUser.ToString() ?? "" : ""; | ||
#endif | ||
if (UpdateClothes) | ||
{ | ||
RemoveDummyClothes(_previewDummy.Value); | ||
if (ShowClothes) | ||
GiveDummyJobClothes(_previewDummy.Value, GetPreferredJob(maybeProfile), maybeProfile); | ||
if (ShowLoadouts) | ||
_loadouts.ApplyCharacterLoadout(_previewDummy.Value, GetPreferredJob(maybeProfile), maybeProfile, | ||
_jobRequirements.GetRawPlayTimeTrackers(), _jobRequirements.IsWhitelisted()); | ||
_jobRequirements.GetRawPlayTimeTrackers(), _jobRequirements.IsWhitelisted() | ||
#if LPP_Sponsors | ||
, sponsorTier, uuid | ||
#endif | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Необходима защита от null при работе с UUID!
В строке 132 возможно возникновение NullReferenceException при вызове ToString()
. Также рекомендуется выделить логику получения спонсорской информации в отдельный метод для улучшения читаемости кода.
Предлагаемые изменения:
- var uuid = _playerManager.LocalUser != null ? _playerManager.LocalUser.ToString() ?? "" : "";
+ var uuid = _playerManager.LocalUser?.ToString() ?? "";
Рекомендуется выделить логику получения спонсорской информации в отдельный метод:
private (int tier, string uuid) GetSponsorInfo()
{
var sponsorTier = 0;
if (IoCManager.Resolve<SponsorsManager>().TryGetInfo(out var sponsorInfo))
sponsorTier = sponsorInfo.Tier;
var uuid = _playerManager.LocalUser?.ToString() ?? "";
return (sponsorTier, uuid);
}
#if LPP_Sponsors | ||
var sys = IoCManager.Resolve<SponsorsManager>(); | ||
var sponsorTier = 0; | ||
if (sys.TryGetInfo(out var sponsorInfo)) | ||
sponsorTier = sponsorInfo.Tier; | ||
var uuid = _playerManager.LocalUser != null ? _playerManager.LocalUser.ToString() ?? "" : ""; | ||
#endif | ||
var job = GetPreferredJob(profile); | ||
GiveDummyJobClothes(dummy, job, profile); | ||
_loadouts.ApplyCharacterLoadout(dummy, job, profile, _jobRequirements.GetRawPlayTimeTrackers(), _jobRequirements.IsWhitelisted()); | ||
_loadouts.ApplyCharacterLoadout(dummy, job, profile, _jobRequirements.GetRawPlayTimeTrackers(), _jobRequirements.IsWhitelisted() | ||
#if LPP_Sponsors | ||
, sponsorTier, uuid | ||
#endif | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Устранить дублирование кода!
Код получения спонсорской информации дублируется в методах UpdateCharacterUI
и GiveDummyJobClothesLoadout
. Это нарушает принцип DRY и усложняет поддержку кода.
После создания предложенного метода GetSponsorInfo()
, замените дублирующийся код:
- var sys = IoCManager.Resolve<SponsorsManager>();
- var sponsorTier = 0;
- if (sys.TryGetInfo(out var sponsorInfo))
- sponsorTier = sponsorInfo.Tier;
- var uuid = _playerManager.LocalUser != null ? _playerManager.LocalUser.ToString() ?? "" : "";
+ var (sponsorTier, uuid) = GetSponsorInfo();
Committable suggestion was skipped due to low confidence.
#if LPP_Sponsors | ||
using Content.Server._LostParadise.Sponsors; | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Проверьте необходимость явного указания default!
Зависимость CheckSponsorSystem
объявлена как private readonly
с явным указанием default!
. В системе внедрения зависимостей Robust.Shared это излишне, так как система DI гарантирует инициализацию всех зависимостей.
- [Dependency] private readonly CheckSponsorSystem _checkSponsor = default!;
+ [Dependency] private readonly CheckSponsorSystem _checkSponsor;
Also applies to: 34-36
Описание обновления:
Поменял некоторые моменты, чтобы спонсорка работала (наверное)
Проверки
🆑 Inari