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

[Refactoring]: Reduce number of times the exposed area is calculated #3744

Closed
kwvanderlinde opened this issue Nov 22, 2022 · 3 comments
Closed
Assignees
Labels
code-maintenance Adding/editing javadocs, unit tests, formatting. performance A performance or quality of life improvement

Comments

@kwvanderlinde
Copy link
Collaborator

Describe the problem

When using Individual FoW, the exposed area is recalculated every time fog needs to be rendered. This includes every time the zone is panned or zoomed, even though that has no bearing on the exposed area.

Additionally, we calculate the exposed area twice. We should calculate it at most once per frame because it can be really expensive for complicated FoW.

The improvement you'd like to see

ZoneRenderer would cache the exposed area associated with a given PlayerView. If that view ever changes, the exposed area would be recalculated. Also, if FoW is ever explicitly flushed, then the exposed area would also be recalculated.

Expected Benefits

The first frame that renders a given set of exposed areas will still be expensive, but most frames would be very cheap. Panning and zooming the map would be much more responsive.

Additional Context

The expensive call that calculates the exposed area is zone.getExposedArea(view) in ZoneRenderer.renderFog(). While we only call that method once, we actually duplicate its logic a little further down when in the calculation of tempArea. I.e., tempArea and combined are always equal as far as I can tell, so there is no need to recalculate tempArea or union it with combined as we do.

For comparison, I made a prototypical code change (no cache invalidation) to measure the potential impact. Using the sole map in this test campaign, I saw an improvement from ~270-350 ms to <20ms for renderFog in most frames. Just removing tempArea gives a 2x speed up (naturally) and caching provided the remaining benefit.

@kwvanderlinde kwvanderlinde added code-maintenance Adding/editing javadocs, unit tests, formatting. performance A performance or quality of life improvement labels Nov 22, 2022
@kwvanderlinde
Copy link
Collaborator Author

There's also some room to clean up the various cases under the renderFogArea timer. If I'm reading things right:

  1. view.getTokens() is only null for a player that doesn't own any tokens or - if Individual Views is off - if there are no PCs with sight. So when we don't need a combined view, we should be able to not render anything in this case.
  2. In the non-null case for view.getTokens(), the combined/non-combined cases are identical and don't need to be distinguished.

@kwvanderlinde
Copy link
Collaborator Author

There's one more place where we repeatedly recalculate the exposed area: AbstractAStarWalker.calculatePath(). Not bad when only moving one token, but can get sluggish when there are several.

@kwvanderlinde
Copy link
Collaborator Author

As this is a pure refactoring and we haven't had complaints yet, I'll close this off. Bug reports can be filed if anything comes up.

@github-project-automation github-project-automation bot moved this from Needs Testing to Merged in MapTool 1.13.0 Nov 3, 2023
@kwvanderlinde kwvanderlinde moved this from Merged to Done in MapTool 1.13.0 Nov 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-maintenance Adding/editing javadocs, unit tests, formatting. performance A performance or quality of life improvement
Projects
Status: Done
Development

No branches or pull requests

1 participant