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.

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]Health Analyzer update #925

Merged
merged 6 commits into from
Nov 21, 2024
Merged

[PORT]Health Analyzer update #925

merged 6 commits into from
Nov 21, 2024

Conversation

trest100
Copy link
Contributor

@trest100 trest100 commented Nov 19, 2024

Описание PR

Обновляет Анализатор здоровья, теперь можно тыкать и смотреть повреждения определенных конечностей

Медиа

image

Тип PR

  • Feature
  • Fix
  • Tweak
  • Balance
  • Refactor
  • Port
  • Translate
  • Resprite

Изменения
🆑 Trest100

  • tweak: Обновляет Анализатор здоровья, теперь можно тыкать и смотреть повреждения определенных конечностей
    Feat Mocho

Summary by CodeRabbit

Release Notes

  • New Features

    • Enhanced health analyzer interface with new buttons for body part selection, improving interactivity.
    • Added a return button for easier navigation within the health analyzer window.
    • Introduced functionality to select and analyze specific body parts, enhancing user experience.
  • Bug Fixes

    • Improved error handling and lifecycle management for the health analyzer window.
  • Documentation

    • Updated Russian localization for health analyzer messages, simplifying text display for bleeding status.
    • Added localization entry for the new return button in the health analyzer window.

@trest100 trest100 requested a review from Rxup as a code owner November 19, 2024 20:25
Copy link
Contributor

coderabbitai bot commented Nov 19, 2024

Walkthrough

The changes involve significant enhancements to the HealthAnalyzer functionality across various components. The HealthAnalyzerBoundUserInterface class has been refactored to improve event handling and body part selection. The HealthAnalyzerWindow.xaml file has been updated with new UI elements and layout adjustments, while the corresponding code-behind file has introduced new event handling and button interactions. The server-side HealthAnalyzerSystem has been modified to better manage body part analysis. Additionally, message classes have been updated to include body part information, and localization files have been adjusted for improved user interface text.

Changes

File Path Change Summary
Content.Client/HealthAnalyzer/UI/HealthAnalyzerBoundUserInterface.cs Refactored Open method with object initializer; added event handler subscriptions for _window; added SendBodyPartMessage method; enhanced Dispose method for event unsubscription; modified imports.
Content.Client/HealthAnalyzer/UI/HealthAnalyzerWindow.xaml Increased MinWidth of FancyWindow from 300 to 350; added ReturnButton and several TextureButton controls for body parts; adjusted layout and sizes of UI elements; removed commented-out buttons; reformatted ScanModeLabel.
Content.Client/HealthAnalyzer/UI/HealthAnalyzerWindow.xaml.cs Introduced OnBodyPartSelected event; added _bodyPartControls dictionary; modified SetActiveBodyPart and ResetBodyPart methods; updated Populate method for entity management; refined alert handling and display logic.
Content.Server/Medical/HealthAnalyzerSystem.cs Updated Initialize method to subscribe to HealthAnalyzerPartMessage; modified Update method for body part validation; added OnHealthAnalyzerPartSelected method; adjusted BeginAnalyzingEntity and UpdateScannedUser methods.
Content.Shared/MedicalScanner/HealthAnalyzerScannedUserMessage.cs Added nullable Part property to HealthAnalyzerScannedUserMessage; updated constructor; introduced new HealthAnalyzerPartMessage class with Owner and BodyPart properties.
Resources/Locale/ru-RU/medical/components/health-analyzer-component.ftl Updated health-analyzer-window-entity-bleeding-text to remove color formatting, simplifying the message.
Resources/Locale/ru-RU/medical/components/medical-scanner-component.ftl Added new localization entry for health-analyzer-window-return-button-text with value < НАЗАД.

Poem

In the land of health and care,
A window opens, bright and fair.
Buttons for each part to see,
An analyzer's joy, oh what glee!
With messages clear, and colors bright,
We scan and heal, all day and night. 🐇✨


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions github-actions bot added Changes: UI Can be reviewed or fixed by people who are knowledgeable with UI design Changes: Localization Can be reviewed or fixed by people who are knowledgeable with translation labels Nov 19, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 10

🧹 Outside diff range and nitpick comments (5)
Content.Shared/MedicalScanner/HealthAnalyzerScannedUserMessage.cs (1)

20-20: Consider improving property name and documentation.

The property name Part is quite vague. Consider renaming it to something more descriptive like SelectedBodyPart or TargetBodyPart to better reflect its purpose. Also, add XML documentation to explain its usage.

-    public NetEntity? Part; // backmen: surgery
+    /// <summary>
+    /// The currently selected body part entity for detailed analysis.
+    /// </summary>
+    public NetEntity? SelectedBodyPart;
Content.Client/HealthAnalyzer/UI/HealthAnalyzerBoundUserInterface.cs (1)

41-41: Consider breaking down the long line for better readability

-        private void SendBodyPartMessage(TargetBodyPart? part, EntityUid target) => SendMessage(new HealthAnalyzerPartMessage(EntMan.GetNetEntity(target), part ?? null));
+        private void SendBodyPartMessage(TargetBodyPart? part, EntityUid target)
+        {
+            var netEntity = EntMan.GetNetEntity(target);
+            var message = new HealthAnalyzerPartMessage(netEntity, part ?? null);
+            SendMessage(message);
+        }
Content.Client/HealthAnalyzer/UI/HealthAnalyzerWindow.xaml (2)

17-20: Consider enhancing ReturnButton accessibility and clarity

While the button is properly localized, consider adding:

  1. A tooltip to clarify its purpose
  2. Accessibility properties for screen readers
 <Button Name="ReturnButton"
         Text="{Loc 'health-analyzer-window-return-button-text'}"
         Margin="0 0 0 10"
-        HorizontalExpand="False"/>
+        HorizontalExpand="False"
+        ToolTip="{Loc 'health-analyzer-window-return-button-tooltip'}"
+        AccessKeyHint="{Loc 'health-analyzer-window-return-button-key'}"
+        Help="{Loc 'health-analyzer-window-return-button-help'}"/>

67-91: Remove commented-out code for eyes and mouth buttons

If these features are planned for future implementation, they should be tracked in separate issues rather than kept as commented code.

Content.Client/HealthAnalyzer/UI/HealthAnalyzerWindow.xaml.cs (1)

80-80: Remove unused parameter 'button' from 'SetActiveBodyPart' method.

The parameter TextureButton button in SetActiveBodyPart is not used within the method. Removing it simplifies the method signature and reduces unnecessary code.

Apply this diff to remove the unused parameter:

- public void SetActiveBodyPart(TargetBodyPart part, TextureButton button)
+ public void SetActiveBodyPart(TargetBodyPart part)
 {
     if (_target == null)
         return;

     // Temporary workaround until Groin prototypes are available.
     OnBodyPartSelected?.Invoke(part == TargetBodyPart.Groin ? TargetBodyPart.Torso : part, _target.Value);
 }

...

- bodyPartButton.Value.OnPressed += _ => SetActiveBodyPart(bodyPartButton.Key, bodyPartButton.Value);
+ bodyPartButton.Value.OnPressed += _ => SetActiveBodyPart(bodyPartButton.Key);

Also applies to: 86-93

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between d67b2d3 and 09d13a7.

📒 Files selected for processing (7)
  • Content.Client/HealthAnalyzer/UI/HealthAnalyzerBoundUserInterface.cs (3 hunks)
  • Content.Client/HealthAnalyzer/UI/HealthAnalyzerWindow.xaml (2 hunks)
  • Content.Client/HealthAnalyzer/UI/HealthAnalyzerWindow.xaml.cs (9 hunks)
  • Content.Server/Medical/HealthAnalyzerSystem.cs (11 hunks)
  • Content.Shared/MedicalScanner/HealthAnalyzerScannedUserMessage.cs (2 hunks)
  • Resources/Locale/ru-RU/medical/components/health-analyzer-component.ftl (1 hunks)
  • Resources/Locale/ru-RU/medical/components/medical-scanner-component.ftl (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • Resources/Locale/ru-RU/medical/components/medical-scanner-component.ftl
🔇 Additional comments (8)
Content.Shared/MedicalScanner/HealthAnalyzerScannedUserMessage.cs (2)

Line range hint 22-31: LGTM! Constructor changes look good.

The constructor modification is well-implemented with proper optional parameter handling and maintains backward compatibility.


35-41: 🛠️ Refactor suggestion

Add documentation and verify C# feature compatibility.

The new message class needs XML documentation to explain its purpose and usage. Also, document when null values are expected for the properties.

+/// <summary>
+/// Message sent when a specific body part is selected in the Health Analyzer interface.
+/// </summary>
+/// <param name="owner">The entity that owns the selected body part. Null if no entity is selected.</param>
+/// <param name="bodyPart">The selected body part. Null if no part is selected.</param>
 [Serializable, NetSerializable]
 public sealed class HealthAnalyzerPartMessage(NetEntity? owner, TargetBodyPart? bodyPart) : BoundUserInterfaceMessage

Let's verify the C# language version used in the project:

Content.Client/HealthAnalyzer/UI/HealthAnalyzerBoundUserInterface.cs (2)

43-56: LGTM! Well-implemented disposal logic

The disposal implementation follows best practices:

  • Proper event handler cleanup
  • Appropriate null checks
  • Correct disposal pattern

25-26: Verify system-side event handling

Let's ensure that the system properly handles these UI events.

✅ Verification successful

Based on the results, I can generate the final response as we have sufficient information about the system-side handling of these UI events.

UI events are properly handled on the system side

The verification confirms that the UI events are correctly implemented:

  • The HealthAnalyzerPartMessage is properly defined in the shared code
  • The server-side system (HealthAnalyzerSystem.cs) subscribes to and handles the message via OnHealthAnalyzerPartSelected
  • The client-side UI properly sends the message through SendBodyPartMessage
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for handlers of HealthAnalyzerPartMessage
rg -l "HealthAnalyzerPartMessage" | while read -r file; do
  echo "=== $file ==="
  rg -A 5 "HealthAnalyzerPartMessage" "$file"
done

# Search for related system implementations
ast-grep --pattern 'class $_ : EntitySystem
{
  $$$
  HandleMessage($_, HealthAnalyzerPartMessage $_)
  {
    $$$
  }
  $$$
}'

Length of output: 1901

Content.Client/HealthAnalyzer/UI/HealthAnalyzerWindow.xaml (4)

Line range hint 1-16: LGTM: Window size adjustment supports new UI elements

The increased MinWidth provides adequate space for the new body part selection interface.


199-208: LGTM: Status display implementation

The grid layout is well-structured and all text is properly localized.


212-220: LGTM: Alerts section implementation

The alerts container is properly structured with appropriate visibility controls.


27-168: Verify style classes for body part buttons

The implementation looks good, but please verify that all referenced style classes exist:

  • TargetDollButtonRightArm
  • TargetDollButtonRightHand
  • TargetDollButtonHead
  • TargetDollButtonChest
  • TargetDollButtonGroin
  • TargetDollButtonLeftLeg
  • TargetDollButtonLeftFoot
  • TargetDollButtonRightLeg
  • TargetDollButtonRightFoot
  • TargetDollButtonLeftArm
  • TargetDollButtonLeftHand
✅ Verification successful

All style classes are properly defined

All the style classes used in the HealthAnalyzerWindow.xaml are correctly defined in StyleNano.cs:

  • TargetDollButtonHead
  • TargetDollButtonChest
  • TargetDollButtonGroin
  • TargetDollButtonLeftArm
  • TargetDollButtonLeftHand
  • TargetDollButtonRightArm
  • TargetDollButtonRightHand
  • TargetDollButtonLeftLeg
  • TargetDollButtonLeftFoot
  • TargetDollButtonRightLeg
  • TargetDollButtonRightFoot

Each style class is properly implemented with hover states and corresponding texture assets.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if all required style classes are defined in the stylesheet

# Search for style class definitions in XAML and CSS files
echo "Searching for style class definitions..."
rg -l "TargetDollButton(Right|Left)(Arm|Hand|Leg|Foot)|TargetDollButton(Head|Chest|Groin)" \
   --type xaml --type css

Length of output: 235


Script:

#!/bin/bash
# Let's try a broader search without file type restrictions since xaml isn't recognized
echo "Searching for style class definitions..."
rg -l "TargetDollButton"

# Also search for any .xaml files to understand the styling pattern
echo -e "\nSearching for XAML files..."
fd ".xaml$"

# Search for any style-related files that might contain these definitions
echo -e "\nSearching for style-related files..."
fd -i "style|theme"

Length of output: 21054


Script:

#!/bin/bash
# Let's check the StyleNano.cs file since it was found in the search and likely contains the style definitions
echo "Checking StyleNano.cs for TargetDollButton styles..."
rg "TargetDollButton" Content.Client/Stylesheets/StyleNano.cs -A 5 -B 5

Length of output: 4047

@@ -12,7 +12,7 @@ health-analyzer-window-entity-damage-total-text = Общие поврежден
health-analyzer-window-damage-group-text = { $damageGroup }: { $amount }
health-analyzer-window-damage-type-text = { $damageType }: { $amount }
health-analyzer-window-entity-unrevivable-text = [color=red]Обнаружено уникальное строение тела! Пациент не может быть реанимирован стандартными средствами![/color]
health-analyzer-window-entity-bleeding-text = [color=red]У пациента кровотечение![/color]
health-analyzer-window-entity-bleeding-text = У пациента кровотечение!
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

Restore color formatting for consistency and better visibility.

The bleeding status is a critical health condition that should be visually emphasized. Other critical messages in this file (e.g., unrevivable-text) use red color formatting to highlight important medical warnings.

Apply this diff to restore the color formatting:

-health-analyzer-window-entity-bleeding-text = У пациента кровотечение!
+health-analyzer-window-entity-bleeding-text = [color=red]У пациента кровотечение![/color]
📝 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
health-analyzer-window-entity-bleeding-text = У пациента кровотечение!
health-analyzer-window-entity-bleeding-text = [color=red]У пациента кровотечение![/color]

Comment on lines +21 to +27
_window = new HealthAnalyzerWindow
{
Title = EntMan.GetComponent<MetaDataComponent>(Owner).EntityName,
};
_window.OnClose += Close;
_window.OnBodyPartSelected += SendBodyPartMessage;
_window.OpenCentered();
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

Add null check for EntityManager

The EntMan usage could potentially throw a null reference exception. Consider adding a null check or using the null-conditional operator.

 _window = new HealthAnalyzerWindow
 {
-    Title = EntMan.GetComponent<MetaDataComponent>(Owner).EntityName,
+    Title = EntMan?.GetComponent<MetaDataComponent>(Owner)?.EntityName ?? "Health Analyzer",
 };
📝 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
_window = new HealthAnalyzerWindow
{
Title = EntMan.GetComponent<MetaDataComponent>(Owner).EntityName,
};
_window.OnClose += Close;
_window.OnBodyPartSelected += SendBodyPartMessage;
_window.OpenCentered();
_window = new HealthAnalyzerWindow
{
Title = EntMan?.GetComponent<MetaDataComponent>(Owner)?.EntityName ?? "Health Analyzer",
};
_window.OnClose += Close;
_window.OnBodyPartSelected += SendBodyPartMessage;
_window.OpenCentered();

Comment on lines 9 to 12
// Shitmed Start
using Content.Shared.Body.Part;
using Content.Shared.Body.Systems;
// Shitmed End
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

Remove unprofessional comments and maintain code professionalism

The comments // Shitmed Start and // Shitmed End are unprofessional and may cause confusion. Please replace them with meaningful and professional comments or remove them if they are unnecessary.

Comment on lines 52 to 57
// Start-Shitmed
Subs.BuiEvents<HealthAnalyzerComponent>(HealthAnalyzerUiKey.Key, subs =>
{
subs.Event<HealthAnalyzerPartMessage>(OnHealthAnalyzerPartSelected);
});
// End-Shitmed
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

Eliminate unprofessional comments for better clarity

The use of // Start-Shitmed and // End-Shitmed is unprofessional. It's important to maintain professional standards in code comments to ensure clarity and maintainability. Consider removing these comments or replacing them with appropriate, descriptive comments.

Comment on lines 78 to 87
// Shitmed Change Start
if (component.CurrentBodyPart != null
&& (Deleted(component.CurrentBodyPart)
|| TryComp(component.CurrentBodyPart, out BodyPartComponent? bodyPartComponent)
&& bodyPartComponent.Body is null))
{
BeginAnalyzingEntity((uid, component), patient, null);
continue;
}
// Shitmed Change End
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

Avoid unprofessional comments and standardize comment usage

The comments // Shitmed Change Start and // Shitmed Change End are unprofessional and can be confusing to other developers. Please use clear and professional comments that accurately describe the purpose of the code changes.

Comment on lines +206 to +229
// Start-backmen: surgery
/// <summary>
/// Handle the selection of a body part on the health analyzer
/// </summary>
/// <param name="healthAnalyzer">The health analyzer that's receiving the updates</param>
/// <param name="args">The message containing the selected part</param>
private void OnHealthAnalyzerPartSelected(Entity<HealthAnalyzerComponent> healthAnalyzer, ref HealthAnalyzerPartMessage args)
{
if (!TryGetEntity(args.Owner, out var owner))
return;

if (args.BodyPart == null)
{
BeginAnalyzingEntity(healthAnalyzer, owner.Value, null);
}
else
{
var (targetType, targetSymmetry) = _bodySystem.ConvertTargetBodyPart(args.BodyPart.Value);
if (_bodySystem.GetBodyChildrenOfType(owner.Value, targetType, symmetry: targetSymmetry) is { } part)
BeginAnalyzingEntity(healthAnalyzer, owner.Value, part.FirstOrDefault().Id);
}
}
// End-backmen: surgery

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

Maintain professionalism in comments

The comments // Start-backmen: surgery and // End-backmen: surgery should be replaced with professional and descriptive comments that reflect the functionality being implemented. Unprofessional comments can detract from code readability and maintainability.

Comment on lines +224 to +225
if (_bodySystem.GetBodyChildrenOfType(owner.Value, targetType, symmetry: targetSymmetry) is { } part)
BeginAnalyzingEntity(healthAnalyzer, owner.Value, part.FirstOrDefault().Id);
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

Prevent possible NullReferenceException when accessing body parts

In the code:

if (_bodySystem.GetBodyChildrenOfType(owner.Value, targetType, symmetry: targetSymmetry) is { } part)
    BeginAnalyzingEntity(healthAnalyzer, owner.Value, part.FirstOrDefault().Id);

If part is not null but the collection is empty, part.FirstOrDefault() will return null, and accessing .Id on null will cause a NullReferenceException. Please add a check to ensure that part.FirstOrDefault() is not null before accessing .Id.

Apply this diff to fix the issue:

if (_bodySystem.GetBodyChildrenOfType(owner.Value, targetType, symmetry: targetSymmetry) is { } part)
{
    var bodyPart = part.FirstOrDefault();
    if (bodyPart != null)
        BeginAnalyzingEntity(healthAnalyzer, owner.Value, bodyPart.Id);
}

Comment on lines +214 to +215
if (!TryGetEntity(args.Owner, out var owner))
return;
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

Ensure correct entity retrieval and existence check

In the code:

if (!TryGetEntity(args.Owner, out var owner))
    return;

Please verify that TryGetEntity is the appropriate method for checking the existence of args.Owner. If args.Owner is an EntityUid, and you need to check whether it exists or is valid, consider using EntityManager.EntityExists(args.Owner). Also, ensure that owner is correctly assigned and used in subsequent code.

private EntityUid _spriteViewEntity; // backmen: surgery
// Start-Shitmed
public event Action<TargetBodyPart?, EntityUid>? OnBodyPartSelected;
private EntityUid _spriteViewEntity;
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

Initialize '_spriteViewEntity' to avoid using an uninitialized value.

The _spriteViewEntity field is declared but not initialized. Before checking if it's deleted or attempting to delete it, ensure that it has been assigned a valid value. Otherwise, it may default to EntityUid.Invalid, and calling methods on it could lead to unintended behavior.

Apply this diff to initialize _spriteViewEntity:

- private EntityUid _spriteViewEntity;
+ private EntityUid _spriteViewEntity = EntityUid.Invalid;

Also applies to: 336-337

Comment on lines +91 to +92
// Bit of the ole shitcode until we have Groins in the prototypes.
OnBodyPartSelected?.Invoke(part == TargetBodyPart.Groin ? TargetBodyPart.Torso : part, _target.Value);
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

Remove unprofessional language from comments.

Comments contain unprofessional language such as "shitcode" and colloquial expressions like "im lazy". Please replace them with professional and descriptive comments.

Apply this diff to correct the comments:

- // Bit of the ole shitcode until we have Groins in the prototypes.
+ // Temporary workaround until Groin prototypes are available.

- // It's probably shitcode but im lazy to get into sprite stuff - It is shitcode :)
+ // TODO: Optimize sprite handling logic.

Also applies to: 351-351

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (6)
Content.Client/HealthAnalyzer/UI/HealthAnalyzerWindow.xaml.cs (4)

116-118: Consider simplifying null checks and component access

The current pattern of null checking and component access could be simplified using pattern matching.

Consider this alternative:

- if (_target == null
-     || !_entityManager.TryGetComponent<DamageableComponent>(isPart ? part : _target, out var damageable))
- {
+ if (_target is not { } target 
+     || !_entityManager.TryGetComponent(isPart ? part : target, out DamageableComponent? damageable))
+ {

194-214: Consider extracting alert creation logic

The alert creation code has duplication and uses magic numbers.

Consider extracting a helper method:

+ private RichTextLabel CreateAlertLabel(string message)
+ {
+     const int MAX_WIDTH = 300;
+     const int MARGIN = 4;
+     
+     return new RichTextLabel
+     {
+         Margin = new Thickness(0, MARGIN),
+         MaxWidth = MAX_WIDTH
+     }.WithMessage(message, defaultColor: Color.Red);
+ }

- var unrevivableLabel = new RichTextLabel
- {
-     Margin = new Thickness(0, 4),
-     MaxWidth = 300
- };
- unrevivableLabel.SetMessage(...);
+ var unrevivableLabel = CreateAlertLabel(Loc.GetString("health-analyzer-window-entity-unrevivable-text"));

Line range hint 336-339: Improve entity cleanup pattern

The current entity cleanup pattern could lead to potential issues if the method is called multiple times in quick succession.

Consider this safer approach:

- if (!_entityManager.Deleted(_spriteViewEntity))
-     _entityManager.QueueDeleteEntity(_spriteViewEntity);
- 
- _spriteViewEntity = _entityManager.Spawn(_bodyView);
+ var oldEntity = _spriteViewEntity;
+ _spriteViewEntity = _entityManager.Spawn(_bodyView);
+ 
+ if (_entityManager.IsAlive(oldEntity))
+     _entityManager.DeleteEntity(oldEntity);

Line range hint 344-358: Consider improving sprite layer management

The sprite layer management could be more robust with proper error handling and layer validation.

Consider adding validation:

+ const int MAX_LAYERS = 20; // Or appropriate maximum
+ if (layer >= MAX_LAYERS)
+ {
+     Logger.Warning($"Exceeded maximum layer count in health analyzer sprite");
+     continue;
+ }

if (!sprite.TryGetLayer(layer, out _))
    sprite.AddLayer(_spriteSystem.Frame0(rsi));
else
    sprite.LayerSetTexture(layer, _spriteSystem.Frame0(rsi));
Content.Server/Medical/HealthAnalyzerSystem.cs (2)

260-262: Remove or Clarify Commented-Out Code

The commented-out code:

/*if (HasComp<UnrevivableComponent>(target)) Somehow we dont have unrevivable???
    unrevivable = true;
*/

includes an unclear comment and unnecessary code. Leaving such code can cause confusion for future maintainers.

Consider removing this commented-out code if it's not needed. If the UnrevivableComponent check is required, implement it properly. If the component is missing, you may need to define it or handle the case differently.


264-268: Ensure Safe Handling of Potentially Null body

The variable body is initialized as null and may remain so if HasComp<TargetingComponent>(target) returns false. Ensure that any subsequent usage of body accounts for the possibility of it being null to avoid NullReferenceException.

Add null checks where body is used, or ensure that the UI can handle a null value appropriately.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 09d13a7 and 0d20b29.

📒 Files selected for processing (2)
  • Content.Client/HealthAnalyzer/UI/HealthAnalyzerWindow.xaml.cs (9 hunks)
  • Content.Server/Medical/HealthAnalyzerSystem.cs (11 hunks)
🔇 Additional comments (11)
Content.Client/HealthAnalyzer/UI/HealthAnalyzerWindow.xaml.cs (3)

41-41: LGTM: Well-defined event for body part selection

The event declaration follows C# conventions and provides a clear contract for body part selection notifications.


62-83: LGTM: Clean initialization of body part controls

The initialization code is well-structured with:

  • Complete mapping of body parts to buttons
  • Consistent event handler setup
  • Proper mouse filter configuration

86-101: LGTM: Clear and focused part selection methods

Both methods have proper null checks and clear responsibility boundaries. The special case handling for the groin part is properly implemented.

Content.Server/Medical/HealthAnalyzerSystem.cs (8)

40-43: Dependencies Added for Extended Functionality

The addition of dependencies for SharedBodySystem, UserInterfaceSystem, TransformSystem, and SharedPopupSystem is appropriate for the new features introduced. This ensures that the system has access to the necessary components for handling body parts and user interface interactions.


78-87: Proper Handling of Invalid CurrentBodyPart

The added condition effectively checks if CurrentBodyPart is deleted or invalid and resets it by calling BeginAnalyzingEntity with a null part. This ensures that the analyzer does not reference an invalid body part.


100-100: Updating Scanned User with Body Part Information

The call to UpdateScannedUser now includes component.CurrentBodyPart as an argument. This change correctly passes the current body part to the update method, allowing for more detailed information to be displayed.


117-117: Correct Usage of NeedHand in DoAfterArgs

Including NeedHand = true in the DoAfterArgs object initializer ensures that the action requires the user to have a free hand. This is important for maintaining realistic interactions.


180-188: Extended BeginAnalyzingEntity Method Signature

The method BeginAnalyzingEntity now accepts an optional EntityUid? part = null parameter. This enhancement allows the system to analyze specific body parts, providing more granular functionality.


199-199: Resetting CurrentBodyPart in StopAnalyzingEntity

Setting healthAnalyzer.Comp.CurrentBodyPart to null when stopping the analysis correctly resets the state of the analyzer. This prevents stale data from persisting.


206-229: Implementation of OnHealthAnalyzerPartSelected Method

The new method OnHealthAnalyzerPartSelected appropriately handles the selection of a body part from the UI. It processes the input and updates the analyzer's state accordingly. Ensure that all potential null references are handled to prevent runtime exceptions.


277-278: Passing Body and Part Data to UI Message

Including body and part in the HealthAnalyzerScannedUserMessage allows the UI to display detailed information about the body and the specific part being analyzed. This enhances the user's ability to assess health conditions.

Comment on lines +52 to +57
// Start-backmen: surgery
Subs.BuiEvents<HealthAnalyzerComponent>(HealthAnalyzerUiKey.Key, subs =>
{
subs.Event<HealthAnalyzerPartMessage>(OnHealthAnalyzerPartSelected);
});
// End-backmen: surgery
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

Possible Typo or Undefined Method Subs

The method Subs.BuiEvents used here may not be defined or could be a typo. It is unclear what Subs refers to. If the intention is to subscribe to UI events, please verify that you're using the correct method provided by the framework.

Apply this diff to use the correct event subscription method:

- // Start-backmen: surgery
- Subs.BuiEvents<HealthAnalyzerComponent>(HealthAnalyzerUiKey.Key, subs =>
- {
-     subs.Event<HealthAnalyzerPartMessage>(OnHealthAnalyzerPartSelected);
- });
- // End-backmen: surgery
+ // Start-backmen: surgery
+ SubscribeLocalEvent<HealthAnalyzerPartMessage>(OnHealthAnalyzerPartSelected);
+ // End-backmen: surgery

Alternatively, if Subs is a valid method within your codebase, ensure it is properly defined and that its usage here is correct.

Committable suggestion skipped: line range outside the PR's diff.

@Rxup Rxup merged commit b4cbe24 into Rxup:master Nov 21, 2024
11 checks passed
TokenStyle pushed a commit to TokenStyle/space-station-14-next that referenced this pull request Nov 21, 2024
Add amazing Health analyzer UI.
Rxup/space-station-14#925

* Update HealthAnalyzerSystem.cs

* Update medical-scanner-component.ftl

* Update HealthAnalyzerWindow.xaml.cs

* Update health-analyzer-component.ftl

* fix

---------

Co-authored-by: trest100 <[email protected]>
AwareFoxy added a commit to space-syndicate/space-station-14-next that referenced this pull request Nov 22, 2024
* The ShitMed

* Fixes

fix locale

* Delete redudant systems

* fix components

* fix error, update system

* Update BodyPartComponent.cs

* fix max health, fix test legs

* fix test

* Port fixes from EE (space-syndicate#875)

* [Fix] Healing Fixes (space-syndicate#874)

* Targeting & Healing Fixes

* Update Content.Server/Medical/HealingSystem.cs

Co-authored-by: gluesniffler <[email protected]>

* Fix Vital Damage

* Update BodyPartComponent.cs

* fix

---------

Co-authored-by: gluesniffler <[email protected]>

* [Port] Surgery fixes: Part 2 (space-syndicate#876)

* Fix a ton of shitmed

Fixed a buncho crap

* Guidebook

* Russian Locale

* fix

* hotfix added Rxup/space-station-14@e92f013

---------

Co-authored-by: gluesniffler <[email protected]>
Co-authored-by: Zack Backmen <[email protected]>
Co-authored-by: Token <github.com/TokenStyle>

* remove backmen ccvar

* Body doll in the Health Analyzer menu (space-syndicate#877)

* fixes and fix guidebook paths (space-syndicate#887)

* fixes

* Added some experimental lag fixes to surgery. Might have crashes

* russian locale

* Revert "Added some experimental lag fixes to surgery. Might have crashes"

This reverts commit de95d85.

* [Port] Surgery fixes: Part 3 (space-syndicate#892)

* Added some experimental lag fixes to surgery. Might have crashes

* Fix

* migrate and add comments to shit

* fix

---------

Co-authored-by: gluesniffler <[email protected]>

* [Tweak] Rework BodyPartComponent to use DamageSpecifier (space-syndicate#890)

* Rework BodyPart to use DamageSpecifier

* Fix that shit

* Update Content.Shared/Backmen/Surgery/Body/SharedBodySystem.Integrity.cs

Co-authored-by: gluesniffler <[email protected]>

* Uh?

* Update HealthAnalyzerWindow.xaml.cs

* balanced a buncho crap

---------

Co-authored-by: gluesniffler <[email protected]>

* [Port] Surgery fixes: Part 4 (space-syndicate#909)

* defelinization almost real

* fix?

* Corvax next fix that surgery doesn't work with fatal error

---------

Co-authored-by: gluesniffler <[email protected]>
Roudenn: Rxup/space-station-14#909

* [Fix] surgery emergency fix

* Fixed mechanics of attaching body parts to Body. (now taken from prototypes, not generated at random)

---------

Co-authored-by: gluesniffler <[email protected]>
KayzelW: Rxup/space-station-14#912

* [Balance] Surgery healing tweaks (space-syndicate#911)

* Balance surgery

* Fix Severing parts

---------

Co-authored-by: gluesniffler <[email protected]>
Roudenn: Rxup/space-station-14#911

* [Port] Surgery Update V: The Phantom Gib (space-syndicate#919)

* Megasquached Surgery Updates

Megasquached Surgery Updates

Quick patch to see if tests shut up

Refactored body parts to use damageablecomponent

newmed health analyzer real, also refactors and some bugfixes

* added solidus's comments, pending removal of namespace thingies for this repo, also fixed some healing thingies

* fixes?

* fix gib torso

* fix prototypes

* oops

---------

Co-authored-by: gluesniffler <[email protected]>
Co-authored-by: Zack Backmen <[email protected]>
Roudenn: Rxup/space-station-14#919

* let me pass tests

* let me pass test pls

* let me pass test pls

* let me pass tests

* chief medical officer pass tests

* no felinid ears no fun pass tests

* [Port] Surgery Fixes: Popup Walker (space-syndicate#923)

* Megasquached Surgery Updates

Megasquached Surgery Updates

Quick patch to see if tests shut up

Please do not cherrypick this one yet.

Refactored body parts to use damageablecomponent

newmed health analyzer real, also refactors and some bugfixes

* added solidus's comments, pending removal of namespace thingies for this repo, also fixed some healing thingies

* fixes?

* fix gib torso

* fix prototypes

* oops

* Shitmed Update 1 (space-syndicate#1240)

First in a series of PRs to introduce bugfixes and updates to Shitmed,
this will generally feature PRs from Goobstation or Backmen as well
since they are actively helping me maintain the code.

Usual Shoutouts:
Deltanedas: Goob-Station/Goob-Station#882

---

:cl: Mocho, Deltanedas
- add: You can now perform surgery as a monke. Rejoice.
- add: You can perform surgery on a lot of animals now, I missed a lot
of them so just ask if you want any particular critter to get it.
- tweak: Entities now perish after 60 seconds of losing their heart
and/or brain.
- fix: Entities properly take asphyxiation damage after losing their
brain.
- fix: Torsos being gibbable, which would break surgery or just about
anything.
- fix: Items not being removed from their respective slots if the parts
were gibbed rather than dropped.
- fix: Animal organs not being usable properly in surgeries
- fix: Cyborg limbs are now usable as pseudo-peg arm/legs.

---------

Signed-off-by: gluesniffler <[email protected]>
Co-authored-by: FoxxoTrystan <[email protected]>
Co-authored-by: goet <[email protected]>
Co-authored-by: Saphire Lattice <[email protected]>
Co-authored-by: Roudenn <[email protected]>

* Shitmed Surgery Popups (space-syndicate#1241)

Adds popups for surgery steps in Shitmed that every player within PVS
range can see. This allows other players to see if the correct procedure
is being performed. This PR also includes locale text for the new
procedures and steps in space-syndicate#1240.

**Remove Brain / Insert Brain**

https://github.com/user-attachments/assets/ac20afa1-df74-48ab-b1d5-2e9a273dfba2

<details><summary>See more</summary>

**Amputate Right Arm**

https://github.com/user-attachments/assets/17f78683-6d3b-44ee-aea3-bb6987844fdc

**Attach Right Arm**

https://github.com/user-attachments/assets/584d4da2-d8b0-4c82-a323-26636e7fa4b8

</details>

:cl: Skubman
- add: Surgery step descriptions (like making an incision,
removing/attaching limbs and organs) are now shown as popups to everyone
in range upon the start of the step. This makes it clear which surgical
procedure is being done and to which body part. No more stealthy
brain-stealing in front of everyone!

* fix the things + locale

---------

Signed-off-by: gluesniffler <[email protected]>
Co-authored-by: gluesniffler <[email protected]>
Co-authored-by: Zack Backmen <[email protected]>
Co-authored-by: gluesniffler <[email protected]>
Co-authored-by: FoxxoTrystan <[email protected]>
Co-authored-by: goet <[email protected]>
Co-authored-by: Saphire Lattice <[email protected]>
Co-authored-by: Skubman <[email protected]>

* [PORT]Health Analyzer update (space-syndicate#925)

Add amazing Health analyzer UI.
Rxup/space-station-14#925

* Update HealthAnalyzerSystem.cs

* Update medical-scanner-component.ftl

* Update HealthAnalyzerWindow.xaml.cs

* Update health-analyzer-component.ftl

* fix

---------

Co-authored-by: trest100 <[email protected]>

* let me pass tests

* remove shadowkin let tests pass

* first commit

* Revert "first commit" formatting changes

Reverted only formats though, being saved moves everythin to appropriate folders, and nice and clean
This reverts commit e66b04b.

* add corvax next comments for changes for easier upstreams

Be aware though, that everything that related to BodySystem, OrganComponent, DamageSystem etc was changed completely

- add comments _CorvaxNext
- moved .ftl files and cvars to appropriate Corvax Next folders
- removed unnecessary felinids and silicon_base

* namespaces changes for everythin

* Revert "namespaces changes for everythin"

This reverts commit 221ee3e.

* [Fix] Surgery Hotfix: Head and Limbs dissapears

---------

Signed-off-by: gluesniffler <[email protected]>
Co-authored-by: gluesniffler <[email protected]>
Co-authored-by: Roudenn <[email protected]>
Co-authored-by: Zack Backmen <[email protected]>
Co-authored-by: Rouden <[email protected]>
Co-authored-by: gluesniffler <[email protected]>
Co-authored-by: FoxxoTrystan <[email protected]>
Co-authored-by: goet <[email protected]>
Co-authored-by: Saphire Lattice <[email protected]>
Co-authored-by: Skubman <[email protected]>
Co-authored-by: Trest <[email protected]>
Co-authored-by: AwareFoxy <[email protected]>
Co-authored-by: AwareFoxy <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changes: Localization Can be reviewed or fixed by people who are knowledgeable with translation Changes: UI Can be reviewed or fixed by people who are knowledgeable with UI design
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants