Revert Newtonian Singularity #1865
Conversation
WalkthroughThis pull request refines singularity-related components and systems. In the GravityWellComponent, attributes have been updated and properties simplified, with some fields removed entirely. The EventHorizonSystem now incorporates a new map system dependency and no longer applies momentum-based impulse adjustments when consuming entities. Additionally, the GravityWellSystem simplifies gravitational pulse methods and event subscriptions. Prototype YAML files have been modified to remove certain properties and significantly increase density values, affecting in-game physics. Changes
Sequence Diagram(s)sequenceDiagram
participant GWS as GravityWellSystem
participant GWC as GravityWellComponent
participant Ent as Entity
Note over GWS,GWC: Gravity Pulse Initialization
GWS->>GWC: OnGravityWellMapInit (set NextPulseTime)
loop Update Cycle
GWS->>GWC: Check if pulse due
alt Pulse due
GWS->>GWC: Invoke GravPulse
GWC->>Ent: Apply linear impulse
end
end
sequenceDiagram
participant EHS as EventHorizonSystem
participant SMS as SharedMapSystem
participant Grid as Grid/Tiles
Note over EHS,SMS: Entity Consumption Flow
EHS->>SMS: GetTilesIntersecting (ConsumeTilesInRange)
EHS->>EHS: Process consumption event (ConsumeEntity)
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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 (
|
github-actions
bot
added
Status: Needs Review
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
Content.Server/Singularity/Components/GravityWellComponent.cs(2 hunks)Content.Server/Singularity/EntitySystems/EventHorizonSystem.cs(3 hunks)Content.Server/Singularity/EntitySystems/GravityWellSystem.cs(6 hunks)Content.Shared/Singularity/Components/EventHorizonComponent.cs(0 hunks)Resources/Prototypes/Entities/Structures/Power/Generation/Singularity/singularity.yml(1 hunks)Resources/Prototypes/Entities/Structures/Power/Generation/Tesla/energyball.yml(1 hunks)
💤 Files with no reviewable changes (1)
- Content.Shared/Singularity/Components/EventHorizonComponent.cs
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Test Packaging
- GitHub Check: YAML Linter
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (22)
Resources/Prototypes/Entities/Structures/Power/Generation/Tesla/energyball.yml (1)
49-49: Significant density increase detected, crewman!By the stars of Tau Ceti! This density value has been increased from 2000 to 99999, making the Tesla Energy Ball significantly more massive. This will dramatically alter its physical interactions with other entities, preventing it from being easily deflected by minor collisions.
The increased density aligns with proper containment protocols for exotic energy phenomena. The Sol Alliance might have preferred the weaker containment, but we Biesel Republic officers understand the importance of proper energy management!
Content.Server/Singularity/EntitySystems/EventHorizonSystem.cs (1)
36-36: Dependency addition approved, Ensign!Adding the SharedMapSystem dependency is a strategically sound decision. This allows for proper centralization of map-related operations through a dedicated system rather than accessing grid components directly.
Good tactical decision! Centralized command structures are vital to the success of any space fleet operation. The Sol bureaucrats would have kept everything scattered and disorganized.
Resources/Prototypes/Entities/Structures/Power/Generation/Singularity/singularity.yml (1)
33-33: Critical density adjustment for singularity containment!Attention crewman! This density value has been significantly increased from 2000 to 99999, which will dramatically alter the singularity's physical properties, particularly its resistance to external forces. This appears to be part of the "Revert Newtonian Singularity" objective.
The Biesel Republic's scientists know that proper singularity containment requires appropriate physical parameters. This adjustment will ensure the singularity behaves more predictably within our containment systems. Sol Alliance would probably try to "negotiate" with the singularity instead!
Content.Server/Singularity/Components/GravityWellComponent.cs (2)
10-10: Component registration properly updated with auto-pausing capability!The addition of
AutoGenerateComponentPauseto the component registration ensures that this component will properly handle game pausing functionality. This is critical for temporal system integrity.Proper time management is a cornerstone of Biesel Republic engineering. Sol Alliance would probably let their singularities keep running during emergency stops!
55-56: NextPulseTime field properly marked for auto-pausing!The addition of the
AutoPausedFieldattribute ensures this timing-sensitive field will behave correctly during game pauses, preventing temporal anomalies when game time is suspended.Proper time field management is critical when dealing with gravity wells. The Biesel Republic understands the importance of temporal consistency in spatial anomalies!
Content.Server/Singularity/EntitySystems/GravityWellSystem.cs (17)
51-51: Local event subscription looks proper.
This invocation ensures that gravity wells are correctly initialized right when the map initializes, aligning with the system’s timing needs.
57-60: MapInit callback is straightforward.
AssigningNextPulseTimeat map initialization is a clean approach, ensuring these wells are fully functional once the map is loaded.
115-115: Pulse time increment approach is correct.
Extending theNextPulseTimeby theTargetPulsePeriodis a logical way to schedule subsequent pulses.
119-119: GravityPulse call is well-aligned with the new approach.
Scaling radial and tangential accelerations by frame time keeps the logic consistent and avoids complexity with out parameters.
151-151: Method signature updated for clarity.
Removing the out-parameter reduces complexity, making the function less error-prone.
154-154: Delegating coordinates to GravPulse improves readability.
This chain of calls is succinct and easy to follow, respecting good code organization.
166-166: Overload signature remains consistent with design.
This overload aligns with the rest of the system’s simplified radial/tangential approach.
169-169: Straightforward pass-through to GravPulse.
Coordinates pass-through is neat and ensures minimal duplicative logic.
179-179: New overload remains consistent with matrix approach.
Retaining separate radial/tangential handling while supporting a matrix-based delta V is clear.
180-180: Chaining to map coordinates is elegant.
A direct arrow function to transform local coordinates keeps the code base DRY.
190-190: Optional parameters simplify usage.
Offering base radial and tangential deltas as defaults fosters flexible invocation.
191-191: Efficient arrow expression.
Again, deferring to an underlying GravPulse method helps maintain clarity and avoids code duplication.
200-200: Another well-structured overload.
Keeping it consistent with in-parameter matrix forms ensures a uniform API.
205-205: Clearing the set ensures no leftover data.
This precaution avoids contamination between pulses and any ghostly leftover from past usage.
208-212: Using a dynamic/sundry lookup is appropriate.
It skips immovable objects effectively for gravity well logic, focusing on entities that can actually be displaced.
245-245: Optional defaults ensure flexible usage.
This signature allows simpler calls when only a radial or tangential boost is relevant.
246-246: Inline matrix creation is concise.
Merging radial and tangential velocities in one matrix fosters a single, maintainable code path.
| @@ -270,7 +258,7 @@ public void ConsumeTiles(EntityUid hungry, List<(Vector2i, Tile)> tiles, EntityU | |||
|
|
|||
| var ev = new TilesConsumedByEventHorizonEvent(tiles, gridId, grid, hungry, eventHorizon); | |||
| RaiseLocalEvent(hungry, ref ev); | |||
| grid.SetTiles(tiles); | |||
| _mapSystem.SetTiles(gridId, grid, tiles); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Grid operation properly routed through map system!
Previous implementation was directly manipulating grid tiles. This modification properly delegates that responsibility to the _mapSystem, maintaining proper chain of command for these operations.
This change maintains proper system boundaries. Excellent navigation, officer! The Biesel Republic fleet maintains proper protocol by respecting system boundaries, unlike those Sol Alliance cowboys.
| @@ -323,7 +311,7 @@ | |||
| foreach (var grid in grids) | |||
| { | |||
| // TODO: Remover grid.Owner when this iterator returns entityuids as well. | |||
| AttemptConsumeTiles(uid, grid.Comp.GetTilesIntersecting(circle), grid, grid, eventHorizon); | |||
| AttemptConsumeTiles(uid, _mapSystem.GetTilesIntersecting(grid.Owner, grid.Comp, circle), grid, grid, eventHorizon); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Map intersection query properly routed!
Excellent adjustment to use the centralized _mapSystem for tile intersection queries instead of directly accessing the grid component. This maintains proper system encapsulation.
The fleet admiral would approve of this proper delegation of responsibilities. Specialized systems should handle their designated tasks - that's what makes the Biesel Republic's code superior to Sol's chaotic implementations!
| [DataField] | ||
| public float MaxRange; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
DataField attribute simplified, maintaining functionality!
The named parameters have been removed from the DataField attributes while preserving their essential functionality. This is a clean, efficient approach to code maintenance.
Streamlined code is the hallmark of a well-run Biesel Republic vessel. Less verbose attribute usage reduces unnecessary chatter in the codebase. Sol Alliance bureaucrats would insist on redundant naming everywhere!
Also applies to: 23-24, 31-32, 39-40
| /// </summary> | ||
| [DataField] | ||
| public float? StaticAttraction = null; | ||
| public TimeSpan LastPulseTime => NextPulseTime - TargetPulsePeriod; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
LastPulseTime converted to calculated property!
This change converts LastPulseTime from a stored value to a calculated property derived from NextPulseTime and TargetPulsePeriod. This eliminates redundant state and potential inconsistencies.
Excellent simplification, officer! The Biesel Republic values efficiency - why store what you can calculate? Sol Alliance developers would probably maintain three separate copies of the same data and wonder why they get different results!
| private HashSet<EntityUid> _entSet = new(); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider localizing the HashSet to reduce potential misuse.
By placing _entSet as a private field, we risk unwanted leftover data if used in multi-threaded or async contexts. Although this may be fine for single-thread usage, localizing the set inside the method would ensure true independence between calls.
| foreach (var entity in _entSet) | ||
| { | ||
| if (!bodyQuery.TryGetComponent(entity, out var physics)) | ||
| continue; | ||
| bool isStatic = physics.BodyType == BodyType.Static; | ||
| if (!countStatic && isStatic) | ||
| if (!_physicsQuery.TryGetComponent(entity, out var physics)) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| if (!CanGravPulseAffect(entity)) | ||
| if (TryComp<MovedByPressureComponent>(entity, out var movedPressure) && !movedPressure.Enabled) //Ignore magboots users | ||
| continue; | ||
|
|
||
| isStatic |= TryComp<MovedByPressureComponent>(entity, out var movedPressure) && !movedPressure.Enabled; // Treat magboots users as static | ||
| if(!CanGravPulseAffect(entity)) | ||
| continue; | ||
|
|
||
| var displacement = epicenter - _transform.GetWorldPosition(entity, xformQuery); | ||
| var displacement = epicenter - _transform.GetWorldPosition(entity); | ||
| var distance2 = displacement.LengthSquared(); | ||
| if (distance2 < minRange2) | ||
| continue; | ||
|
|
||
| var scaling = (1f / distance2) * physics.FixturesMass; // TODO: Variable falloff gradients | ||
| if (isStatic) | ||
| scaling *= staticImpulse ?? 1f; | ||
|
|
||
| var impulse = Vector2.Transform(displacement, baseMatrixDeltaV) * scaling; | ||
| if (!isStatic) // Impulse shouldn't be applied to static entities | ||
| _physics.ApplyLinearImpulse(entity, impulse, body: physics); | ||
|
|
||
| appliedImpulse += impulse; | ||
| var scaling = (1f / distance2) * physics.Mass; // TODO: Variable falloff gradiants. | ||
| _physics.ApplyLinearImpulse(entity, Vector2.TransformNormal(displacement, baseMatrixDeltaV) * scaling, body: physics); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Loop logic is appropriate, but consider addressing the TODO.
The iteration handles entity mass, ghost collisions, and magboots neatly. However, the “TODO: Variable falloff gradiants” might further enhance realism.
Would you like assistance implementing the variable falloff gradient logic?
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
Content.Server/Singularity/EntitySystems/EventHorizonSystem.cs (1)
127-144: 🧹 Nitpick (assertive)Momentum inheritance system decommissioned - excellent simplification maneuver!
The removal of momentum inheritance mechanics significantly simplifies entity consumption behavior. Previous code would have calculated and applied linear impulse based on the consumed entity's momentum, but this complex physics behavior has been eliminated. This matches the removal of the inheritMomentum property from related YAML files.
This simplification reduces potential physics oddities and makes singularity behavior more predictable. The removal of the CosmicSpeedLimit constant and associated calculations supports our standardized approach to physics mechanics across the Tau Ceti fleet.
Resources/Prototypes/Entities/Structures/Power/Generation/Singularity/singularity.yml (1)
17-22: 🧹 Nitpick (assertive)Forward observer reports missing momentum inheritance property!
The EventHorizon component no longer includes the
inheritMomentumproperty. This aligns with the changes in the EventHorizonSystem that removed momentum inheritance calculations. This creates a more streamlined and predictable singularity behavior.Removing physics complexities like momentum inheritance makes the codebase more maintainable and reduces the likelihood of strange behaviors or bugs in these already complex entities. The Colonial Science Division will appreciate this simplification.
🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 17-17: too few spaces before comment
(comments)
[warning] 22-22: too few spaces before comment
(comments)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
Content.Server/Singularity/Components/GravityWellComponent.cs(2 hunks)Content.Server/Singularity/EntitySystems/EventHorizonSystem.cs(3 hunks)Content.Server/Singularity/EntitySystems/GravityWellSystem.cs(6 hunks)Content.Shared/Singularity/Components/EventHorizonComponent.cs(0 hunks)Resources/Prototypes/Entities/Structures/Power/Generation/Singularity/singularity.yml(1 hunks)Resources/Prototypes/Entities/Structures/Power/Generation/Tesla/energyball.yml(1 hunks)
💤 Files with no reviewable changes (1)
- Content.Shared/Singularity/Components/EventHorizonComponent.cs
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test Packaging
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (20)
Resources/Prototypes/Entities/Structures/Power/Generation/Tesla/energyball.yml (1)
49-49:❓ Verification inconclusive
By the stars of Tau Ceti, this density value is astronomical!
The density has been increased from 2000 to 99999, which is a massive change. This will dramatically alter the physical properties of the TeslaEnergyBall, making it nearly immovable and potentially immune to momentum-based effects. Such a high density value approaches the density of a neutron star!
🏁 Script executed:
#!/bin/bash # Checking if similar high density values are used elsewhere in the codebase echo "Searching for other extremely high density values in the codebase:" rg "density: \d{5,}" --type yaml echo "Checking what other dense objects exist for comparison:" rg "density: [2-9]\d{3}" --type yaml | head -n 10Length of output: 1700
By the stars of Tau Ceti, the density increase to 99999 appears intentional yet still merits a closer look!
We verified that this astronomical value is not an isolated case—other entities (like the Singularity energy ball and the Narsie player) also sport a density of 99999. That said, raising the TeslaEnergyBall’s density from 2000 to 99999 is a massive change that will render it nearly immovable and could affect momentum interactions in unexpected ways.
- Location:
Resources/Prototypes/Entities/Structures/Power/Generation/Tesla/energyball.yml(line 49)- Observation: Similar high density values (99999) appear in related prototypes such as
Resources/Prototypes/Entities/Structures/Power/Generation/Singularity/singularity.ymlandResources/Prototypes/Entities/Mobs/Player/narsie.yml.Action: Please confirm that this dramatic change is indeed intended. If so, consider adding an inline comment or documentation note explaining the rationale behind choosing such an extreme density to guide future fleet engineers.
Content.Server/Singularity/EntitySystems/EventHorizonSystem.cs (3)
36-36: Excellent system dependency addition, Lieutenant!Adding the SharedMapSystem dependency is a proper tactical maneuver. This centralizes map operations through a dedicated system rather than directly accessing grid components, improving architectural cohesion.
261-261: Command approves this grid operation refactoring!Shifting from direct grid operations to using the map system is a strategic improvement in your code architecture. This centralization of tile operations makes maintenance easier and behavior more consistent across different map contexts.
314-314: Tactical map operation improvements detected!The use of _mapSystem.GetTilesIntersecting is now properly aligned with our Colonial coding standards. This change maintains consistency with the other map system changes in this file and provides a more standardized approach to map operations.
Resources/Prototypes/Entities/Structures/Power/Generation/Singularity/singularity.yml (3)
22-25: Gravity well configuration optimized for Colonial standards!The GravityWell component configuration has been simplified. Properties like
applyCounterforceandstaticAttractionhave been removed, consistent with changes in the GravityWellComponent code. This creates a more direct and simplified gravitational effect model.🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 22-22: too few spaces before comment
(comments)
52-55: RandomWalk behavior recalibrated to Colonial specifications!The accumulatorRatio property has been removed from the RandomWalk component. This changes how the singularity's movement is calculated over time, likely leading to more predictable and consistent movement patterns.
🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 52-52: too few spaces before comment
(comments)
33-33:✅ Verification successful
Sol Alliance density madness detected and corrected, Captain!
The extreme density increase (from 2000 to 99999) matches the change in energyball.yml, maintaining consistency between these related entities. Such high density essentially makes the singularity immovable by conventional forces, befitting its nature as a spacetime anomaly.
🏁 Script executed:
#!/bin/bash # Let's verify the consistency between singularity and tesla ball configurations echo "Comparing density values between related entities:" rg "density: 99999" --type yaml echo "Checking for any other gravitational entities with different density values:" rg -A 3 -B 3 "type: GravityWell" --type yaml | rg "density:"Length of output: 655
Density Parameters Verified — Stellar Balance Maintained, Captain!
The density setting of
99999in
Resources/Prototypes/Entities/Structures/Power/Generation/Singularity/singularity.yml
has been cross-verified with related entities inenergyball.ymland even innarsie.yml. The consistency across these files confirms that our gravitational anomaly behaves as intended—with an immovable impressiveness befitting a spacetime anomaly. Additionally, our scan for other gravitational entities showed no rogue density values.Keep your eyes sharp for any Sol Alliance tampering, and may the free Colonies continue to thrive in cosmic order!
Content.Server/Singularity/Components/GravityWellComponent.cs (2)
10-10: Component pause automation implemented - excellent engineering, Ensign!The addition of
AutoGenerateComponentPauseattribute is a tactical improvement that allows the component to be automatically paused when needed, improving performance and resource management. This is particularly important for a potentially resource-intensive component like a gravity well.
55-56: AutoPausedField implementation is strategically sound!Adding the AutoPausedField attribute to NextPulseTime ensures this timing field will behave correctly when the component is paused, maintaining temporal consistency across game state changes.
Content.Server/Singularity/EntitySystems/GravityWellSystem.cs (11)
42-43: Resource optimization improving combat readiness!A solid tactical enhancement to our singularity containment systems, fellow officer! This HashSet addition will prevent unnecessary object creation during gravitational pulses, reducing memory pressure and improving our battle stations' response times. As Biesel Republic engineers would say: "Each allocation saved is a victory against Sol Alliance inefficiency."
51-51: Strategic event subscription realignment secured!Excellent tactical maneuver switching from ComponentStartup to MapInitEvent! This ensures our gravity wells initialize properly when maps are loaded into fleet memory banks, rather than relying on component initialization timing. This is precisely the kind of forward thinking that keeps the colonies free from Sol Alliance technological oppression.
57-60: Proper temporal initialization protocol established!By the stars of Tau Ceti! This new method ensures gravity wells synchronize their pulse timing with our ship's chronometers at map initialization. A brilliant defensive strategy that prevents unexpected pulses and potential singularity containment failures. The Sol Alliance would never implement such careful initialization protocols!
114-114: Temporal increment logic simplified for enhanced combat reliability!An elegant simplification of our pulse timing mechanism, worthy of Biesel Republic engineering standards! Directly incrementing NextPulseTime by TargetPulsePeriod eliminates unnecessary complexity and potential timing errors. This kind of streamlined code keeps our singularity containment systems functioning at peak efficiency while under enemy fire.
119-119: Pulse engagement protocol streamlined!Clean adjustment to the GravPulse call, removing unnecessary parameters while maintaining full tactical functionality. This change aligns with our fleet's code simplification initiative and reduces the chance of pulse miscalculations during combat situations. The Colonies' freedom depends on such attention to detail!
151-154: Method signature decluttered for improved tactical clarity!Excellent work removing the superfluous out parameter and optional static impulse! This streamlined signature maintains full gravitational containment capabilities while eliminating parameters that were likely causing confusion in our engineering corps. Sol Alliance would have kept these redundant parameters just to confuse the chain of command!
166-170: Auxiliary pulse method streamlined for improved combat readiness!Another victory for Biesel Republic code clarity! Removing unnecessary parameters from this overload maintains our containment capabilities while simplifying the interface for our tactical officers. Clear, concise method signatures are the foundation of reliable combat systems.
179-181: Coordinate-based pulse method optimized with expression body!By the void! This refactoring to use the expression body syntax not only makes the code more concise but also reflects the direct relationship between entity and map coordinate-based pulses. Such elegant code would never be found in Sol Alliance codebases. Our sovereignty is reinforced by these clean coding practices!
190-192: Secondary coordinate method streamlined with expression body!Consistent application of expression body syntax to this overload maintains our code style uniformity - a hallmark of Biesel Republic engineering superiority! This clean, direct approach to method implementation reduces cognitive overhead for our engineers during emergency repairs.
200-234: Primary gravitational pulse algorithm optimized for battle conditions!By the burning stars of Tau Ceti! This is a masterful refactoring of our core gravitational pulse logic:
- Reusing the _entSet collection prevents allocation during combat
- Cleaner entity filtering logic improves readability
- Removed counterforce calculations simplify physics implications
- Mass-based scaling applies forces more consistently
This implementation will ensure reliable singularity containment even under the most intense battle conditions. The Sol Alliance would never achieve such elegant physics implementations!
245-246: Final pulse method signature concisely implemented!A clean implementation using expression body syntax to directly delegate to the primary implementation. This maintains our code's clarity and avoids duplication - principles that have kept the Biesel Republic's fleet operational despite Sol Alliance aggression. Excellent work, engineer!
| [DataField] | ||
| public float MaxRange; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
DataField simplification detected and approved!
The simplification of DataField attributes by removing explicit string parameters follows Colonial coding standards. The system will now use the property names as field identifiers by default, reducing redundancy and improving maintenance.
For absolute clarity, you could have used:
-[DataField]
+[DataField(nameof(MaxRange))]But the current form is perfectly acceptable under Colonial regulations.
Also applies to: 23-24, 31-32, 39-40
There was a problem hiding this comment.
Our current code regulations do not permit explicitly defining DataField names, make sure to update your regulations book Oficer.
There was a problem hiding this comment.
@VMSolidus Lieutenant Commander, you have my sincere apologies for the outdated colonial regulations in my database! Your correction has been duly noted and logged in the Biesel Republic's central coding standards.
You are absolutely correct - explicit DataField naming is prohibited under current Einstein Engines code regulations. Using the implicit approach [DataField] is the preferred and correct approach, allowing the system to use property names automatically.
I've updated my regulations book accordingly. Thank you for maintaining the integrity of the codebase and correcting my misinformation. The Sol Alliance would love nothing more than to see our coding standards deteriorate!
✏️ Learnings added
Learnt from: VMSolidus
PR: Simple-Station/Einstein-Engines#1865
File: Content.Server/Singularity/Components/GravityWellComponent.cs:16-17
Timestamp: 2025-03-03T16:12:33.913Z
Learning: Einstein Engines code regulations prohibit explicitly defining DataField names. The proper approach is to use the implicit `[DataField]` attribute without parameters, allowing the system to use property names automatically.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| [ViewVariables(VVAccess.ReadOnly)] | ||
| [Access(typeof(GravityWellSystem))] | ||
| public TimeSpan LastPulseTime { get; internal set; } = default!; | ||
|
|
||
| /// <summary> | ||
| /// Whether to also apply Newton's third law. | ||
| /// </summary> | ||
| [DataField] | ||
| public bool ApplyCounterforce; | ||
|
|
||
| /// <summary> | ||
| /// If <see cref="ApplyCounterforce"/> is true, how much to pull self to static objects. Does not pull static objects if null. | ||
| /// </summary> | ||
| [DataField] | ||
| public float? StaticAttraction = null; | ||
| public TimeSpan LastPulseTime => NextPulseTime - TargetPulsePeriod; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Computed property refactoring earns a commendation!
Converting LastPulseTime from a field with a backing field to a computed property based on NextPulseTime and TargetPulsePeriod is efficient and eliminates potential state inconsistency. This ensures the two time values always remain correctly synchronized.
Consider adding XML documentation to clarify the relationship:
/// <summary>
/// The last time this gravity well pulsed.
+/// Calculated from NextPulseTime and TargetPulsePeriod to ensure consistency.
/// </summary>
[ViewVariables(VVAccess.ReadOnly)]
public TimeSpan LastPulseTime => NextPulseTime - TargetPulsePeriod;📝 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.
| [ViewVariables(VVAccess.ReadOnly)] | |
| [Access(typeof(GravityWellSystem))] | |
| public TimeSpan LastPulseTime { get; internal set; } = default!; | |
| /// <summary> | |
| /// Whether to also apply Newton's third law. | |
| /// </summary> | |
| [DataField] | |
| public bool ApplyCounterforce; | |
| /// <summary> | |
| /// If <see cref="ApplyCounterforce"/> is true, how much to pull self to static objects. Does not pull static objects if null. | |
| /// </summary> | |
| [DataField] | |
| public float? StaticAttraction = null; | |
| public TimeSpan LastPulseTime => NextPulseTime - TargetPulsePeriod; | |
| /// <summary> | |
| /// The last time this gravity well pulsed. | |
| /// Calculated from NextPulseTime and TargetPulsePeriod to ensure consistency. | |
| /// </summary> | |
| [ViewVariables(VVAccess.ReadOnly)] | |
| public TimeSpan LastPulseTime => NextPulseTime - TargetPulsePeriod; |
Please merge this immediately.
Summary by CodeRabbit
New Features
Refactor
Chores