Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert Newtonian Singularity #1865

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 7 additions & 25 deletions Content.Server/Singularity/Components/GravityWellComponent.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,40 +7,36 @@ namespace Content.Server.Singularity.Components;
/// The server-side version of <see cref="SharedGravityWellComponent"/>.
/// Primarily managed by <see cref="GravityWellSystem"/>.
/// </summary>
[RegisterComponent]
[RegisterComponent, AutoGenerateComponentPause]
public sealed partial class GravityWellComponent : Component
{
/// <summary>
/// The maximum range at which the gravity well can push/pull entities.
/// </summary>
[DataField("maxRange")]
[ViewVariables(VVAccess.ReadWrite)]
[DataField]
public float MaxRange;
Comment on lines +16 to 17
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 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

Comment on lines +16 to 17
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 3, 2025

Choose a reason for hiding this comment

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

🧹 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

Copy link
Member Author

Choose a reason for hiding this comment

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

Our current code regulations do not permit explicitly defining DataField names, make sure to update your regulations book Oficer.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.


/// <summary>
/// The minimum range at which the gravity well can push/pull entities.
/// This is effectively hardfloored at <see cref="GravityWellSystem.MinGravPulseRange"/>.
/// </summary>
[DataField("minRange")]
[ViewVariables(VVAccess.ReadWrite)]
[DataField]
public float MinRange = 0f;

/// <summary>
/// The acceleration entities will experience towards the gravity well at a distance of 1m.
/// Negative values accelerate entities away from the gravity well.
/// Actual acceleration scales with the inverse of the distance to the singularity.
/// </summary>
[DataField("baseRadialAcceleration")]
[ViewVariables(VVAccess.ReadWrite)]
[DataField]
public float BaseRadialAcceleration = 0.0f;

/// <summary>
/// The acceleration entities will experience tangent to the gravity well at a distance of 1m.
/// Positive tangential acceleration is counter-clockwise.
/// Actual acceleration scales with the inverse of the distance to the singularity.
/// </summary>
[DataField("baseTangentialAcceleration")]
[ViewVariables(VVAccess.ReadWrite)]
[DataField]
public float BaseTangentialAcceleration = 0.0f;

#region Update Timing
Expand All @@ -56,28 +52,14 @@ public sealed partial class GravityWellComponent : Component
/// <summary>
/// The next time at which this gravity well should pulse.
/// </summary>
[ViewVariables(VVAccess.ReadOnly)]
[Access(typeof(GravityWellSystem))]
[DataField, Access(typeof(GravityWellSystem)), AutoPausedField]
public TimeSpan NextPulseTime { get; internal set; } = default!;

/// <summary>
/// The last time this gravity well pulsed.
/// </summary>
[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;
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

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!

Comment on lines 61 to +62
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 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.

Suggested change
[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;


#endregion Update Timing
}
18 changes: 3 additions & 15 deletions Content.Server/Singularity/EntitySystems/EventHorizonSystem.cs
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,11 @@ public sealed class EventHorizonSystem : SharedEventHorizonSystem
[Dependency] private readonly SharedContainerSystem _containerSystem = default!;
[Dependency] private readonly SharedPhysicsSystem _physics = default!;
[Dependency] private readonly SharedTransformSystem _xformSystem = default!;
[Dependency] private readonly SharedMapSystem _mapSystem = default!;
[Dependency] private readonly TagSystem _tagSystem = default!;
#endregion Dependencies

private EntityQuery<PhysicsComponent> _physicsQuery;
private const float CosmicSpeedLimit = 20f;

public override void Initialize()
{
Expand Down Expand Up @@ -137,18 +137,6 @@ public void ConsumeEntity(EntityUid hungry, EntityUid morsel, EventHorizonCompon
}

EntityManager.QueueDeleteEntity(morsel);

if (eventHorizon.InheritMomentum && TryComp<PhysicsComponent>(hungry, out var thisPhysics) && TryComp<PhysicsComponent>(morsel, out var otherPhysics))
{
var impulse = (otherPhysics.LinearVelocity - thisPhysics.LinearVelocity)
* otherPhysics.FixturesMass
* thisPhysics.FixturesMass / (thisPhysics.FixturesMass + otherPhysics.FixturesMass); // Accounts for the expected mass change from consuming the object
if (impulse.Length() >= CosmicSpeedLimit) // Stop it from quantum tunneling through walls.
impulse = impulse.Normalized() * CosmicSpeedLimit;

_physics.ApplyLinearImpulse(hungry, impulse, body: thisPhysics);
}

var evSelf = new EntityConsumedByEventHorizonEvent(morsel, hungry, eventHorizon, outerContainer);
var evEaten = new EventHorizonConsumedEntityEvent(morsel, hungry, eventHorizon, outerContainer);
RaiseLocalEvent(hungry, ref evSelf);
Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

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.

}

/// <summary>
Expand Down Expand Up @@ -323,7 +311,7 @@ public void ConsumeTilesInRange(EntityUid uid, float range, TransformComponent?
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);
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

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!

}
}

Expand Down
Loading
Loading