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

Spatially partition some selection/overlay renderables #14082

Merged
merged 3 commits into from Dec 23, 2017

Conversation

Projects
None yet
5 participants
@reaperrr
Contributor

reaperrr commented Sep 28, 2017

These were drawn regardless of viewport position before.

On my machine, in bleed RA, the difference between 200 selected actors on-screen and 200 off-screen was only 1.2-1.5 ms per Render tick on bleed, with this PR that increases to around 5ms, dropping render time per render tick from over 10 to around 5.5 ms with 200 off-screen selected actors.

The primary gain possibly comes from the skipped FogObscures checks, rather than the skipped rendering itself, but the gains are potentially significant either way.

Note: I didn't feel like dealing with calculating bounds for target lines or range circles (probably wouldn't be worth the effort anyway), so I concentrated on overlays that are drawn on top of or very close to the selected actor.

Closes #12111.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Oct 14, 2017

Member

Note: I didn't feel like dealing with calculating bounds for target lines or range circles (probably wouldn't be worth the effort anyway), so I concentrated on overlays that are drawn on top of or very close to the selected actor.

These are actually the most worthwhile, because they require a switch to the line renderer. Certainly not a job for this PR though.

Member

pchote commented Oct 14, 2017

Note: I didn't feel like dealing with calculating bounds for target lines or range circles (probably wouldn't be worth the effort anyway), so I concentrated on overlays that are drawn on top of or very close to the selected actor.

These are actually the most worthwhile, because they require a switch to the line renderer. Certainly not a job for this PR though.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Oct 14, 2017

Contributor

These are actually the most worthwhile, because they require a switch to the line renderer. Certainly not a job for this PR though.

What I meant is not that those don't cost enough performance. Rather,

  • For circles, ignoring CombatDebugOverlays, you rarely have more than a few actors displaying a range circle at once (though it might be a few more with actors that have sensors, but RA ships are the only thing that comes to mind here).
  • For target lines, the reason why I'm not expecting large gains is that since they're usually triggered by manual orders and linger only for a short amount of time, they'll usually be at least partially on-screen most of their lifetime, meaning the chance that they'll be entirely outside the ScreenMap is quite low in practice, so the percentage of avoided renderer switches is probably quite low.
Contributor

reaperrr commented Oct 14, 2017

These are actually the most worthwhile, because they require a switch to the line renderer. Certainly not a job for this PR though.

What I meant is not that those don't cost enough performance. Rather,

  • For circles, ignoring CombatDebugOverlays, you rarely have more than a few actors displaying a range circle at once (though it might be a few more with actors that have sensors, but RA ships are the only thing that comes to mind here).
  • For target lines, the reason why I'm not expecting large gains is that since they're usually triggered by manual orders and linger only for a short amount of time, they'll usually be at least partially on-screen most of their lifetime, meaning the chance that they'll be entirely outside the ScreenMap is quite low in practice, so the percentage of avoided renderer switches is probably quite low.
@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Nov 18, 2017

Contributor

Updated.

Contributor

reaperrr commented Nov 18, 2017

Updated.

@pchote

Looks reasonable, just one style nit.

Would you mind squashing the last commit into the earlier one(s)? When reviewing this I ended up squashing the last three together so that I could see the the aggregate changes.

Show outdated Hide outdated OpenRA.Game/Graphics/WorldRenderer.cs Outdated
@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Nov 18, 2017

Contributor

Updated.

Contributor

reaperrr commented Nov 18, 2017

Updated.

@reaperrr reaperrr modified the milestones: Next + 1, Next release Nov 19, 2017

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Nov 26, 2017

Contributor

Updated.

Contributor

reaperrr commented Nov 26, 2017

Updated.

@penev92

This comment has been minimized.

Show comment
Hide comment
@penev92

penev92 Nov 30, 2017

Member

Needs a rebase.
Also looking at the commits there are review comments that don't seem to have been addressed but have been hidden (maybe because of pushes over the old code?). Please don't forget those. I guess those were addressed in later commits, sorry.
(Tip to reviewers: don't leave comments on specific commits, GitHub is not exactly deterministic about dealing with those... Leave them in the "Files changed" tab instead.) I am seeing some improvement in that regard compared to 2-3 years ago, but assumed here that that was the case again.

Member

penev92 commented Nov 30, 2017

Needs a rebase.
Also looking at the commits there are review comments that don't seem to have been addressed but have been hidden (maybe because of pushes over the old code?). Please don't forget those. I guess those were addressed in later commits, sorry.
(Tip to reviewers: don't leave comments on specific commits, GitHub is not exactly deterministic about dealing with those... Leave them in the "Files changed" tab instead.) I am seeing some improvement in that regard compared to 2-3 years ago, but assumed here that that was the case again.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Dec 1, 2017

Contributor

Rebased.

Contributor

reaperrr commented Dec 1, 2017

Rebased.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Dec 10, 2017

Member

As mentioned in #14482, for all our sanity please lets merge that first and then deal with the conflicts/changes here afterwards.

Member

pchote commented Dec 10, 2017

As mentioned in #14482, for all our sanity please lets merge that first and then deal with the conflicts/changes here afterwards.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Dec 11, 2017

Contributor

Rebased.

Contributor

reaperrr commented Dec 11, 2017

Rebased.

@pchote

pchote approved these changes Dec 22, 2017

@pchote pchote added the PR: Needs +2 label Dec 22, 2017

@RoosterDragon RoosterDragon merged commit fbc18df into OpenRA:bleed Dec 23, 2017

2 checks passed

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

This comment has been minimized.

Show comment
Hide comment
@RoosterDragon
Member

RoosterDragon commented Dec 23, 2017

@reaperrr reaperrr deleted the reaperrr:aboveshroud-screenmap branch Feb 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment