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

Overhaul MapPreview rule loading #19317

Merged
merged 6 commits into from Apr 21, 2021
Merged

Conversation

pchote
Copy link
Member

@pchote pchote commented Apr 5, 2021

This PR reworks the way MapPreview handles custom rules, with two major goals in mind:

  1. Fix the atrocious performance issues with the mission browser. On my system, and I suspect most others, opening the mission browser will trigger unusable performance on the shellmap for 10+ seconds while the mission rules are cached in the "background" (doesn't feel very background to me).

  2. Require maps to pass the lint checks before dedicated servers will allow them to be used. This is responding to the flood of crash reports and complaints from the competitive community caused by copying the previous generation RAGL maps into release-20210321 without verifying that they were actually compatible (they weren't).

The first point is achieved by replacing the general Ruleset parsing with focused parsing for just the world and player definitions, which cuts the parse time for mission/custom maps from ~250ms to ~25ms each. This is small enough to get away with removing the asynchronous code completely, and load the rules up-front at the cost of only 1-2s of additional startup time in RA (less in the other mods).

The previous (very limited) rule syntax checks in the lobby were not compatible with this new approach, so they are moved to the server using a new LobbyInfo.GlobalSettings.MapStatus field to relay the result back to the clients. This decoupled approach makes it easy to add additional and longer checks, so I scope creeped a solution to the second point while I was here.

If we can get this merged within a reasonable timeframe I expect to work on a second PR that would bring the "map not available on server" handling under the same system (edit: ended up taking a different approach), which would then naturally extend to supporting server-side map allow/deny lists to make @ubitux's life easier with managing the map pool on the competitive servers.

I strongly recommend reviewing by commit, which makes it a lot easier to understand the different parts of this.

@pchote
Copy link
Member Author

pchote commented Apr 5, 2021

I realized while writing the PR description that I have forgotten to add a new lint rule to enforce the new World and Player inheritance restrictions.

@pchote pchote force-pushed the mappreview-linting branch 2 times, most recently from 81b8ca7 to 14d6b7b Compare April 5, 2021 09:43
@pchote
Copy link
Member Author

pchote commented Apr 5, 2021

Updated. I have added the missing lint test and also bumped the orders protocol to avoid breakage from the alternative server implementation.

@reaperrr
Copy link
Contributor

reaperrr commented Apr 5, 2021

Fix the atrocious performance issues with the mission browser. On my system, and I suspect most others, opening the mission browser will trigger unusable performance on the shellmap for 10+ seconds while the mission rules are cached in the "background" (doesn't feel very background to me).

Hm, not on my system, opening the mission browser in RA is virtually instant here with no noticable lag on UI or shellmap afterwards (Win10, Ryzen 3600, 16GB DDR4-3200, SATA SSDs only).

Doesn't change that this overhaul is certainly justified, I just won't be able to test the performance difference since I can't reproduce the perf issue on bleed.

reaperrr
reaperrr previously approved these changes Apr 5, 2021
Copy link
Contributor

@reaperrr reaperrr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't see any issues glancing over the code and mission/skirmish browsers still work fine for me

@pchote
Copy link
Member Author

pchote commented Apr 5, 2021

Hmm, odd.

This is what it looks like for me on current bleed:

Screen.Recording.2021-04-05.at.13.29.40.mov

This only started relatively recently, not sure if it was due to the switch to .NET5, due to all the new missions we've merged over the last year, or something else. In any case, this PR puts an end to it.

@pchote
Copy link
Member Author

pchote commented Apr 6, 2021

Updated:

  • Moved MapStatusCache and MapStatus definitions from OpenRA.Network into OpenRA.Server and OpenRA.Network.Session respectively.
  • Fixed compile error on "Optimize MapPreview rule loading." commit (it relied on changes that were introduced in the next commit)
  • Fixed map installation (one regression from Replace Webclient with HttpClient #18995 and a second introduced in this PR)

@reaperrr
Copy link
Contributor

Needs rebase.

@pchote
Copy link
Member Author

pchote commented Apr 10, 2021

Rebased.

reaperrr
reaperrr previously approved these changes Apr 10, 2021
@pchote
Copy link
Member Author

pchote commented Apr 11, 2021

Now needs a rebase/update for #19239.

@pchote
Copy link
Member Author

pchote commented Apr 11, 2021

Rebased.

This allows text to be displayed earlier in the loading screen.
Copy link
Contributor

@teinarss teinarss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also looks like public enum MapRuleStatus in MapPreview is not used.

OpenRA.Game/Map/MapPreview.cs Outdated Show resolved Hide resolved
The previous asynchronous approach did not work particularly well,
leading to large janks when switching to custom maps or opening the
mission browser.

This commit introduces two key changes:

 * Rule loading for WorldActorInfo and PlayerActorInfo is made
   synchronous, in preparation for the next commit which will
   significantly optimize this path.
 * The full ruleset loading, which is required for map validation,
   is moved to the server-side and managed by a new ServerMapStatusCache.
   The previous syntax check is expanded to include the ability to run
   lint tests.
@pchote
Copy link
Member Author

pchote commented Apr 20, 2021

Updated.

@teinarss teinarss merged commit bb8a634 into OpenRA:bleed Apr 21, 2021
@pchote pchote deleted the mappreview-linting branch August 21, 2021 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants