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

Reduce Map.Contains(CPos) cost in legacy mods #16840

Merged
merged 1 commit into from Aug 7, 2019

Conversation

@reaperrr
Copy link
Contributor

commented Jul 26, 2019

If a mod uses rectangular maps and no height levels, checking if the CPos is within Map.Bounds should be enough and cheaper than the whole ToMPos conversion and checks.

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2019

Ping @teinarss for benchmarks in some non-TS mod.

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

@teinarss

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2019

Looks like a win to me
image

@abcdefg30
Copy link
Member

left a comment

This can be simplified to

// If the mod uses flat & rectangular maps, ToMPos and Contains(MPos) create unnecessary cost.
// Just check if CPos is within map bounds.
if (Grid.MaximumTerrainHeight == 0 && Grid.Type == MapGridType.Rectangular)
	return Bounds.Contains(cell.X, cell.Y);
Reduce Map.Contains(CPos) cost in legacy mods
If a mod uses rectangular maps and no height levels,
checking if the CPos is within Bounds
should be enough and cheaper than the whole ToMPos
conversion and checks.

@reaperrr reaperrr force-pushed the reaperrr:perf-map-contains branch from a576f30 to 69d71c6 Aug 3, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2019

Updated.

@pchote
pchote approved these changes Aug 6, 2019

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

@abcdefg30 abcdefg30 merged commit b0a7865 into OpenRA:bleed Aug 7, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@abcdefg30

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

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