Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces storage/jug-based dispenser UI and messages with prototype-driven reagent inventories and ReagentId-based actions. Server builds inventory from new inventory prototypes, integrates charges, and dispenses by ReagentId; client UIs and shared messages/state updated accordingly; storage/eject flows removed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as "Player Client"
participant UI as "Dispenser UI (Client)"
participant BUI as "Bound UI (Client→Server)"
participant Server as "ReagentDispenserSystem (Server)"
participant Prototype as "PrototypeManager"
participant Charges as "ChargesSystem"
participant Solution as "SolutionContainerSystem"
Client->>UI: Click Reagent Card (ReagentId)
UI->>BUI: Send ReagentDispenserDispenseReagentMessage(ReagentId)
BUI->>Server: Forward message
Server->>Prototype: Resolve ReagentId → ReagentPrototype/base cost
Server->>Charges: Check/consume charges (affect amount/cost)
Server->>Solution: Transfer computed reagent amount to container
Solution-->>Server: Transfer result
Server-->>BUI: Push updated UI state (inventory, charges)
BUI-->>UI: Update reagent list and charge display
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Resources/Prototypes/Entities/Structures/Dispensers/base_structuredispensers.yml (1)
81-82:⚠️ Potential issue | 🟡 MinorRemove the orphaned StorageUiKey interface definition.
The
StorageBoundUserInterfaceforenum.StorageUiKey.Key(lines 81-82) is unused. The Storage component was removed from the base entity and no child dispenser entities add it back, making this interface dead configuration. Remove it to clean up the configuration.Content.Shared/Chemistry/SharedReagentDispenser.cs (1)
111-118:⚠️ Potential issue | 🟡 MinorRemove unused
ReagentInventoryItemclass.The class is not referenced anywhere in the codebase. Delete it from lines 111-118.
🤖 Fix all issues with AI agents
In `@Content.Client/Chemistry/UI/ReagentCardControl.xaml.cs`:
- Line 8: Remove the now-unused using directive "using Content.Shared.Storage;"
from ReagentCardControl.xaml.cs; the refactor replaced types from that namespace
with ReagentId, so delete the import to avoid an unused-usings warning and keep
the file clean.
In `@Content.Client/Chemistry/UI/ReagentDispenserWindow.xaml`:
- Around line 68-69: The SpriteView with Name="View" is missing Access="Public"
but is referenced from code-behind (e.g., View.SetEntity()), so add
Access="Public" to the SpriteView declaration to match other named controls
(AmountGrid, ReagentList, ClearButton, EjectButton, ContainerInfoName,
ContainerInfoFill) and ensure it is accessible from the code-behind.
In `@Content.Server/Chemistry/EntitySystems/ReagentDispenserSystem.cs`:
- Around line 218-219: The code throws a bare Exception on a null `solution` (in
ReagentDispenserSystem) which is unhelpful; replace the null-check expression in
the call to _solutionContainerSystem.TryAddReagent so it either uses the
null-forgiving operator (solution!) since TryGetFitsInDispenser is annotated
[NotNullWhen(true]), or, if you want runtime safety, throw a more specific
exception with a clear message (e.g., InvalidOperationException with context)
before calling TryAddReagent; update the call site that currently reads
"solution ?? throw new()" to use "solution!" (or throw new
InvalidOperationException(\"Reagent dispenser solution was null despite
TryGetFitsInDispenser returning true\") if you prefer explicit error).
In `@Content.Shared/Chemistry/Dispenser/ReagentDispenserInventoryPrototype.cs`:
- Around line 13-17: The XML doc comment for ReagentDispenserInventoryPrototype
contains a typo ("despenser") — update the summary text in the
ReagentDispenserInventoryPrototype XML documentation to replace "despenser" with
"dispenser" so the comment reads "reagent dispenser <c>pack</c> value" (locate
the summary block in ReagentDispenserInventoryPrototype and fix the word).
🧹 Nitpick comments (9)
Content.Server/Chemistry/Components/ReagentDispenserComponent.cs (1)
83-89: Remove redundantdefault!initialization on nullable fields.Both
PackPrototypeIdandEmagPackPrototypeIdare declared asstring?(nullable), yet they're initialized with= default!. The null-forgiving operator (!) is misleading here since the fields can legitimately be null (as indicated by the?). For nullable reference types, omitting the initializer or using= nullis clearer.♻️ Suggested simplification
[DataField("pack", customTypeSerializer: typeof(PrototypeIdSerializer<ReagentDispenserInventoryPrototype>))] [ViewVariables(VVAccess.ReadWrite)] -public string? PackPrototypeId = default!; +public string? PackPrototypeId; [DataField("emagPack", customTypeSerializer: typeof(PrototypeIdSerializer<ReagentDispenserInventoryPrototype>))] [ViewVariables(VVAccess.ReadWrite)] -public string? EmagPackPrototypeId = default!; +public string? EmagPackPrototypeId;Content.Shared/Chemistry/Dispenser/ReagentDispenserInventoryPrototype.cs (2)
5-9: Remove unusedusingdirectives.The imports
System,System.Linq,System.Text, andSystem.Threading.Tasksappear unused in this file.♻️ Suggested cleanup
using Content.Shared.Chemistry.Reagent; using Robust.Shared.Prototypes; using Robust.Shared.Serialization; using Robust.Shared.Serialization.TypeSerializers.Implementations.Custom.Prototype.List; -using System; using System.Collections.Generic; -using System.Linq; -using System.Text; -using System.Threading.Tasks;
26-28: Acknowledge TODO: Consider usingReagentIdinstead ofList<string>.The TODO comment indicates intent to use
ReagentIdfor type safety. This would provide better compile-time validation and consistency with other parts of the codebase that now useReagentId.Would you like me to open an issue to track this improvement, or would you prefer to address it in this PR?
Content.Client/Chemistry/UI/ReagentDispenserWindow.xaml.cs (2)
82-88: Sorting with null prototypes may produce inconsistent results.When
x.prototypeis null, theCompareTocall returns 0 regardless ofy, causing items with missing prototypes to be interleaved inconsistently rather than grouped together.♻️ Proposed fix for stable null handling in sort
- // Sort reagents by name - reagentPrototypes.Sort((x, y) => x.prototype?.LocalizedName?.CompareTo(y.prototype?.LocalizedName ?? untranslatedText) ?? 0); + // Sort reagents by name, placing missing prototypes at the end + reagentPrototypes.Sort((x, y) => + string.Compare( + x.prototype?.LocalizedName ?? untranslatedText, + y.prototype?.LocalizedName ?? untranslatedText, + StringComparison.CurrentCulture));
85-85: Minor typo in comment."not that much reagents" should be "not that many reagents".
Resources/Prototypes/Catalog/reagent_dispenser.yml (1)
60-65: Inconsistent YAML indentation for inventory items.
SodaDispenserEmagInventoryandBoozeDispenserEmagInventoryuse 4-space indentation for inventory items (lines 63-65, 91-93), while other inventories use 2-space indentation. While YAML parsers will accept both, consistent formatting improves readability.♻️ Proposed fix for consistent indentation
- type: reagentDispenserInventory id: SodaDispenserEmagInventory inventory: - - FourteenLoko - - Ephedrine - - Histamine + - FourteenLoko + - Ephedrine + - Histamine- type: reagentDispenserInventory id: BoozeDispenserEmagInventory inventory: - - AtomicBomb - - Ethanol - - Iron + - AtomicBomb + - Ethanol + - IronAlso applies to: 88-93
Content.Server/Chemistry/EntitySystems/ReagentDispenserSystem.cs (2)
167-167: Typo in comment: "Finded" should be "Found".
137-173: Use the deserialized component object instead of manual YAML traversal.This code manually navigates prototype YAML nodes to extract reagent IDs. A more robust approach exists in the codebase: deserialize the
SolutionContainerManagercomponent data and use the system'sEnumerateSolutions()method, as demonstrated inPricingSystem.cs. The current manual traversal hardcodes the YAML structure ("solutions" → "reagents" → "ReagentId") and will silently fail if the schema changes. Consider castingdata.ComponenttoSolutionContainerManagerComponentand leveraging_solutionContainerSystem.EnumerateSolutions()for type-safe iteration instead.Content.Shared/Chemistry/SharedReagentDispenser.cs (1)
86-88: Parameter namestorageLocationis misleading for aReagentIdtype.The parameter and assignment retain the old naming from when this held an
ItemStorageLocation. Rename for clarity.♻️ Proposed fix
- public ReagentDispenserDispenseReagentMessage(ReagentId storageLocation) + public ReagentDispenserDispenseReagentMessage(ReagentId reagentId) { - ReagentId = storageLocation; + ReagentId = reagentId; }
Content.Server/Chemistry/EntitySystems/ReagentDispenserSystem.cs
Outdated
Show resolved
Hide resolved
Content.Shared/Chemistry/Dispenser/ReagentDispenserInventoryPrototype.cs
Show resolved
Hide resolved
|
Changelog
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@Content.Client/Chemistry/UI/ReagentCardControl.xaml.cs`:
- Around line 19-31: The constructor of ReagentCardControl fails to assign the
incoming cost to the Cost property so internal state remains stale; inside the
ReagentCardControl(ReagentId reagentId, ReagentPrototype? prototype, int cost)
constructor assign the passed-in cost to the Cost property (e.g., set Cost =
cost) after setting ReagentId so any code that reads Cost later sees the correct
value.
In `@Content.Server/Chemistry/EntitySystems/ReagentDispenserSystem.cs`:
- Around line 169-237: GetInventory is using Dictionary.Add for multiple
inventory sources which throws on duplicate reagent keys; change all
inventory.Add(...) calls (inside the SolutionContainerManager loop and when
merging packPrototype.Inventory and emagPackPrototype.Inventory) to a
deterministic upsert (overwrite) pattern so later sources replace earlier ones —
e.g., replace Add with assignment/overwriting (inventory[reagentKey] = value) or
a TryAdd-if-not-present depending on desired precedence; ensure this change is
applied for the reagentId.Value insertion in the SolutionContainerManager block
and for the loops that iterate packPrototype.Inventory and
emagPackPrototype.Inventory so duplicates no longer throw.
- Around line 246-287: The calculation limiting totalDispenseAmount uses integer
division (avalaibleCharges / singleCost) which underestimates available partial
dispenses when singleCost > 1; in OnDispenseReagentMessage change that division
to float division (e.g., cast avalaibleCharges or singleCost to float) so
totalDispenseAmount = avalaibleCharges / (float) singleCost (or equivalent) to
allow fractional dispensing; keep the rest of the charge handling logic the same
and ensure the Ceil/charge deduction still rounds up correctly.
In `@Resources/Prototypes/Entities/Structures/Dispensers/chem.yml`:
- Around line 57-61: The YAML contains a dangling comment stating "Doesn't work"
for the LimitedCharges field lastCharges — either remove the non-functional
field or document and track the bug: confirm whether the LimitedCharges
component supports lastCharges (search for LimitedCharges implementation/usage),
if unsupported remove lastCharges from the chem.yml entry and rely on maxCharges
+ AutoRecharge (rechargeDuration), otherwise add a clear FIXME and create a
tracking issue that documents expected vs actual behavior and include
reproduction steps; update the YAML comment to reference the issue ID if leaving
a temporary note.
🧹 Nitpick comments (4)
Content.Shared/Chemistry/SharedReagentDispenser.cs (1)
20-20: Unused import.
Robust.Shared.Toolshed.TypeParsers.Mathdoes not appear to be used in this file. Consider removing it.🧹 Proposed fix
-using Robust.Shared.Toolshed.TypeParsers.Math;Content.Server/Chemistry/Components/ReagentDispenserComponent.cs (1)
89-95: Inconsistent nullability:default!on nullable fields.
PackPrototypeIdandEmagPackPrototypeIdare declared asstring?(nullable) but initialized withdefault!, which suppresses the nullable warning. This is misleading—if these fields are meant to be optional, initialize with= nullinstead. If they are required, remove the?.♻️ Proposed fix
[DataField("pack", customTypeSerializer: typeof(PrototypeIdSerializer<ReagentDispenserInventoryPrototype>))] [ViewVariables(VVAccess.ReadWrite)] - public string? PackPrototypeId = default!; + public string? PackPrototypeId; [DataField("emagPack", customTypeSerializer: typeof(PrototypeIdSerializer<ReagentDispenserInventoryPrototype>))] [ViewVariables(VVAccess.ReadWrite)] - public string? EmagPackPrototypeId = default!; + public string? EmagPackPrototypeId;Content.Shared/Chemistry/Dispenser/ReagentDispenserInventoryPrototype.cs (1)
5-9: Remove unusedSystem.*imports.The following imports are not used in this file:
System,System.Text,System.Threading.Tasks, andSystem.Linq.🧹 Proposed fix
using Robust.Shared.Prototypes; using Robust.Shared.Serialization; using Robust.Shared.Serialization.TypeSerializers.Implementations.Custom.Prototype.List; -using System; -using System.Collections.Generic; -using System.Linq; -using System.Text; -using System.Threading.Tasks; +using System.Collections.Generic;Content.Client/Chemistry/UI/ReagentDispenserWindow.xaml.cs (1)
91-92: Sorting comparison may not handle null names consistently.When
x.prototypeory.prototypeis null, the entire expression evaluates to0, meaning items with null prototypes won't sort deterministically relative to each other or to valid items. Consider using a more explicit comparison:♻️ Proposed fix for more predictable sorting
// Sort reagents by name - reagentPrototypes.Sort((x, y) => x.prototype?.LocalizedName?.CompareTo(y.prototype?.LocalizedName ?? untranslatedText) ?? 0); + reagentPrototypes.Sort((x, y) => + string.Compare( + x.prototype?.LocalizedName ?? untranslatedText, + y.prototype?.LocalizedName ?? untranslatedText, + StringComparison.CurrentCulture));
|
Хсквиз далее подгонит UI. |
|
WIP, добавлю комментарии MAID (и пофикшу другие контрибут гайдлайны) на все изменения чуть позже |
|
Добавил ко всем изменениям метку Maid |
|
Похоже у LimitedCharges компонента нельзя указать начальное значение, поэтому lastCharges не учитывается. В прочем наверное не имеет значение |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@Content.Client/Chemistry/UI/ReagentDispenserWindow.xaml.cs`:
- Line 47: The Maid START/END markers currently wrap the entire namespace/class;
remove the `// Maid START PR №21` and `// Maid END` that enclose the whole file
and instead place them only around the actual modifications: the event
declaration you changed, the UpdateReagentsList method, and the charge display
logic inside the ReagentDispenserWindow class. Ensure the markers use the exact
comments `// Maid START PR №21` and `// Maid END`, are balanced, and only
surround the minimal changed blocks (event declaration, method
UpdateReagentsList, and the charge display code) so unchanged code is not
enclosed.
- Around line 83-100: The current Sort comparator on reagentPrototypes can
return 0 when x.prototype is null, making the sort unstable; update the
comparator used on reagentPrototypes so it compares x.prototype.LocalizedName
and y.prototype.LocalizedName when both prototypes exist, and when one or both
prototypes are null fall back to comparing the reagent prototype IDs (use the
reagent.Prototype string from the tuple) or another stable reagent identifier
before returning 0; modify the Sort lambda that references
ReagentPrototype.LocalizedName to implement this three-way fallback (localized
name -> prototype id -> final tie-breaker such as reagent.Prototype.CompareTo)
so unknown prototypes sort deterministically.
In `@Resources/Prototypes/Entities/Structures/Dispensers/booze.yml`:
- Around line 159-166: The YAML uses lastCharges: 0 which is treated as
"uninitialized" by the OnChargesMapInit logic in SharedChargesSystem.cs (it
treats LastCharges == 0 as unset and resets to MaxCharges), so change the YAML
entry for the Booze dispenser (the LimitedCharges block with lastCharges) to use
lastCharges: -1 to produce an empty spawn (the map-init will clamp -1 to 0);
reference OnChargesMapInit / LastCharges / MaxCharges when locating the logic if
you instead prefer to adjust code behavior or add a clarifying comment.
In `@Resources/Prototypes/Entities/Structures/Dispensers/soda.yml`:
- Around line 164-168: The YAML sets lastCharges: 0 which is ineffective because
SharedChargesSystem.OnChargesMapInit treats 0 as a sentinel and replaces
ent.Comp.LastCharges with MaxCharges; update the Dispensers entries (soda.yml,
and the similar chem.yml and booze.yml instances) to use lastCharges: -1 if you
want an empty start (the system clamps -1 to 0), or remove the lastCharges field
entirely; reference SharedChargesSystem.OnChargesMapInit, ent.Comp.LastCharges,
and MaxCharges when making the change so you alter the correct YAML blocks.
🧹 Nitpick comments (9)
Content.Shared/Chemistry/Dispenser/ReagentDispenserInventoryPrototype.cs (1)
1-31: Unusedusingdirectives and leftover TODO.Lines 5–11 include several
usingdirectives (System.Text,System.Threading.Tasks,System.Linq, and thePrototype.Listserializer) that don't appear to be referenced anywhere in this file. While the TODO on Line 28 (// TODO use ReagentId) is a valid improvement note, consider tracking it in an issue so it doesn't get lost.Would you like me to open an issue to track the
ReagentIdmigration for the inventory dictionary?Resources/Prototypes/Entities/Structures/Dispensers/base_structuredispensers.yml (1)
81-82: Prefer removing dead code over commenting it out.Multiple blocks of YAML are commented out with
# Maid PR №21markers. Version control already preserves history — leaving large commented-out blocks adds noise for future maintainers. Consider deleting them and referencing the PR number in a single short comment or commit message instead.Also applies to: 105-110, 124-124
Content.Client/Chemistry/UI/ReagentDispenserBoundUserInterface.cs (1)
71-77: Remove commented-out old code.Same pattern as other files — the old bindings on lines 72–74 should be deleted rather than kept as block comments. The PR reference is sufficient context.
Content.Server/Chemistry/Components/ReagentDispenserComponent.cs (1)
91-97:default!on nullable fields is misleading.
PackPrototypeIdandEmagPackPrototypeIdare declaredstring?(nullable), yet initialized withdefault!(null-forgiving). This suppresses nullability warnings on a type that already permits null, which is confusing. Use= null;or omit the initializer entirely.📝 Proposed fix
[DataField("pack", customTypeSerializer: typeof(PrototypeIdSerializer<ReagentDispenserInventoryPrototype>))] [ViewVariables(VVAccess.ReadWrite)] - public string? PackPrototypeId = default!; + public string? PackPrototypeId; [DataField("emagPack", customTypeSerializer: typeof(PrototypeIdSerializer<ReagentDispenserInventoryPrototype>))] [ViewVariables(VVAccess.ReadWrite)] - public string? EmagPackPrototypeId = default!; + public string? EmagPackPrototypeId;Content.Server/Chemistry/EntitySystems/ReagentDispenserSystem.cs (4)
100-116: Update loop placement and unused component variable.The
_updateTimerfield is declared between methods rather than with other fields at the top of the class. Also,uiCompandchargesfrom the query enumerator on line 109 are never read — they only serve as query filters. This is fine functionally but worth noting for clarity.
274-277: Addingfloat.Epsilonbefore int cast can over-count charges.Line 276:
(int)(MathF.Ceiling((float)dispensed * singleCost) + float.Epsilon)— ifCeilingreturns an exact integer value (e.g.,3.0f), addingfloat.Epsilonthen truncating with(int)is harmless in practice becausefloat.Epsilonis ~1.4e-45, far too small to push past the next integer. However, the intent is unclear and the comment "should be safe" suggests uncertainty. Consider using(int)MathF.Ceiling(dispensed * singleCost)directly —Ceilingalready rounds up, so the epsilon guard is unnecessary.Proposed simplification
- _chargesSystem.AddCharges(reagentDispenser.Owner, -((int) (MathF.Ceiling((float) dispensed * singleCost) + float.Epsilon))); // Ceil dispensed amount up (should be safe) + _chargesSystem.AddCharges(reagentDispenser.Owner, -(int)MathF.Ceiling((float)dispensed * singleCost));
163-201: Fragile raw YAML node traversal for backward compatibility.The
GetInventorymethod manually walksMappingDataNode/SequenceDataNodetrees to extract reagent IDs fromStorageFillentity prototypes. This is brittle — any structural change toSolutionContainerManagerserialization will silently break it without compile-time errors.A few minor items in this block:
- Line 197:
// Finded!→// Found!(typo)- Line 258:
"less then required"→"less than required"- Line 264:
avalaibleCharges→availableChargesGiven that the PR comments mention this is WIP and will be cleaned up later, flagging for awareness.
240-253:TryFirstOrNullwith LINQ onIEnumerablere-enumeratesGetInventory.
GetInventoryreturnsIEnumerable(viainventory.Select(...)) built on top ofDictionary. On line 250,GetInventoryis called and the result is iterated withTryFirstOrNull. On line 128,GetInventoryis called again and.ToList()is used. InOnDispenseReagentMessage, the full dictionary is built just to find one element. Consider caching or passing the inventory if this becomes a hot path.Resources/Prototypes/Entities/Structures/Dispensers/booze.yml (1)
114-142: Commented-out code blocks left in place.Large blocks of commented-out YAML (lines 115–133, 160–163) are retained. Per the PR comments, this is WIP and will be cleaned up. Ensure these are removed before merging to keep prototypes clean.
| using System.Linq; | ||
| using static Robust.Client.UserInterface.Controls.BoxContainer; | ||
|
|
||
| // Maid START PR №21 |
There was a problem hiding this comment.
Maid START/END markers wrap the entire namespace — should only wrap changed sections.
The // Maid START PR №21 on Line 47 and // Maid END on Line 179 enclose the entire class, including large sections of unchanged code. This makes it impossible to distinguish what was actually modified. Move the markers to only wrap the specific changed segments (e.g., the event declaration, UpdateReagentsList, and the charge display logic).
Also applies to: 177-179
🤖 Prompt for AI Agents
In `@Content.Client/Chemistry/UI/ReagentDispenserWindow.xaml.cs` at line 47, The
Maid START/END markers currently wrap the entire namespace/class; remove the `//
Maid START PR №21` and `// Maid END` that enclose the whole file and instead
place them only around the actual modifications: the event declaration you
changed, the UpdateReagentsList method, and the charge display logic inside the
ReagentDispenserWindow class. Ensure the markers use the exact comments `// Maid
START PR №21` and `// Maid END`, are balanced, and only surround the minimal
changed blocks (event declaration, method UpdateReagentsList, and the charge
display code) so unchanged code is not enclosed.
| var reagentPrototypes = inventory | ||
| // Convert reagentId[] to (reagentId, prototype)[] | ||
| .Select(reagentPair => ( | ||
| reagentPair.reagent, | ||
| prototype: _prototypeManager.TryIndex(reagentPair.reagent.Prototype, out ReagentPrototype? prototype) ? prototype : null, | ||
| reagentPair.cost | ||
| )) | ||
| .ToList(); // Copy, but should be fine since there not that much reagents | ||
|
|
||
| // Sort reagents by name | ||
| reagentPrototypes.Sort((x, y) => x.prototype?.LocalizedName?.CompareTo(y.prototype?.LocalizedName ?? untranslatedText) ?? 0); | ||
|
|
||
| foreach (var item in reagentPrototypes) | ||
| { | ||
| var card = new ReagentCardControl(item); | ||
| var card = new ReagentCardControl(item.reagent, item.prototype, item.cost * amount); | ||
| card.OnPressed += OnDispenseReagentButtonPressed; | ||
| card.OnEjectButtonPressed += OnEjectJugButtonPressed; | ||
| ReagentList.Children.Add(card); | ||
| } |
There was a problem hiding this comment.
Sort comparator doesn't handle null prototypes consistently.
On Line 93, when x.prototype is null, the null-conditional chain (x.prototype?.LocalizedName?.CompareTo(...)) evaluates to null, and ?? 0 makes the comparison return 0 — meaning unknown reagents are "equal" to every other reagent, producing an unstable/arbitrary sort position. Consider falling back to a string comparison on the reagent ID for null prototypes.
📝 Proposed fix
- reagentPrototypes.Sort((x, y) => x.prototype?.LocalizedName?.CompareTo(y.prototype?.LocalizedName ?? untranslatedText) ?? 0);
+ reagentPrototypes.Sort((x, y) =>
+ string.Compare(
+ x.prototype?.LocalizedName ?? x.reagent.Prototype,
+ y.prototype?.LocalizedName ?? y.reagent.Prototype,
+ StringComparison.CurrentCulture));📝 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.
| var reagentPrototypes = inventory | |
| // Convert reagentId[] to (reagentId, prototype)[] | |
| .Select(reagentPair => ( | |
| reagentPair.reagent, | |
| prototype: _prototypeManager.TryIndex(reagentPair.reagent.Prototype, out ReagentPrototype? prototype) ? prototype : null, | |
| reagentPair.cost | |
| )) | |
| .ToList(); // Copy, but should be fine since there not that much reagents | |
| // Sort reagents by name | |
| reagentPrototypes.Sort((x, y) => x.prototype?.LocalizedName?.CompareTo(y.prototype?.LocalizedName ?? untranslatedText) ?? 0); | |
| foreach (var item in reagentPrototypes) | |
| { | |
| var card = new ReagentCardControl(item); | |
| var card = new ReagentCardControl(item.reagent, item.prototype, item.cost * amount); | |
| card.OnPressed += OnDispenseReagentButtonPressed; | |
| card.OnEjectButtonPressed += OnEjectJugButtonPressed; | |
| ReagentList.Children.Add(card); | |
| } | |
| var reagentPrototypes = inventory | |
| // Convert reagentId[] to (reagentId, prototype)[] | |
| .Select(reagentPair => ( | |
| reagentPair.reagent, | |
| prototype: _prototypeManager.TryIndex(reagentPair.reagent.Prototype, out ReagentPrototype? prototype) ? prototype : null, | |
| reagentPair.cost | |
| )) | |
| .ToList(); // Copy, but should be fine since there not that much reagents | |
| // Sort reagents by name | |
| reagentPrototypes.Sort((x, y) => | |
| string.Compare( | |
| x.prototype?.LocalizedName ?? x.reagent.Prototype, | |
| y.prototype?.LocalizedName ?? y.reagent.Prototype, | |
| StringComparison.CurrentCulture)); | |
| foreach (var item in reagentPrototypes) | |
| { | |
| var card = new ReagentCardControl(item.reagent, item.prototype, item.cost * amount); | |
| card.OnPressed += OnDispenseReagentButtonPressed; | |
| ReagentList.Children.Add(card); | |
| } |
🤖 Prompt for AI Agents
In `@Content.Client/Chemistry/UI/ReagentDispenserWindow.xaml.cs` around lines 83 -
100, The current Sort comparator on reagentPrototypes can return 0 when
x.prototype is null, making the sort unstable; update the comparator used on
reagentPrototypes so it compares x.prototype.LocalizedName and
y.prototype.LocalizedName when both prototypes exist, and when one or both
prototypes are null fall back to comparing the reagent prototype IDs (use the
reagent.Prototype string from the tuple) or another stable reagent identifier
before returning 0; modify the Sort lambda that references
ReagentPrototype.LocalizedName to implement this three-way fallback (localized
name -> prototype id -> final tie-breaker such as reagent.Prototype.CompareTo)
so unknown prototypes sort deterministically.
| ## Maid START PR №21 | ||
| # - type: Storage | ||
| # whitelist: | ||
| # tags: | ||
| # - DrinkBottle | ||
| - type: LimitedCharges | ||
| lastCharges: 0 # Doesn't work, search for comment in PR 21 for more info | ||
| ## Maid END |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the booze.yml file
find . -name "booze.yml" -type fRepository: TheMaidDev/Maid-14
Length of output: 125
🏁 Script executed:
# Search for LimitedChargesComponent to understand how lastCharges is used
rg -l "LimitedCharges" --type cs | head -20Repository: TheMaidDev/Maid-14
Length of output: 1119
🏁 Script executed:
# Check the specific file content around the flagged lines
cat -n Resources/Prototypes/Entities/Structures/Dispensers/booze.yml | sed -n '150,170p'Repository: TheMaidDev/Maid-14
Length of output: 547
🏁 Script executed:
cat -n Content.Shared/Charges/Components/LimitedChargesComponent.csRepository: TheMaidDev/Maid-14
Length of output: 2321
🏁 Script executed:
cat -n Content.Shared/Charges/Systems/SharedChargesSystem.csRepository: TheMaidDev/Maid-14
Length of output: 11590
lastCharges: 0 does not work due to MapInit override logic.
The OnChargesMapInit event in SharedChargesSystem.cs (lines 90-105) treats LastCharges == 0 as "uninitialized" and resets it to MaxCharges. This means setting lastCharges: 0 in the YAML will have no effect—BoozeDispenserEmpty will spawn with full charges instead of empty.
To spawn empty, use lastCharges: -1 instead (which is clamped to 0 on MapInit).
🤖 Prompt for AI Agents
In `@Resources/Prototypes/Entities/Structures/Dispensers/booze.yml` around lines
159 - 166, The YAML uses lastCharges: 0 which is treated as "uninitialized" by
the OnChargesMapInit logic in SharedChargesSystem.cs (it treats LastCharges == 0
as unset and resets to MaxCharges), so change the YAML entry for the Booze
dispenser (the LimitedCharges block with lastCharges) to use lastCharges: -1 to
produce an empty spawn (the map-init will clamp -1 to 0); reference
OnChargesMapInit / LastCharges / MaxCharges when locating the logic if you
instead prefer to adjust code behavior or add a clarifying comment.
| - type: LimitedCharges | ||
| lastCharges: 1000 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find the LimitedChargesComponent definition
ast-grep --pattern $'public sealed partial class LimitedChargesComponent : Component {
$$$
}'Repository: TheMaidDev/Maid-14
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Also search for LimitedChargesComponent more broadly
rg -A 30 "class LimitedChargesComponent" --type csharpRepository: TheMaidDev/Maid-14
Length of output: 1653
🏁 Script executed:
#!/bin/bash
# Read the YAML file to understand the context around lines 137-138 and line 69
cat -n Resources/Prototypes/Entities/Structures/Dispensers/chem.yml | sed -n '60,150p'Repository: TheMaidDev/Maid-14
Length of output: 2518
The lastCharges override may not work on either variant—investigate why the field isn't being applied despite being properly decorated with [DataField].
The field is correctly marked with [DataField, AutoNetworkedField] in LimitedChargesComponent, so YAML override should be supported. However, the comment on Line 69 states lastCharges: 0 # Doesn't work, indicating the override isn't being applied on ChemDispenserEmpty. Since both ChemDispenser (Filled) and ChemDispenserEmpty use the same component with identical field decoration, the same issue affects the override on Line 138. The problem likely lies in how LastCharges is initialized or used elsewhere in the code, not in the field declaration itself.
| components: | ||
| ## Maid START PR №21 | ||
| - type: LimitedCharges | ||
| lastCharges: 0 # Doesn't work, search for comment in PR 21 for more info | ||
| # Maid END |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "lastCharges|LastCharges|_charges" --type cs -C3 -g '!obj/**' | head -100Repository: TheMaidDev/Maid-14
Length of output: 11011
🏁 Script executed:
# Search for PR 21 references or related comments in YAML files
rg -n "PR 21|PR №21" --type yamlRepository: TheMaidDev/Maid-14
Length of output: 1579
🏁 Script executed:
# Find LimitedCharges component definition
fd -e cs | xargs grep -l "class LimitedCharges\|struct LimitedCharges" 2>/dev/null | head -5Repository: TheMaidDev/Maid-14
Length of output: 122
🏁 Script executed:
cat -n Content.Shared/Charges/Components/LimitedChargesComponent.csRepository: TheMaidDev/Maid-14
Length of output: 2321
🏁 Script executed:
# Also search for SharedChargesSystem to understand deserialization behavior
rg -n "class SharedChargesSystem\|lastCharges" --type cs Content.Shared/Charges/ -A 5 -B 2Repository: TheMaidDev/Maid-14
Length of output: 44
🏁 Script executed:
# Find SharedChargesSystem
fd -e cs -name "*ChargesSystem*" | head -10Repository: TheMaidDev/Maid-14
Length of output: 292
🏁 Script executed:
# Search for component initialization or registration patterns in the charges system
rg -n "public.*SharedChargesSystem|LastCharges|OnComponentInit" --type cs -g '*Charges*' -A 3 -B 1 | head -80Repository: TheMaidDev/Maid-14
Length of output: 5727
🏁 Script executed:
# Look at the full SharedChargesSystem initialization logic
cat -n Content.Shared/Charges/Systems/SharedChargesSystem.cs | sed -n '85,110p'Repository: TheMaidDev/Maid-14
Length of output: 1032
🏁 Script executed:
# Check what MaxCharges value is set for the Empty variants in soda.yml
cat -n Resources/Prototypes/Entities/Structures/Dispensers/soda.yml | sed -n '160,175p'Repository: TheMaidDev/Maid-14
Length of output: 352
🏁 Script executed:
# Check SodaDispenser parent configuration for MaxCharges
cat -n Resources/Prototypes/Entities/Structures/Dispensers/soda.yml | sed -n '100,135p'Repository: TheMaidDev/Maid-14
Length of output: 1286
🏁 Script executed:
# Also check chem.yml to see if it has the same issue
cat -n Resources/Prototypes/Entities/Structures/Dispensers/chem.yml | sed -n '55,75p'Repository: TheMaidDev/Maid-14
Length of output: 800
🏁 Script executed:
# Check the parent entity for ChemDispenserEmpty to see if it has MaxCharges override
cat -n Resources/Prototypes/Entities/Structures/Dispensers/chem.yml | sed -n '65,80p'Repository: TheMaidDev/Maid-14
Length of output: 621
🏁 Script executed:
# Check booze.yml for similar pattern to confirm this affects all dispensers
cat -n Resources/Prototypes/Entities/Structures/Dispensers/booze.yml | sed -n '110,170p'Repository: TheMaidDev/Maid-14
Length of output: 1855
lastCharges: 0 cannot work due to MapInit logic treating 0 as a sentinel value.
In SharedChargesSystem.OnChargesMapInit (lines 93-96), the code explicitly checks if (ent.Comp.LastCharges == 0) and replaces it with MaxCharges. This means setting lastCharges: 0 in the YAML does nothing—the field is overwritten to 1000 during initialization, defeating the purpose of the Empty variant.
To actually start with 0 charges, use lastCharges: -1 instead (which the system correctly clamps to 0 per lines 98-100). Alternatively, remove this non-functional field and address the underlying API issue: 0 should not be reserved as a sentinel value.
This pattern repeats across chem.yml, booze.yml, and soda.yml with identical comments.
🤖 Prompt for AI Agents
In `@Resources/Prototypes/Entities/Structures/Dispensers/soda.yml` around lines
164 - 168, The YAML sets lastCharges: 0 which is ineffective because
SharedChargesSystem.OnChargesMapInit treats 0 as a sentinel and replaces
ent.Comp.LastCharges with MaxCharges; update the Dispensers entries (soda.yml,
and the similar chem.yml and booze.yml instances) to use lastCharges: -1 if you
want an empty start (the system clamps -1 to 0), or remove the lastCharges field
entirely; reference SharedChargesSystem.OnChargesMapInit, ent.Comp.LastCharges,
and MaxCharges when making the change so you alter the correct YAML blocks.
|
Все SomeOldMethod()
{
//...
//Maid edit start
SomeNewLogic(blah blah blah);
if(something != null)
return;
//Maid edit end
//...
AnotherNewLogic(blah blah blah);//Maid edit
//...
} |
| // Maid START PR №21 | ||
| /* | ||
| _window.OnDispenseReagentButtonPressed += (location) => SendMessage(new ReagentDispenserDispenseReagentMessage(location)); | ||
| _window.OnEjectJugButtonPressed += (location) => SendMessage(new ReagentDispenserEjectContainerMessage(location)); | ||
| */ | ||
| _window.OnDispenseReagentButtonPressed += (reagent) => SendMessage(new ReagentDispenserDispenseReagentMessage(reagent)); | ||
| // Maid END |
There was a problem hiding this comment.
Старый код можно удалять если новый окружен //Maid edit
| if (hasCharges && dispensed > float.Epsilon) | ||
| { | ||
| return; | ||
| _chargesSystem.AddCharges(reagentDispenser.Owner, -((int) (MathF.Ceiling((float) dispensed * singleCost) + float.Epsilon))); // Ceil dispensed amount up (should be safe) |
There was a problem hiding this comment.
Почему -((int) (MathF.Ceiling((float) dispensed * singleCost) + float.Epsilon)), распиши зачем ты прибавляешь float.Epsilon после MathF.Ceiling()
RedFoxIV
left a comment
RedFoxIV
left a comment
There was a problem hiding this comment.
дальше платинового foreach (var item in storageFillComp.Contents) не смотрел
по-хорошему, тебе надо создать новую ветку и начать с нуля; то, что у тебя есть сейчас - уже не спасти.
Content.Server/Chemistry/EntitySystems/ReagentDispenserSystem.cs
Outdated
Show resolved
Hide resolved
Content.Server/Chemistry/EntitySystems/ReagentDispenserSystem.cs
Outdated
Show resolved
Hide resolved
Content.Server/Chemistry/EntitySystems/ReagentDispenserSystem.cs
Outdated
Show resolved
Hide resolved
Content.Server/Chemistry/EntitySystems/ReagentDispenserSystem.cs
Outdated
Show resolved
Hide resolved
|
Updated |
|
Ожидаем зарядку при подключении к сети, а ещё "очень интересные" комментарии всё ещё на месте. |
4 participants
About the PR
Откат раздатчика химикатов (до этого обновления)
Changelog
🆑 CREAsTIVE
Content.Server/Chemistry/EntitySystems/ReagentDispenserSystem.cs L137)LimitedCharges)Новый UI:
Summary by CodeRabbit
Refactor
New Features