OpenRA / OpenRA/OpenRA

Introduce [MP]PosWithUVAlreadyKnownToBeInMapBounds

Open
#18,778 10 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Idea/Wishlist Performance
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, MapPPos and MapCPos types 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 UV value lie outside map bounds. The creator is responsible for checking before.
  • Allow conversion to their existing counterparts MPos PPos and CPos etc. To i.e. allow calculations on them where result may fall outside bounds.
  • Implement methods like Map.ProjectedCellsCovering and Shroud.IsVisible also 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 Shroud and ShroudRenderer could 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 all PPos that do not lie inside the bounds of the map.
  • I.e. when ShroudRenderer marks neighbours cells dirty - these neighbours may lie outside the map. As soon as is validated that all neighbour PPos lie inside the bounds of the map they can be converted to MapPPos types.

Advantage:

  • Clear insight in which code parts deals with UV values that could result in a UV values falling outside map bounds.
  • At least 3 bounds checks less per tick for each touched shroud cell for both PPos, MPos and likely CPos.
  • 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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.