search

SWY1985 / CivOne

An open source implementation of Sid Meier's Civilization.
http://www.civone.org/
Creative Commons Zero v1.0 Universal
244 stars 49 forks source link

Low performance with many cities #316

Closed AlexFolland closed 7 years ago

AlexFolland commented 7 years ago

In a game with many (>100) cities, each unit movement incurs roughly a 7-second delay. Before it gets to this point, the delay slowly increases throughout the game. This makes testing large-scale games difficult. I am guessing this is due to what happens in the nested foreach loops in MovementDone which relocate cities' resource tiles. Turns ending do not incur the delay.

SWY1985 commented 7 years ago

There's some performance issues that really need to get fixed. That's an ongoing process though. I will keep this issue open, but I will not focus on this for now. Performance optimization is a later stage of development. I understand it's annoying, and I will eventually get it fixed.

AlexFolland commented 7 years ago

No worries! Besides, this is not only for you. This is an open-source project with the most permissive license possible, after all. Someone else may be able to find the problem and fix it. I am excited to play CivOne in its "finished" form and see where it can go from there, hence all the testing and issue reports. :) A Civ 1 experience with a modern engine is an awesome prospect.

SWY1985 commented 7 years ago

I played a full game last night, and it's very annoying, the further the game progresses, the slower it becomes. There's definitely something wrong and I'm going to fix it.

SWY1985 commented 7 years ago

I've played an entire game after today's code changes and the delays between turns appear to have been fixed. Can you confirm this?

AlexFolland commented 7 years ago

Will do! I'm going to sleep right now, but I'm excited to do so while simultaneously testing all your other most recent fixes!

SWY1985 commented 7 years ago

Is this issue resolved?

AlexFolland commented 7 years ago

Unfortunately not. The game I started didn't have the grid of cities, but I still see a major delay between unit movements. It's not 7 seconds long because I don't have as many cities as I did in that other game though.

SWY1985 commented 7 years ago

I've tested with about 25 cities and everything went well. I will test with over 100 cities and see what happens.

AlexFolland commented 7 years ago

It may have extra performance issues with cities very close together. I'm just guessing though.

AlexFolland commented 7 years ago

I made a couple videos comparing with a similar number of cities in Civilization and CivOne.

This one is CivOne, showing a significant movement delay with 26 cities on the map.

http://lex.clansfx.co.uk/requested/CivOnemovementdelay.mkv

This one is Civilization, showing no significant movement delay with at least 36 cities.

http://lex.clansfx.co.uk/requested/Civilizationnomovementdelay.mkv

SWY1985 commented 7 years ago

Okay, thanks a lot for these videos. I now know what code I have to improve to solve this issue. Problem with performance testing is that I have two pretty beefy development machines, so usually I don't experience a lot of delays.

AlexFolland commented 7 years ago

I have a very beefy machine, myself: i7-2600k overclocked to 3.9GHz. Note that these videos were recorded with a CPU encoder, x264, and there were 0 frames dropped during recording. There was plenty of spare CPU time available. I suspect that CivOne is doing some sort of consistent idling during these delays, especially considering it stays at 100% of its CPU core at all times. On that note, to prevent 100% CPU core usage, it needs something like a Sleep(1) between main loop synchronization points to prevent it from wasting CPU time, like this section of a program I wrote a while ago and improved recently: https://github.com/AlexFolland/turbo/blob/master/turbo.cpp#L105

SWY1985 commented 7 years ago

You were right, the Game.MovementDone function was a mess and it didn't do its job properly. I have now fixed it so it will only check the cities around the unit instead of all cities. I've also made a correction to the checking algorithm so that it will actually do what it's supposed to do. It should act a lot more like the original game now.

If you have time, could you please test it?

AlexFolland commented 7 years ago

Will do! I think I can just load my old game with lots of cities, even though it saved and loads improperly. I'll try that.

SWY1985 commented 7 years ago

Wait, there's a bug! I need to do one more commit... one moment please.

AlexFolland commented 7 years ago

Sure.

SWY1985 commented 7 years ago

Okay, it's fixed now. There was an error causing the game to crash when you move too close to the poles. It's fixed with the latest commit.

SWY1985 commented 7 years ago

I have tested this by spawning about 100 Settlers (right click allows you to spawn multiple units quickly) and making them build cities, then moving around.

AlexFolland commented 7 years ago

It's definitely much better now. Great job!

SWY1985 commented 7 years ago

Okay, I'm going to close this issue then, thanks for testing.

AlexFolland commented 7 years ago

There is still a significant delay when moving around near friendly cities. I can make a video of it if desired. Friendly cities should not matter, as friendly units do not affect tile placement. Also, Civilization only updates tile placement once at the end of the turn, not on every movement step, so this shouldn't affect movement anyway.

SWY1985 commented 7 years ago

Good point, I'll see how I can improve this.