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

OpenRa trees edition #16698

Merged
merged 5 commits into from Jul 26, 2019

Conversation

@teinarss
Copy link
Contributor

commented Jun 15, 2019

image

Adding a new trait, "Static" to mark actors that we can cache for the blocking check in the Pathfinder.

Benchmark 1

Map

  • 256x256
  • Over 8000 trees

image

Results

bench_cell_cost_cache2

Benchmark 2

Map

  • 128x128
  • More similar to a normal game with a lot of moving actors and some static

Results

image

And without log scale
image

@GraionDilach

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2019

Should be named PermanentPathfinderBlock or something along that line. In the current modding environment, a Static trait name has no meaning.

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2019

@abcdefg30
Copy link
Member

left a comment

👍 to StaticPathfinderBlocker. I can see the static vs. permanent argument, but agree that permanent makes less sense here.

mods/ra/rules/defaults.yaml Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/World/Locomotor.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/World/Locomotor.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/World/Locomotor.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Pathfinder/PathGraph.cs Outdated Show resolved Hide resolved

@teinarss teinarss force-pushed the teinarss:priority_queue branch from 760d8fb to e8b3a08 Jun 15, 2019

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2019

Updated!

@pchote

This comment has been minimized.

Copy link
Member

commented Jun 15, 2019

Wouldn't it make more sense to add this as an interface that Building can implement?

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented Jun 16, 2019

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

[17:37:54] +pchote having a property doesnt seem too bad
[17:38:11] since that would just need to check if the crushable trait is defined
[17:39:07] but in principle it shouldnt matter at all - i will need to look at the pr in detail to comment further though
[17:39:35] the cache is per locomotor and crushing is too
[17:39:51] so in principle walls can still block for everything except mammoth tanks
[17:40:17] and that doesnt depend on Building at all
[17:40:49] the more important detail here is things that move vs things that dont
[17:41:31] but i may be missing something
[18:19:27] +pchote Mesacer_: thinking out loud here, so bear with me and then feel free to point out the fatal flaw
[18:20:19] 1) #16698 is a very scaled back version of the generalized crushability cache, which was sunk by the complications around crates/mines
[18:20:19] @orabot Pull request #16698 (open) by teinarss: OpenRa trees edition | http://bugs.openra.net/16698
[18:21:18] +pchote 2) #16408 is fundamantally sunk by not having the generalized crushability cache, or something similar to it
[18:21:19] @orabot Pull request #16408 (open) by tovl: Prevent harvesters getting stuck in gridlock by allowing units to give way. | http://bugs.openra.net/16408
[18:23:11] +pchote I don't remember the specifics about what caused problems with your general cache, but what is stopping us from having two BitSets for blocking - transient and static for each player?
[18:23:46] instead of saying "cell is blocked by player x" and then resolving owners at query time, have it as "cell is blocked for player X"
[18:24:10] and then update that whenever an actor moves or is added/removed from the world
[18:25:08] where IOccupySpace exposes the crush types to the actor map, which are then passed to the locomotor
[18:25:35] the special case logic for crates, mines, etc then shouldn't matter at all
[18:25:50] and we get #16698 and enable #16408 for free
[18:25:50] @orabot Pull request #16698 (open) by teinarss: OpenRa trees edition | http://bugs.openra.net/16698
[18:25:51] Pull request #16408 (open) by tovl: Prevent harvesters getting stuck in gridlock by allowing units to give way. | http://bugs.openra.net/16408
[19:24:05] Mesacer_ pchote i will need some time to digest this :P
[19:26:05] @orabot New pull request #16716 by RoosterDragon: Silence some doc errors in VS2019. | http://bugs.openra.net/16716
[19:26:29] Mesacer_ #16698 was sunk in complications around the crush feature, not for the crate?mines which we had removed from the icrushable interface
[19:26:29] @orabot Pull request #16698 (open) by teinarss: OpenRa trees edition | http://bugs.openra.net/16698
[19:27:28] Mesacer_ not that pr but the one that we closed
[19:29:55] +pchote ah right that one was actually caching cost, not crushability
[19:31:25] Mesacer_ the one we closed cached crush, moving, static if i remember correctly

@RoosterDragon

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

If I'm following this right, when the PathGraph asks GetCostToNode for a blocked cell, we can return a cached "yes it's blocked" result rather than having to calculate CanMoveFreelyInto? Is that the main idea here?

A few high-level questions:

  • Is there a reason/benefit to cache the path cost, rather than just the "is blocked" nature of the cell?
  • Why is there a cache per locomotor (as opposed to a single cache for the world)?
  • Why a marker trait? Since Locomotor owns the IsBlockedBy implementation can't it use the knowledge of that implementation to determine immobile actors that always block and determine that automatically? (at a glance, immobile, non-temporary-blocker and non-crushable) - perhaps there is some subtly in the previous discussion about crushing that answers this (or maybe I'm barking up the wrong tree entirely... not sure)
@GraionDilach

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

@RoosterDragon Regarding your last point - you're assuming too much there. For example, in RA, tree husks (charred trees) are separate actors. This means that while the tree itself can be killed, it's cell won't be freed up due to the charred husk. Cases like these require actual use-case insight and the code cannot determinate/would take a lot of edge-case-based assumptions to figure this out on his own. A marker trait poses less burden for maintenance for the developers and more exact setups for the modders.

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

If I'm following this right, when the PathGraph asks GetCostToNode for a blocked cell, we can return a cached "yes it's blocked" result rather than having to calculate CanMoveFreelyInto? Is that the main idea here?

A few high-level questions:

* Is there a reason/benefit to cache the path cost, rather than just the "is blocked" nature of the cell?

We cache both the cell cost and "is blocked". The gain caching the cell cost is arguable.

* Why is there a cache per locomotor (as opposed to a single cache for the world)?

Mostly because its based on a previous PR that we closed that also cached crushing. And also for the cell cost which is dependent on the locomotor. If we gonna do the crushing type again as suggested by pchote above we need it for each locomotor.

* Why a marker trait? Since `Locomotor` owns the `IsBlockedBy` implementation can't it use the knowledge of that implementation to determine immobile actors that always block and determine that automatically? (at a glance, immobile, non-temporary-blocker and non-crushable) - perhaps there is some subtly in the previous discussion about crushing that answers this (or maybe I'm barking up the wrong tree entirely... not sure)

That was my first idea:ish but https://logs.openra.net/?year=2019&month=05&day=25#20:37:57

@teinarss teinarss force-pushed the teinarss:priority_queue branch 2 times, most recently from b9ec2e4 to e1baa4e Jul 1, 2019

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2019

Updated.
"Struct with flag opti" is this PR
image

image

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2019

For benchmark 1
image

@pchote pchote added this to the Next Release milestone Jul 3, 2019

@pchote
Copy link
Member

left a comment

Some initial comments. I haven't yet looked at the meat of the cost/blocking cache logic.

OpenRA.Game/Player.cs Outdated Show resolved Hide resolved
OpenRA.Game/Player.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/World/CreateMPPlayers.cs Outdated Show resolved Hide resolved
OpenRA.Game/Traits/TraitsInterfaces.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/World/ActorMap.cs Outdated Show resolved Hide resolved
OpenRA.Game/Primitives/LongBitSet.cs Show resolved Hide resolved
OpenRA.Mods.Common/Traits/World/Locomotor.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/World/Locomotor.cs Outdated Show resolved Hide resolved

@teinarss teinarss force-pushed the teinarss:priority_queue branch from e5444f4 to d137a3d Jul 7, 2019

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2019

Updated!

@teinarss teinarss force-pushed the teinarss:priority_queue branch from d137a3d to ddb05d2 Jul 9, 2019

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2019

Updated with the things discussed.

@teinarss teinarss force-pushed the teinarss:priority_queue branch 4 times, most recently from 63b854c to 0ebc455 Jul 20, 2019

@@ -84,6 +84,7 @@ sealed class PathGraph : IGraph<CellInfo>

readonly CellConditions checkConditions;
readonly LocomotorInfo locomotorInfo;

This comment has been minimized.

Copy link
@reaperrr

reaperrr Jul 26, 2019

Contributor

I doubt caching locomotorInfo separately is worth it now. Can you remove this and use locomotor.Info directly instead?

@@ -45,13 +45,16 @@ class HarvesterTraitWrapper
public readonly Harvester Harvester;
public readonly Parachutable Parachutable;
public readonly LocomotorInfo LocomotorInfo;

This comment has been minimized.

Copy link
@reaperrr

reaperrr Jul 26, 2019

Contributor

Same here.

var li = self.Info.TraitInfo<MobileInfo>().LocomotorInfo;
using (var search = PathSearch.FromPoints(self.World, li, self, refs.Values.Select(r => r.Location), self.Location, false)

var locomotor = mobile.Locomotor;

This comment has been minimized.

Copy link
@reaperrr

reaperrr Jul 26, 2019

Contributor

The old var was mostly about reducing line length of the using, I think we can drop this now and use mobile.Locomotor directly below.

@@ -64,7 +64,10 @@ public PathFinder(World world)

public List<CPos> FindUnitPath(CPos source, CPos target, Actor self, Actor ignoreActor)
{
var li = self.Info.TraitInfo<MobileInfo>().LocomotorInfo;
var mobile = self.Trait<Mobile>();
var li = mobile.Info.LocomotorInfo;

This comment has been minimized.

Copy link
@reaperrr

reaperrr Jul 26, 2019

Contributor

I'd either drop this (likely no real benefit) and use locomotor.Info directly, or at least move it below locomotor so you can just populate it by locomotor.Info.

var li = mi.LocomotorInfo;
var mobile = self.Trait<Mobile>();
var li = mobile.Info.LocomotorInfo;
var mi = mobile.Info;

This comment has been minimized.

Copy link
@reaperrr

reaperrr Jul 26, 2019

Contributor

If you switch order of these two, you can populate li through mi.LocomotorInfo.

@reaperrr

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2019

LGTM after those final nits 👍

@teinarss teinarss force-pushed the teinarss:priority_queue branch from c304949 to 1641161 Jul 26, 2019

@reaperrr reaperrr merged commit cc84daa into OpenRA:bleed Jul 26, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@reaperrr reaperrr referenced this pull request Jul 26, 2019
@RoosterDragon

This comment has been minimized.

Copy link
Member

commented Jul 26, 2019

This appears to have caused tick times to increase on the RA shellmap from ~10ms to ~14ms on my machine. Can anybody else reproduce? Profiling suggests Locomotor.UpdateCellBlocking is responsible for the increase.

@pchote

This comment has been minimized.

Copy link
Member

commented Jul 26, 2019

Confirmed (~6 → 9ms in my case), and derp at the massive blind spot that we had in testing/profiling this.

@reaperrr

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2019

Yeah. Last commit seems to be the culprit.

@pchote

This comment has been minimized.

Copy link
Member

commented Jul 26, 2019

We can save this by changing Locomotor.CellsUpdated to be lazy - mark the cells as dirty, but defer calling UpdateCellBlocking and UpdateCellCost until a pathfind actually needs to know the value.

At this point its probably going to be better to revert this, and aim an improved version for Next + 1.

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2019

Or go parallel?

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2019

image

@pchote

This comment has been minimized.

Copy link
Member

commented Jul 27, 2019

The perf regression was caused by a silly bug rather than any fundamental algorithm issue - fixed in #16846.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.