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

Fixes in locomotor path cache #16861

Merged
merged 4 commits into from Aug 10, 2019

Conversation

@teinarss
Copy link
Contributor

commented Jul 29, 2019

Fixes #16845
Fixes #16848
Fixes #16863
Fixes: #16862
Fixes: #16880

Note: Is not dependent on the suggested change on setting MovementType to none after the movement is finished. But if we fix that we can cache the case when a crushable unit has moved.

@teinarss teinarss added this to the Next Release milestone Jul 29, 2019

@teinarss teinarss changed the title Fix lm cache2 Fixes in locomotor path cache Jul 29, 2019

@fusion809
Copy link
Contributor

left a comment

This PR seems to fix the problem (saying "seems to" as it is intermittent, so it is difficult to know for certain).

@teinarss teinarss force-pushed the teinarss:fix_lm_cache2 branch from 63c98b0 to 4dfa61b Aug 1, 2019

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

Updated
Had to remove the cached crushable logic because Crushable trait is conditional
Added support to cache the logic to determine if the cell has a ITemporaryBlocker

@pchote

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

Conditional Crushable shouldn't be a problem, surely? We just need to (a) filter disabled traits when calculating the cached value and (b) make Crushable ping the ActorMapLocomotor so its cells can be marked dirty and recalculated when it is enabled/disabled.

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

Would be better to do this in a anther PR, where we can remove the ICrushable interface altogether.

@teinarss teinarss force-pushed the teinarss:fix_lm_cache2 branch from 4dfa61b to 49a836a Aug 1, 2019

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

Updated, fixed the crushable thing now. Still have do the update when the condition changes.

@teinarss teinarss force-pushed the teinarss:fix_lm_cache2 branch from 49a836a to f0d16fc Aug 1, 2019

@teinarss teinarss force-pushed the teinarss:fix_lm_cache2 branch from f0d16fc to 7daf98d Aug 2, 2019

@abcdefg30
Copy link
Member

left a comment

Gates and transports work again.

OpenRA.Mods.Cnc/Traits/Mine.cs Outdated Show resolved Hide resolved
@ubitux

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

Since you mentioned it was supposed to fix it, I tried your branch but can still reproduce this:

stack-teinarss

@abcdefg30

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

I suppose this can/needs to be fixed by another PR (since this PR doesn't make things worse).

@teinarss teinarss force-pushed the teinarss:fix_lm_cache2 branch from 18d3864 to b25f0c8 Aug 5, 2019

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

Updated: hoping that everything will work now. Added a hashset containing all cells that have been updated but not rechecked, so when queried for the blocking it will look at that flag and determine if we need to update this cell with new info about blocking.

@teinarss teinarss force-pushed the teinarss:fix_lm_cache2 branch from b25f0c8 to 8b1679e Aug 5, 2019

@teinarss teinarss force-pushed the teinarss:fix_lm_cache2 branch from ac78f2e to e64bf32 Aug 9, 2019

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

Updated

@teinarss teinarss force-pushed the teinarss:fix_lm_cache2 branch from fd159ac to 07b1ec5 Aug 10, 2019

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2019

Updated

@pchote
Copy link
Member

left a comment

LGTM, and thanks - the code is now much easier to understand.
Just a few more style / renaming requests to help the readability a bit further

actorMap.CellUpdated += CellUpdated;
blockingCache = new CellLayer<CellCache>(map);
cellsCost = new CellLayer<short>(map);
dirtyCells.Clear();

This comment has been minimized.

Copy link
@pchote

pchote Aug 10, 2019

Member

This is in the trait rather than the traitinfo, so dirtyCells should be guaranteed to be clear already?

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/Traits/World/Locomotor.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/World/Locomotor.cs Outdated Show resolved Hide resolved
@pchote

This comment has been minimized.

Copy link
Member

commented Aug 10, 2019

I had originally suggested that this fixes #16901, but it seems that this is only because the PR is based on an old bleed commit, before that regression was introduced. The problem returns when this is rebased on latest bleed.

Edit: Maybe not #16901 exactly, but: https://i.imgur.com/yn46KFu.mp4

@teinarss teinarss force-pushed the teinarss:fix_lm_cache2 branch from d5115f5 to fbdd96f Aug 10, 2019

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2019

updated

@pchote pchote added the PR: Needs +2 label Aug 10, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor

commented Aug 10, 2019

Didn't notice any issues while watching the RA shellmap for a while. LGTM after that typo fixup.

@teinarss teinarss force-pushed the teinarss:fix_lm_cache2 branch from fbdd96f to 56c2f16 Aug 10, 2019

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2019

Updated!

@reaperrr reaperrr merged commit 4193247 into OpenRA:bleed Aug 10, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.