Introduce [MP]PosWithUVAlreadyKnownToBeInMapBounds
Nobody has claimed this yet.
- Dominant language
- C#
- Stars
- 17.4k
- Forks
- 3k
- Avg merge
- 1d 12h
- Merged PRs (30d)
- 14
Description
Introduction:
Both Map.Contains and CellLayer.Contains and ProjectedCellLayer.Contains are called often throughout the code.
For instance in a single tick the Shroud checks for each added cells to see if it falls within the bounds of the map/cell-layer before adding it to the layer. Next the Shroud notifies the ShroudRenderer which does the same, which in turn checks with the Shroud to see if a shroud cell is visible or explored - where again a check is done to validate that the cell falls inside the bounds of the map. This for all Shroud instances.
For each tick this means that for multiple cell layers for many if not all cells the Contains/bounds check is performed multiple times. This to ensure robust code. This makes sense. It is not certain that cells are within bounds.
Many components like Shroud and ShroudRenderer however do not allow cells that are outside the bounds of the map to be added to them in the first place. I.e Cell layers will always have the same dimension as the map. The many bounds checks could be avoided if validated the first time at creation.
This is beneficial also because Map.Contains is not for all cell types a very light weight method - it converts MPos to multiple PPos and after to CPos to check if all CPos-ses are in bounds.
Feature request:
In general make the [MPC]Pos values 'more readonly'. Perform bounds checks once during creation. After take advantage of not having to perform the bound check each use. Let code that uses the new types know for certain that the cells are inside the bounds of the map.
Possible implementation:
- Introduce
MapMPos,MapPPosandMapCPostypes that during construction validate that they fall inside the bounds of the map they belong to - by passing a map in the constructor. - Throw an exception in the constructor if the
UVvalue lie outside map bounds. The creator is responsible for checking before. - Allow conversion to their existing counterparts
MPosPPosandCPosetc. To i.e. allow calculations on them where result may fall outside bounds. - Implement methods like
Map.ProjectedCellsCoveringandShroud.IsVisiblealso for these types - but since it is ensured that the cells fall inside map bounds - the bounds check can be skipped. - Cell layers in at least
ShroudandShroudRenderercould contain these - already bound checked - cells. - Convert from existing counterparts to the new types early as possible.
- As an end result the currently used types are used in fewest places as possible.
- I.e. create
MapPPos[] ProjectedCellsToMapPPos(Map map, PPos[] puv)that drops/removes allPPosthat do not lie inside the bounds of the map. - I.e. when
ShroudRenderermarks neighbours cells dirty - these neighbours may lie outside the map. As soon as is validated that all neighbourPPoslie inside the bounds of the map they can be converted toMapPPostypes.
Advantage:
- Clear insight in which code parts deals with
UVvalues that could result in aUVvalues falling outside map bounds. - At least 3 bounds checks less per tick for each touched shroud cell for both
PPos,MPosand likelyCPos. - Enforces the developer to validate if cells are in bounds as early as possible, allowing them to be dropped if outside bounds.
Assumptions:
- Map bounds are static throughout a game.
Disadvantages:
- Requires structural code changes in many places.
Goal:
- Faster more efficient code.
- Cyclic access to the same cells throughout game components becomes much more performant.
Example of a stacktrace where Contains is called - again - for each modified Shroud cell noticed by the ShroudRenderer.
at OpenRA.CellLayer`1[T].Contains (OpenRA.MPos uv) [0x00000] in <01f2b3a4666b4d60b04d4a74458367e2>:0
at OpenRA.Map.ProjectedCellsCovering (OpenRA.MPos uv) [0x0000e] in <01f2b3a4666b4d60b04d4a74458367e2>:0
at OpenRA.Traits.Shroud.IsVisible (OpenRA.MPos uv) [0x00000] in <01f2b3a4666b4d60b04d4a74458367e2>:0
at OpenRA.Mods.Common.Traits.PlayerRadarTerrain.UpdateTerrainCell (OpenRA.MPos uv) [0x00019] in <59d80dd1432d4c7a8a2f414b546a9d8f>:0
at OpenRA.Mods.Common.Traits.PlayerRadarTerrain.UpdateShroudCell (OpenRA.PPos puv) [0x00023] in <59d80dd1432d4c7a8a2f414b546a9d8f>:0
at OpenRA.Traits.Shroud.OpenRA.Traits.ITick.Tick (OpenRA.Actor self) [0x000c9] in <01f2b3a4666b4d60b04d4a74458367e2>:0
at OpenRA.World+<>c.<Tick>b__112_0 (OpenRA.TraitPair`1[T] x) [0x00000] in <01f2b3a4666b4d60b04d4a74458367e2>:0
at OpenRA.WorldUtils.DoTimed[T] (System.Collections.Generic.IEnumerable`1[T] e, System.Action`1[T] a, System.String text) [0x00015] in <01f2b3a4666b4d60b04d4a74458367e2>:0
at OpenRA.World.Tick () [0x00151] in <01f2b3a4666b4d60b04d4a74458367e2>:0
at OpenRA.Game.InnerLogicTick (OpenRA.Network.OrderManager orderManager) [0x001bc] in <01f2b3a4666b4d60b04d4a74458367e2>:0
at OpenRA.Game.LogicTick () [0x0003e] in <01f2b3a4666b4d60b04d4a74458367e2>:0
at OpenRA.Game.Loop () [0x000f1] in <01f2b3a4666b4d60b04d4a74458367e2>:0
at OpenRA.Game.Run () [0x0003c] in <01f2b3a4666b4d60b04d4a74458367e2>:0
at OpenRA.Game.InitializeAndRun (System.String[] args) [0x00010] in <01f2b3a4666b4d60b04d4a74458367e2>:0
at OpenRA.Program.Main (System.String[] args) [0x00044] in <01f2b3a4666b4d60b04d4a74458367e2>:0
.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start by tracing the shown stack from Map.Contains and CellLayer.Contains through Map.ProjectedCellsCovering, Shroud.IsVisible, PlayerRadarTerrain, Shroud, and ShroudRenderer. Review how MPos, PPos, and CPos are used across these components before defining the scope. Done means bounds checks are safely reduced through validated map-position types without changing out-of-bounds behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp
- Domain
- game-dev, performance
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100