[Tweak] Holoprojector / Голопроектрор #112
Conversation
WalkthroughThe changes in this pull request involve significant modifications to the Changes
Suggested reviewers
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: 8
🧹 Outside diff range and nitpick comments (12)
Content.Server/_White/Holosign/HolosignComponent.cs (1)
1-5: Add XML documentation for the component class.
While the component structure is correct, adding XML documentation would improve maintainability by describing the component's purpose and usage within the holoprojector system.
namespace Content.Server._White.Holosign;
[RegisterComponent]
+/// <summary>
+/// Component that represents a holosign entity and maintains a reference to its associated sign.
+/// Used in conjunction with the holoprojector system for managing holographic signs.
+/// </summary>
public sealed partial class HolosignComponent : ComponentResources/Locale/ru-RU/_white/holoprojector/holoprojector.ftl (1)
1-3: LGTM! Consider standardizing punctuation.
The translations are accurate and properly formatted. For consistency, consider adding an exclamation mark to the first message to match the style of the other two messages.
-holoprojector-active-holo = Активные проекции: [color=fuchsia]{ $activeholo }[/color]
+holoprojector-active-holo = Активные проекции: [color=fuchsia]{ $activeholo }[/color]!Resources/Locale/en-US/_white/holoprojector/holoprojector.ftl (1)
2-2: Consider making the limit message more informative.
The warning message could be more helpful by including the actual maximum number of allowed projections.
-holoprojector-uses-limit = The maximum number of projections was used!
+holoprojector-uses-limit = Maximum limit reached! You can only create { $maxUses } projections.Content.Server/_White/Holosign/HolosignSystem.cs (1)
1-3: Consider adding XML documentation comments.
Adding XML documentation comments for the namespace and class would improve code maintainability by clearly describing the purpose and functionality of this new system.
Content.Server/Holosign/HolosignProjectorComponent.cs (2)
17-19: Add ViewVariables attribute to Uses field.
Consider adding the ViewVariables attribute to the Uses field for consistency and to aid in debugging:
[DataField]
+[ViewVariables(VVAccess.ReadWrite)]
public int Uses = 6;13-22: Consider additional safeguards for the usage system.
A few architectural suggestions to improve robustness:
- Consider adding validation to ensure Uses doesn't exceed MaxUses
- Consider implementing a maximum limit for the Signs list to prevent unbounded growth
- Consider adding documentation explaining the rationale for 6 uses
Here's a more robust implementation:
// WD EDIT START
+/// <summary>
+/// Maximum number of signs that can be created before recharge is needed.
+/// Set to 6 to match the reference design from the 13th station.
+/// </summary>
[DataField]
public int MaxUses = 6;
+[ViewVariables(VVAccess.ReadWrite)]
[DataField]
-public int Uses = 6;
+public int Uses
+{
+ get => _uses;
+ set => _uses = Math.Clamp(value, 0, MaxUses);
+}
+private int _uses = 6;
+/// <summary>
+/// Tracks currently active signs created by this projector.
+/// </summary>
[ViewVariables(VVAccess.ReadOnly)]
+[DataField]
+[DataDefinition]
public List<EntityUid?> Signs = new();
+
+/// <summary>
+/// Maximum number of signs that can exist simultaneously to prevent resource exhaustion.
+/// </summary>
+[DataField]
+public int MaxSigns = 20;
// WD EDIT ENDResources/Prototypes/Entities/Objects/Devices/holoprojectors.yml (1)
Line range hint 44-45: Fix inconsistent tag values across projector variants.
All projector variants are using the "HolofanProjector" tag, which appears to be incorrect for the field and security variants.
Apply these changes to fix the tag consistency:
- type: Tag
tags:
- - HolofanProjector
+ - HoloprojectorField - type: Tag
tags:
- - HolofanProjector
+ - HoloprojectorSecurityAlso applies to: 67-68, 89-90
Resources/Prototypes/Entities/Structures/Holographic/projections.yml (2)
Line range hint 1-124: Consider persistence implications of removing TimedDespawn.
The removal of the TimedDespawn component from all holographic entities has several implications:
- Without automatic cleanup, holograms could persist indefinitely
- This might impact server performance if many holograms are created
- Consider implementing alternative cleanup mechanisms or decay behavior
Consider one of these alternatives:
- Add a maximum lifetime alongside the usage count
- Implement a decay system based on distance from projector
- Add a maximum concurrent holograms limit per area
Line range hint 35-124: Consider restructuring entity inheritance hierarchy.
The current inheritance structure where all holographic entities inherit from HolosignWetFloor might not be optimal:
- Base entity has
canCollide: falsewhile children override with different collision settings - Children have significantly different behaviors (force fields vs. warning signs)
Consider creating a more logical hierarchy:
- Abstract base
HolosignBasewith common properties - Separate hierarchies for interactive vs. barrier holograms
- Group similar collision behaviors together
Content.Server/Holosign/HolosignSystem.cs (1)
28-41: Consider defensive programming improvements.
While the logic is functionally correct, consider these improvements:
- Add null check for component.Signs
- Use consistent string interpolation style
- Use a more specific examination group name
Consider this refactor:
- var activeholo = component.Signs.Count;
+ var activeholo = component.Signs?.Count ?? 0;
- using (args.PushGroup(nameof(HolosignProjectorComponent)))
+ using (args.PushGroup("HoloprojectorStatus"))
{
- args.PushMarkup(Loc.GetString("limited-charges-charges-remaining", ("charges", charges)));
- args.PushMarkup(Loc.GetString("holoprojector-active-holo", ("activeholo", activeholo)));
+ args.PushMarkup(Loc.GetString("limited-charges-charges-remaining", new { charges }));
+ args.PushMarkup(Loc.GetString("holoprojector-active-holo", new { activeholo }));Resources/Migrations/migration.yml (2)
251-251: Fix date format in comment
The date format 2024-30-10 is non-standard. It should be 2024-10-30 to maintain consistency with the YYYY-MM-DD format used throughout the file.
256-256: Add newline at end of file
Add a newline character at the end of the file to comply with YAML best practices and prevent potential issues with some tools.
🧰 Tools
🪛 yamllint
[error] 256-256: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
- Content.Server/Holosign/HolosignProjectorComponent.cs (1 hunks)
- Content.Server/Holosign/HolosignSystem.cs (1 hunks)
- Content.Server/_White/Holosign/HolosignComponent.cs (1 hunks)
- Content.Server/_White/Holosign/HolosignSystem.cs (1 hunks)
- Resources/Locale/en-US/_white/holoprojector/holoprojector.ftl (1 hunks)
- Resources/Locale/ru-RU/_white/holoprojector/holoprojector.ftl (1 hunks)
- Resources/Migrations/migration.yml (1 hunks)
- Resources/Prototypes/Entities/Objects/Devices/holoprojectors.yml (1 hunks)
- Resources/Prototypes/Entities/Objects/Specific/Robotics/borg_modules.yml (1 hunks)
- Resources/Prototypes/Entities/Structures/Holographic/projections.yml (1 hunks)
- Resources/Prototypes/Recipes/Lathes/janitorial.yml (1 hunks)
- Resources/Prototypes/Recipes/Lathes/security.yml (1 hunks)
- Resources/Prototypes/Recipes/Lathes/tools.yml (2 hunks)
✅ Files skipped from review due to trivial changes (1)
- Resources/Prototypes/Recipes/Lathes/security.yml
🧰 Additional context used
🪛 yamllint
Resources/Migrations/migration.yml
[error] 256-256: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (15)
Content.Server/_White/Holosign/HolosignComponent.cs (1)
1-8: Verify the component's integration with HolosignSystem.
The component looks good overall, but we should verify its integration with the system that manages it.
✅ Verification successful
Component is properly integrated with HolosignSystem
The component is well integrated with both HolosignSystem implementations:
- The
_White/Holosign/HolosignSystem.cshandles component destruction viaDestructionEventArgs - The main
Holosign/HolosignSystem.csmanages the component's lifecycle:- Creates and initializes HolosignComponent when placing new signs
- Properly sets the Sign field to reference the projector
- Handles cleanup when signs are removed or destroyed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the HolosignSystem implementation and its interaction with this component
# Test 1: Check if HolosignSystem exists and handles this component
ast-grep --pattern 'class HolosignSystem : EntitySystem
{
$$$
public void $_(EntityUid uid, HolosignComponent component)
{
$$$
}
$$$
}'
# Test 2: Check for proper cleanup handling
rg -A 5 "OnRemove.*HolosignComponent"
Length of output: 151
Script:
#!/bin/bash
# Let's try a broader search to find any mentions of HolosignSystem and its implementation
# Search for HolosignSystem class definition
rg -l "class HolosignSystem"
# Search for any file handling HolosignComponent
rg -l "HolosignComponent"
# Search for any methods that might be handling the Sign field
ast-grep --pattern 'class $_ {
$$$
$_($_, HolosignComponent $_) {
$$$
}
$$$
}'
Length of output: 383
Script:
#!/bin/bash
# Let's examine the content of both HolosignSystem files to understand the implementation
# Check the content of _White/Holosign/HolosignSystem.cs
cat Content.Server/_White/Holosign/HolosignSystem.cs
# Check the content of the other HolosignSystem.cs
cat Content.Server/Holosign/HolosignSystem.cs
# Look for any initialization or cleanup methods
rg -A 5 "Initialize|OnRemove" Content.Server/_White/Holosign/HolosignSystem.cs
Length of output: 4753
Resources/Locale/en-US/_white/holoprojector/holoprojector.ftl (1)
1-3: LGTM! Consistent terminology and clear messaging.
The localization strings are well-structured with consistent use of terminology ("projections") and proper grammar throughout. The color formatting and string interpolation are correctly implemented.
Content.Server/_White/Holosign/HolosignSystem.cs (1)
1-21: Verify the implementation matches the AI summary.
The current implementation appears incomplete compared to the AI-generated summary which mentions:
- Handling of
UseInHandEvent - Integration with
PopupSystem - Management of
MaxUses
Let's verify if these components exist elsewhere in the codebase:
Resources/Prototypes/Recipes/Lathes/janitorial.yml (1)
69-69: LGTM! The recipe update aligns with the new holoprojector system.
The change from HoloprojectorEmpty to Holoprojector as the recipe result is consistent with the broader system updates that remove empty variants and transition to a usage-count based system.
Let's verify there are no remaining references to the removed HoloprojectorEmpty:
✅ Verification successful
The reference to HoloprojectorEmpty in the migration file is expected and correct
The presence of HoloprojectorEmpty: Holoprojector in Resources/Migrations/migration.yml is actually part of the intended change. This migration file is specifically designed to map old entity IDs to new ones when loading older content, ensuring backward compatibility. The migration entry aligns perfectly with the holoprojector redesign that removes empty variants and transitions to the new system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to HoloprojectorEmpty
# Expected: No results, as this entity has been removed
# Search for any remaining references to HoloprojectorEmpty
rg "HoloprojectorEmpty"
Length of output: 92
Script:
#!/bin/bash
# Let's check the migration file to understand the context
cat Resources/Migrations/migration.yml
Length of output: 8309
Resources/Prototypes/Entities/Objects/Devices/holoprojectors.yml (3)
Line range hint 71-76: Consider adding ReverseEngineering component to HoloprojectorSecurity.
The HolofanProjector and HoloprojectorField have ReverseEngineering components with difficulty 3, but HoloprojectorSecurity doesn't. This might be intentional, but worth verifying.
Let's check if this is consistent with similar security devices:
#!/bin/bash
# Check ReverseEngineering usage on security devices
rg "parent: .*Security.*\n.*\n.*\n.*ReverseEngineering:" -A 2Also applies to: 89-93
Line range hint 1-93: Verify the default Uses/MaxUses values are appropriate for all variants.
The AI summary indicates a transition from charge-based to usage-count system, but the YAML doesn't specify MaxUses or Uses values. Consider if different projector types should have different usage limits.
Let's check the component implementation:
11-11: Consider documenting the reasoning behind the 0.5s delay.
While adding a delay helps prevent spam usage, it would be helpful to document why 0.5 seconds was chosen as the specific value. Also, consider using a more descriptive comment format instead of "WD EDIT".
Let's check if there are any established delay values for similar items:
✅ Verification successful
The 0.5s delay is consistent with similar interaction-preventing delays in the codebase
The 0.5 second delay appears to be a common value used across various items to prevent rapid/spam usage, including:
- Desk bells
- Light switches
- Trashbags
- Hyposprays
- Mech equipment
- Shield toggles
While documenting the specific reasoning would be nice, the chosen delay value aligns well with established patterns for preventing interaction spam in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other UseDelay configurations to ensure consistency
rg "UseDelay:?\s*\n\s*delay:" -A 1
Length of output: 228
Script:
#!/bin/bash
# Search for UseDelay configurations with multiline mode
rg -U "UseDelay:?\s*\n\s*delay:" -A 1
# Also search for delay configurations in general to understand common patterns
rg "delay:\s*[0-9.]+" -A 1
Length of output: 45021
Content.Server/Holosign/HolosignSystem.cs (2)
1-23: LGTM: Clean dependency management and initialization.
The changes properly integrate the new PopupSystem dependency and event subscription while removing the battery-related dependencies, aligning well with the new usage-based system.
48-48: Good catch on preventing storage interaction.
The storage component check prevents unintended interactions when storing the projector.
Resources/Prototypes/Recipes/Lathes/tools.yml (2)
241-241: Verify entity exists and approve recipe update.
The recipe update from HoloprojectorFieldEmpty to HoloprojectorField aligns with the new usage-based system. The higher material cost (including plasma) suggests this is an advanced variant.
Let's verify the entity exists:
#!/bin/bash
# Description: Verify that HoloprojectorField entity exists in the prototypes
# Expected: Should find the entity definition
rg -l "^- type: entity\s+id: HoloprojectorField\s*$"192-192: Verify entity exists and approve recipe update.
The recipe update from HolofanProjectorEmpty to HolofanProjector aligns with the new usage-based system.
Let's verify the entity exists:
✅ Verification successful
Recipe update to HolofanProjector is valid and entity exists
The entity HolofanProjector is properly defined in Resources/Prototypes/Entities/Objects/Devices/holoprojectors.yml with all required components. The migration from HolofanProjectorEmpty to HolofanProjector is also documented in Resources/Migrations/migration.yml, confirming this is an intended change aligned with the new usage-based system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that HolofanProjector entity exists in the prototypes
# Expected: Should find the entity definition
rg -l "^- type: entity\s+id: HolofanProjector\s*$"
Length of output: 54
Script:
#!/bin/bash
# Let's try a more flexible search pattern for the entity
rg "id: HolofanProjector" -A 5
# Also search in yaml files specifically
fd -e yml -e yaml -x rg "id: HolofanProjector" {} -A 5
# Let's also check if there might be a different casing or naming
rg -i "holofan" -A 5
Length of output: 18155
Resources/Migrations/migration.yml (2)
249-250: LGTM: VendingMachinePride mapping
The mapping follows the established pattern for vending machine migrations.
252-256: LGTM: Holoprojector mappings
The consolidation of specialized holoprojector variants to their base types aligns with the PR objectives of redesigning the holoprojector system.
Let's verify that these entities are properly handled:
✅ Verification successful
Holoprojector mappings verified successfully
The old holoprojector entities are only referenced in the migration file, and the base types (Holoprojector, HolofanProjector, HoloprojectorField, HoloprojectorSecurity) are properly defined across various prototype and map files. The consolidation appears to be implemented correctly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining references to the old holoprojector entities
rg -l "HoloprojectorEmpty|HoloprojectorBorg|HolofanProjectorEmpty|HoloprojectorFieldEmpty|HoloprojectorSecurityEmpty"
Length of output: 154
Script:
#!/bin/bash
# Let's check the content around these references to ensure they are only in migration mappings
rg -C 3 "HoloprojectorEmpty|HoloprojectorBorg|HolofanProjectorEmpty|HoloprojectorFieldEmpty|HoloprojectorSecurityEmpty" Resources/Migrations/migration.yml
# Also search for base types to ensure they exist
rg -l "Holoprojector|HolofanProjector|HoloprojectorField|HoloprojectorSecurity"
Length of output: 2142
🧰 Tools
🪛 yamllint
[error] 256-256: no new line character at the end of file
(new-line-at-end-of-file)
Resources/Prototypes/Entities/Objects/Specific/Robotics/borg_modules.yml (2)
Line range hint 342-348: LGTM! The change is properly integrated
The addition of the Holoprojector to the advanced cleaning module's item list is well-placed within the janitorial section. The change is consistent with the PR's objective of redesigning holoprojectors.
346-346: Verify the Holoprojector entity exists
The change replaces HoloprojectorBorg with Holoprojector. While this aligns with the holoprojector redesign objectives, we should verify the new entity's existence and compatibility.
✅ Verification successful
Holoprojector entity verified and migration confirmed
The new Holoprojector entity is properly defined in Resources/Prototypes/Entities/Objects/Devices/holoprojectors.yml with appropriate components including HolosignProjector. The migration from HoloprojectorBorg to Holoprojector is also documented in Resources/Migrations/migration.yml, confirming this is an intentional replacement.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the Holoprojector entity exists and is properly defined
# Expected: Find the entity definition for Holoprojector
# Search for Holoprojector entity definition
echo "Searching for Holoprojector entity definition..."
rg "id: Holoprojector$" -A 10
# Search for any remaining references to the old HoloprojectorBorg
echo "Checking for any remaining references to HoloprojectorBorg..."
rg "HoloprojectorBorg"
Length of output: 3026
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (12)
Resources/Locale/ru-RU/_white/holoprojector/holoprojector.ftl (2)
1-3: Consider adding exclamation mark for message consistency.
The messages follow a consistent format, but the first message lacks an exclamation mark while the others have it.
-holoprojector-active-holo = Активные проекции: [color=fuchsia]{ $activeholo }[/color]
+holoprojector-active-holo = Активные проекции: [color=fuchsia]{ $activeholo }[/color]!1-1: Consider using a more accessible color.
While fuchsia provides visual emphasis, it might not be optimal for all users. Consider using a more neutral color or implementing a configurable color system for better accessibility.
Resources/Locale/en-US/_white/holoprojector/holoprojector.ftl (2)
1-3: LGTM! Consider minor improvements to message clarity.
The messages are well-structured and grammatically correct. Consider these minor improvements:
- Make the limit message more informative by including the maximum number:
"Maximum number of projections ({$maxUses}) reached!" - Use more neutral punctuation in the limit message
holoprojector-active-holo = Active projections: [color=fuchsia]{ $activeholo }[/color]
-holoprojector-uses-limit = The maximum number of projections was used!
+holoprojector-uses-limit = Maximum number of projections ({ $maxUses }) reached.
holoprojector-delete-signs = All projections have been deleted!1-3: Consider consistent color formatting across messages.
The active projections count uses fuchsia coloring, but other messages don't use any color formatting. Consider using consistent color highlighting for important numbers or status changes across all messages.
holoprojector-active-holo = Active projections: [color=fuchsia]{ $activeholo }[/color]
-holoprojector-uses-limit = Maximum number of projections ({ $maxUses }) reached.
-holoprojector-delete-signs = All projections have been deleted!
+holoprojector-uses-limit = Maximum number of projections ([color=fuchsia]{ $maxUses }[/color]) reached.
+holoprojector-delete-signs = [color=fuchsia]All[/color] projections have been deleted!Content.Server/_White/Holosign/HolosignSystem.cs (1)
6-11: Add XML documentation for better code maintainability.
Consider adding XML documentation to describe the system's purpose and responsibilities.
+/// <summary>
+/// Handles the destruction of holosigns and manages their relationship with projectors.
+/// </summary>
public sealed class HolosignSystem : EntitySystemContent.Server/Holosign/HolosignProjectorComponent.cs (1)
20-21: Add XML documentation for Signs list
The Signs list would benefit from documentation explaining its purpose and lifecycle management.
+/// <summary>
+/// Tracks all active signs created by this projector for cleanup purposes.
+/// </summary>
[ViewVariables(VVAccess.ReadOnly)]
public List<EntityUid?> Signs = new();Resources/Prototypes/Entities/Objects/Devices/holoprojectors.yml (2)
Line range hint 7-7: Missing configuration for the new usage-based system.
The HolosignProjector component has been changed from a charge-based to a count-based system, but the YAML configurations don't specify the required MaxUses and Uses properties. This could lead to undefined behavior.
Add the missing properties to each holoprojector variant:
- type: HolosignProjector
+ maxUses: 10 # Adjust this value based on design requirements
+ uses: 10 # Initial number of usesAlso applies to: 23-24, 44-45, 67-68
Line range hint 31-32: Fix inconsistent entity tags.
The HoloprojectorField and HoloprojectorSecurity entities incorrectly use the "HolofanProjector" tag. Each entity should have its own specific tag for proper identification.
Apply these fixes:
# In HoloprojectorField
- type: Tag
tags:
- - HolofanProjector
+ - HoloprojectorField
# In HoloprojectorSecurity
- type: Tag
tags:
- - HolofanProjector
+ - HoloprojectorSecurityAlso applies to: 54-55, 76-77
Resources/Prototypes/Entities/Structures/Holographic/projections.yml (1)
26-33: Consider the UX implications of the electrical glove sound.
The melee sound effect (egloves.ogg) seems thematically inconsistent with a holographic projection. Consider using a more appropriate sound effect that better represents the interaction with a hologram, such as an electronic or energy-based sound.
Content.Server/Holosign/HolosignSystem.cs (2)
28-37: Consider using named format parameters in localization strings.
While the functionality is correct, consider updating the localization string "holoprojector-active-holo" to use named parameters instead of positional ones for better maintainability and clarity.
-args.PushMarkup(Loc.GetString("holoprojector-active-holo", ("activeholo", activeholo)));
+args.PushMarkup(Loc.GetString("holoprojector-active-holo", ("count", activeholo)));84-96: Consider adding operation success feedback.
While the sign cleanup logic is correct, consider tracking the number of signs removed and providing more detailed feedback to the user.
+var removedCount = 0;
foreach (var sign in component.Signs.ToList())
{
component.Signs.Remove(sign);
QueueDel(sign);
+ removedCount++;
}
component.Uses = component.MaxUses;
-_popup.PopupEntity(Loc.GetString("holoprojector-delete-signs"), args.User, args.User, PopupType.Medium);
+_popup.PopupEntity(Loc.GetString("holoprojector-delete-signs-count", ("count", removedCount)),
+ args.User, args.User, PopupType.Medium);Resources/Migrations/migration.yml (1)
256-256: Add newline at end of file.
The file is missing a newline character at the end, which is a YAML best practice.
Add a newline at the end of the file:
HoloprojectorSecurityEmpty: HoloprojectorSecurity
+🧰 Tools
🪛 yamllint
[error] 256-256: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
- Content.Server/Holosign/HolosignProjectorComponent.cs (1 hunks)
- Content.Server/Holosign/HolosignSystem.cs (1 hunks)
- Content.Server/_White/Holosign/HolosignComponent.cs (1 hunks)
- Content.Server/_White/Holosign/HolosignSystem.cs (1 hunks)
- Resources/Locale/en-US/_white/holoprojector/holoprojector.ftl (1 hunks)
- Resources/Locale/ru-RU/_white/holoprojector/holoprojector.ftl (1 hunks)
- Resources/Migrations/migration.yml (1 hunks)
- Resources/Prototypes/Entities/Objects/Devices/holoprojectors.yml (1 hunks)
- Resources/Prototypes/Entities/Objects/Specific/Robotics/borg_modules.yml (1 hunks)
- Resources/Prototypes/Entities/Structures/Holographic/projections.yml (1 hunks)
- Resources/Prototypes/Recipes/Lathes/janitorial.yml (1 hunks)
- Resources/Prototypes/Recipes/Lathes/security.yml (1 hunks)
- Resources/Prototypes/Recipes/Lathes/tools.yml (2 hunks)
✅ Files skipped from review due to trivial changes (1)
- Resources/Prototypes/Recipes/Lathes/security.yml
🧰 Additional context used
🪛 yamllint
Resources/Migrations/migration.yml
[error] 256-256: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (15)
Content.Server/_White/Holosign/HolosignComponent.cs (1)
1-3: LGTM! Namespace and component registration are properly implemented.
Content.Server/_White/Holosign/HolosignSystem.cs (2)
1-4: LGTM! Clean and focused imports.
The imports are minimal and directly related to the system's functionality.
6-21: Verify completeness of the implementation.
Based on the AI summary, several expected features appear to be missing:
- OnUse handler for projector usage
- OnExamine functionality
- Sign spawning logic
Let's verify if these features exist elsewhere in the codebase:
Resources/Prototypes/Recipes/Lathes/janitorial.yml (2)
69-69: LGTM! Recipe update aligns with holoprojector system changes.
The change from HoloprojectorEmpty to Holoprojector is consistent with the removal of the charge-based system in favor of the new usage-based system.
69-69: Verify consistency of holoprojector recipe changes.
Let's ensure similar recipe updates were applied consistently across all holoprojector variants.
✅ Verification successful
Holoprojector recipe changes are consistent
All holoprojector-related recipes have been updated consistently:
- Regular Holoprojector in janitorial.yml
- HoloprojectorSecurity in security.yml
- HoloprojectorField in tools.yml
- HolofanProjector in tools.yml
All recipes have the "# WD EDIT" comment and none contain "Empty" variants, confirming the changes were applied uniformly across all variants.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to empty holoprojector variants in recipes
# Expected: No matches for *Empty variants, confirming consistent updates
echo "Checking for any remaining references to empty holoprojector variants in recipes..."
rg -l "result: .*HoloprojectorEmpty" "Resources/Prototypes/Recipes"
echo "Checking for any remaining references to empty holofan/field variants in recipes..."
rg -l "result: .*Holo(fan|field).*Empty" "Resources/Prototypes/Recipes"
Length of output: 493
Script:
#!/bin/bash
# Let's check all holoprojector-related recipes to verify consistency
echo "Checking all holoprojector recipes and variants..."
rg -A 5 -B 5 "result:.*[Hh]oloprojector" "Resources/Prototypes/Recipes"
echo -e "\nChecking all holofan/field recipes..."
rg -A 5 -B 5 "result:.*[Hh]olo(fan|field)" "Resources/Prototypes/Recipes"
Length of output: 3211
Resources/Prototypes/Entities/Objects/Devices/holoprojectors.yml (1)
11-11: Verify the gameplay impact of the 0.5s use delay.
The addition of a 0.5-second delay between uses could affect gameplay mechanics. Please ensure this delay provides a good balance between preventing spam usage while maintaining responsive controls.
Consider documenting the rationale for this specific delay value in the code comment instead of just marking it as an edit.
Resources/Prototypes/Entities/Structures/Holographic/projections.yml (1)
Line range hint 1-140: Verify the removal of TimedDespawn components.
The AI summary indicates that TimedDespawn components were removed from all holosign entities. This architectural change from temporary to permanent holosigns could have significant gameplay implications:
- Resource Management: Without auto-despawn, players need to manually manage/cleanup holosigns
- Server Performance: Permanent holosigns could accumulate over time
- Game Balance: Infinite duration might make these tools too powerful
Let's confirm the impact and current usage:
Consider implementing one of these alternatives:
- Add a manual "disable" interaction using the new Clickable component
- Implement a cleanup system that removes unused holosigns after a certain period
- Add a maximum concurrent holosigns limit per projector
Content.Server/Holosign/HolosignSystem.cs (3)
1-23: LGTM! Dependencies and initialization are properly structured.
The new dependencies and event subscriptions align well with the system's functionality.
48-48: Good addition of storage component check.
The storage component check prevents unintended interactions when storing the projector.
74-79: 🛠️ Refactor suggestion
Consider adding error handling for sign creation.
The sign creation process should handle potential failures in component addition and entity spawning.
-EnsureComp<HolosignComponent>(holoUid, out var holosign);
-component.Signs.Add(holoUid);
-holosign.Sign = uid;
+if (TryComp<HolosignComponent>(holoUid, out var holosign))
+{
+ component.Signs.Add(holoUid);
+ holosign.Sign = uid;
+}
+else
+{
+ QueueDel(holoUid);
+ _popup.PopupEntity(Loc.GetString("holoprojector-creation-failed"), args.User, args.User, PopupType.Error);
+ return;
+}Resources/Prototypes/Recipes/Lathes/tools.yml (2)
192-192: LGTM: Recipe result update aligns with system redesign.
The change from HolofanProjectorEmpty to HolofanProjector correctly reflects the architectural shift from charge-based to count-based usage model.
241-241: LGTM: Recipe result update aligns with system redesign.
The change from HoloprojectorFieldEmpty to HoloprojectorField correctly reflects the architectural shift from charge-based to count-based usage model.
Let's verify that all holoprojector-related recipes have been updated consistently:
✅ Verification successful
All holoprojector recipes have been updated consistently
Based on the search results, all holoprojector recipes (Holoprojector, HoloprojectorField, and HoloprojectorSecurity) have been updated to use the new count-based model, with no remaining references to "Empty" variants. Each recipe has the "# WD EDIT" comment indicating they were part of the same system redesign.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to 'Empty' holoprojectors
# across all recipe files
# Test: Search for any remaining 'Empty' variants in recipes
rg -l "result: .*Holoprojector.*Empty" "Resources/Prototypes/Recipes"
Length of output: 69
Script:
#!/bin/bash
# Let's do a broader search to understand the holoprojector recipes and entities
# First, find all holoprojector-related recipes
rg -i "holoprojector" "Resources/Prototypes/Recipes" -A 5 -B 5
# Also search for holoprojector entities to understand the available types
rg -i "holoprojector" "Resources/Prototypes/Entities" -A 5 -B 5
Length of output: 13883
Resources/Migrations/migration.yml (2)
249-250: LGTM: VendingMachine mapping preserved.
The mapping of VendingMachinePride to RandomVending is maintained correctly.
251-256: Verify holoprojector entity consolidation.
The mappings align with the holoprojector redesign objectives, consolidating specialized variants into their base types. This matches the broader system changes described in the PR.
Let's verify that these entities are properly handled in the codebase:
✅ Verification successful
Let me check the actual content of these files to verify the entity definitions and their usage.
Holoprojector entity consolidation verified successfully
The verification confirms proper implementation of the holoprojector consolidation:
- All old entity IDs (Empty/Borg variants) are completely removed from the prototypes
- Base entities (Holoprojector, HolofanProjector, HoloprojectorField, HoloprojectorSecurity) are properly defined with correct inheritance
- Manufacturing recipes are updated to use the new consolidated entities
- Component configurations are maintained appropriately for each variant
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that old holoprojector entities are properly migrated
# Test 1: Check if old entity IDs are completely removed from prototypes
echo "Checking for any remaining references to old holoprojector entities..."
rg -l "HoloprojectorEmpty|HoloprojectorBorg|HolofanProjectorEmpty|HoloprojectorFieldEmpty|HoloprojectorSecurityEmpty" Resources/Prototypes/
# Test 2: Verify new entity definitions exist
echo "Verifying new holoprojector entity definitions..."
rg -l "^(?: )?id: (Holoprojector|HolofanProjector|HoloprojectorField|HoloprojectorSecurity)$" Resources/Prototypes/
Length of output: 761
Script:
#!/bin/bash
# Check the content of holoprojectors.yml for entity definitions
echo "=== Checking holoprojectors.yml ==="
cat "Resources/Prototypes/Entities/Objects/Devices/holoprojectors.yml"
# Check recipes to ensure they reference correct entities
echo -e "\n=== Checking recipes ==="
cat "Resources/Prototypes/Recipes/Lathes/janitorial.yml" "Resources/Prototypes/Recipes/Lathes/security.yml" "Resources/Prototypes/Recipes/Lathes/tools.yml" | grep -A 5 "Holoprojector"
# Check tags file
echo -e "\n=== Checking tags.yml ==="
cat "Resources/Prototypes/tags.yml" | grep -A 2 -B 2 "Holoprojector"
Length of output: 2908
🧰 Tools
🪛 yamllint
[error] 256-256: no new line character at the end of file
(new-line-at-end-of-file)
Resources/Prototypes/Entities/Objects/Specific/Robotics/borg_modules.yml (1)
346-346: Verify the Holoprojector integration.
The addition of Holoprojector to the advanced cleaning module aligns with the PR's objective of redesigning holoprojectors. However, let's verify a few aspects:
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Holoprojector integration is properly configured and balanced
The Holoprojector entity is well-defined in Resources/Prototypes/Entities/Objects/Devices/holoprojectors.yml with appropriate components and configurations. It's a janitorial tool that projects holographic signs, which thematically fits with the advanced cleaning module. The item is:
- Craftable in janitorial lathe with balanced material costs (250 Plastic, 150 Glass)
- Already integrated into service/janitorial loadouts
- Part of the standard janitorial equipment set
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the Holoprojector entity exists and check its configuration
# Test 1: Check if Holoprojector entity exists
echo "Checking for Holoprojector entity definition..."
rg -l "^- type: entity\n\s+id: Holoprojector\b" "Resources/Prototypes"
# Test 2: Check for any balance-related configurations
echo "Checking for Holoprojector balance configurations..."
rg "Holoprojector" "Resources/Prototypes" -A 5
Length of output: 17711
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
Content.Server/_White/Holosign/HolosignComponent.cs (1)
1-5: Add XML documentation to improve code clarity.The component would benefit from XML documentation explaining:
- The purpose of this component
- Its relationship with HolosignProjectorComponent
- When it's attached/detached
namespace Content.Server._White.Holosign; +/// <summary> +/// Component attached to holosign entities that are created by a <see cref="HolosignProjectorComponent"/>. +/// Tracks the projector that created this holosign for cleanup and management purposes. +/// </summary> [RegisterComponent] public sealed partial class HolosignComponent : ComponentContent.Server/Holosign/HolosignSystem.cs (2)
Line range hint
67-73: Add validation for SignProto before spawning.The code attempts to spawn an entity without validating if the prototype exists.
Add prototype validation:
+ if (!_prototypeManager.HasIndex<EntityPrototype>(component.SignProto)) + { + Log.Error($"Invalid SignProto '{component.SignProto}' in HolosignProjectorComponent on {ToPrettyString(uid)}"); + return; + } + var holoUid = EntityManager.SpawnEntity(component.SignProto, args.ClickLocation.SnapToGrid(EntityManager));
Line range hint
1-100: Consider separating sign management into a dedicated service.The current design combines sign projection, tracking, and cleanup in a single system. Consider extracting sign management into a dedicated service to improve maintainability and testability.
This would:
- Simplify the HolosignSystem
- Make sign tracking more robust
- Enable better testing of sign management logic
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
Content.Server/Holosign/HolosignSystem.cs(2 hunks)Content.Server/_White/Holosign/HolosignComponent.cs(1 hunks)Content.Server/_White/Holosign/HolosignSystem.cs(1 hunks)Resources/Prototypes/Nyanotrasen/Entities/Objects/Devices/holoprojectors.yml(0 hunks)
💤 Files with no reviewable changes (1)
- Resources/Prototypes/Nyanotrasen/Entities/Objects/Devices/holoprojectors.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- Content.Server/_White/Holosign/HolosignSystem.cs
🧰 Additional context used
📓 Learnings (1)
Content.Server/_White/Holosign/HolosignComponent.cs (1)
Learnt from: Remuchi
PR: WWhiteDreamProject/wwdpublic#112
File: Content.Server/Holosign/HolosignProjectorComponent.cs:20-21
Timestamp: 2024-10-31T02:46:05.354Z
Learning: In `HolosignProjectorComponent`, the `Signs` list does not need the `DataField` attribute as it doesn't need to be serialized.
🔇 Additional comments (1)
Content.Server/Holosign/HolosignSystem.cs (1)
1-23: LGTM!
The dependencies and initialization logic are properly structured.
Описание PR
Переработка голопроекторов, теперь они больше похоже на свой вариант из 13-ой станции.
Изменения
🆑 Spatison