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.

Sign up for GitHub

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

Remove warnings from ExplosionSystem.TileFill.cs #34088

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Remove warnings from ExplosionSystem.TileFill.cs #34088

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b5d08bb
Rework initial tile finding to be a separate function
BarryNorfolk Dec 27, 2024
decd9f2
Use referenceGrid and transform system as per obselete comments
BarryNorfolk Dec 27, 2024
88f7d12
Remove obsolete usage of FindGridsIntersecting
BarryNorfolk Dec 27, 2024
fcee41b
Fix obsolete usage of WorldToTile
BarryNorfolk Dec 27, 2024
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
51 changes: 40 additions & 11 deletions Content.Server/Explosion/EntitySystems/ExplosionSystem.TileFill.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,17 +47,18 @@ public sealed partial class ExplosionSystem
var (localGrids, referenceGrid, maxDistance) = GetLocalGrids(epicenter, totalIntensity, slope, maxIntensity);

// get the epicenter tile indices
if (_mapManager.TryFindGridAt(epicenter, out var gridUid, out var candidateGrid) &&
candidateGrid.TryGetTileRef(candidateGrid.WorldToTile(epicenter.Position), out var tileRef) &&
!tileRef.Tile.IsEmpty)
if (FindInitialTile(epicenter, out var foundTile, out var foundGrid))
{
epicentreGrid = gridUid;
initialTile = tileRef.GridIndices;
epicentreGrid = foundGrid;
initialTile = foundTile;
}
else if (referenceGrid != null)
{
// reference grid defines coordinate system that the explosion in space will use
initialTile = Comp<MapGridComponent>(referenceGrid.Value).WorldToTile(epicenter.Position);
initialTile = _map.WorldToTile(
referenceGrid.Value,
Comp<MapGridComponent>(referenceGrid.Value),
epicenter.Position);
}
else
{
Expand Down Expand Up @@ -88,9 +89,9 @@ public sealed partial class ExplosionSystem
var spaceAngle = Angle.Zero;
if (referenceGrid != null)
{
var xform = Transform(Comp<MapGridComponent>(referenceGrid.Value).Owner);
spaceMatrix = xform.WorldMatrix;
spaceAngle = xform.WorldRotation;
var xform = Transform(referenceGrid.Value);
spaceMatrix = _transformSystem.GetWorldMatrix(xform);
spaceAngle = _transformSystem.GetWorldRotation(xform);
}
Comment on lines +92 to 95
Copy link
Contributor

Choose a reason for hiding this comment

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

Use the overload that does both at the same time please, much faster (each one needs to go up the transform hierarchy individually instead of at the same time).


// is the explosion starting on a grid?
Expand Down Expand Up @@ -251,6 +252,33 @@ public sealed partial class ExplosionSystem
return (totalTiles, iterationIntensity, spaceData, gridData, spaceMatrix);
}

private bool FindInitialTile(MapCoordinates epicenter, out Vector2i initialTile, out EntityUid? epicentreGrid)
{
initialTile = new Vector2i();
epicentreGrid = null;

if (!_mapManager.TryFindGridAt(epicenter, out var gridUid, out var candidateGrid))
{
return false;
}

var tilePos = _map.WorldToTile(gridUid, candidateGrid, epicenter.Position);
if (!_map.TryGetTileRef(gridUid, candidateGrid, tilePos, out var tileRef))
{
return false;
}

if (tileRef.Tile.IsEmpty)
{
return false;
}

epicentreGrid = gridUid;
initialTile = tileRef.GridIndices;

return true;
}

Comment on lines 254 to +281
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like it should be an engine method on SharedMapSystem if anything?

Also doesn't TryFindGridAt fail if the tile is empty? is the return at 273 even possible?

/// <summary>
/// Look for grids in an area and returns them. Also selects a special grid that will be used to determine the
/// orientation of an explosion in space.
Expand All @@ -277,7 +305,9 @@ public sealed partial class ExplosionSystem
// diameter x diameter sized box, use a smaller box with radius sized sides:
var box = Box2.CenteredAround(epicenter.Position, new Vector2(radius, radius));

foreach (var grid in _mapManager.FindGridsIntersecting(epicenter.MapId, box))
var mapGrids = new List<Entity<MapGridComponent>>();
_mapManager.FindGridsIntersecting(epicenter.MapId, box, ref mapGrids);
foreach (var grid in mapGrids)
Comment on lines 307 to +310
Copy link
Contributor

Choose a reason for hiding this comment

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

This warning is around the fact you should be storing the list on the system and clearing + re-using it instead of allocating it every time.

{
if (TryComp(grid.Owner, out PhysicsComponent? physics) && physics.Mass > mass)
{
Expand All @@ -297,7 +327,6 @@ public sealed partial class ExplosionSystem

radius *= 4;
box = Box2.CenteredAround(epicenter.Position, new Vector2(radius, radius));
var mapGrids = _mapManager.FindGridsIntersecting(epicenter.MapId, box).ToList();
var grids = mapGrids.Select(x => x.Owner).ToList();

if (referenceGrid != null)
Expand Down
Loading