search

joaofarias / csl-traffic

A WIP mod for Cities: Skylines to improve traffic.
91 stars 30 forks source link

Performance Optimizations for Pathfinding #61

Closed Nefarion closed 9 years ago

Nefarion commented 9 years ago

This is WIP please do not merge jet :+1:

Edit: It works again! :smile:

originalfoo commented 9 years ago

Ah, proper variable names! * hugs @Nefarion *

originalfoo commented 9 years ago

isBycicle -> isBicycle

originalfoo commented 9 years ago

This might be a stupid suggestion, but looking through the pathfinding code there seems to be lots of runtime checks to see if, for example, a pedestrian can use a certain road/path/etc. Why not store that on the node or segment lookup, defined when user places roads/paths/etc, so it can just be quickly referenced by the pathfinder?

Nefarion commented 9 years ago

As far as i figured out, there could be multiple threads running the finding on the same path... which is total crap i think

joaofarias commented 9 years ago

@Nefarion Thanks for working on this. There are just a couple of things that I would like to ask you to take into account while working on it.

1 - Be consistent with the code's nomenclature - i. e., instance members start with 'm' and use camelCase, constants are all caps with '' separating words, methods are CamelCase (starting with a capital letter). Also, put all variables/constants in the beginning of the class with the others.

2 - This one is a personal request: please, do not use 'var'. It does not affect performance at all and only makes the code harder to read since you can't immediatly figure out what the type of the variable is.

Now, regarding your specific changes, I'll be reviewing them all later and run them through you for us to discuss them, but I've already seen some things from just a quick look. I'll make some comments in the commit itself. (Edit: didn't realize that the commit comments showed up here :/ )

@aubergine10 We'd have to change half the code to make that happen or use a separate list (like what I'm doing with lanes) wich results in more performance issues.

Nefarion commented 9 years ago

@joaofarias, i removed the comments 1 by 1 to keep track of the changes... should have fixed everything you pointed out, and a bit more too!

joaofarias commented 9 years ago

Nice, thanks :)

I just took a quick look but it's looking good. Finding the bug that's preventing cars from spawning won't be easy though. Probably some wrong flag is being set.

One question though. What was the problem with the assembly version? I'm just asking because I remember people having problems with the assembly version when updating mods on steam.

joaofarias commented 9 years ago

Oh, I just remembered that the version you have has a bug that makes cars despawn. It might be the same that makes cars not spawn at all. I've been working on that and will commit the fix soon.

Edit:

This was wrong, you should only put AssemblyVersion here.

Why?

Nefarion commented 9 years ago

Windows is checking for [AssemblyFileVersion], and if it exists, the "1.0.*" does not work in the version info...

joaofarias commented 9 years ago

So it's the format that is wrong, not that I can't put the AssemblyFileVersion in there. Being absent, does it get the same version from the AssemblyVersion?

Don't get me wrong, I appreciate that you fixed it. I just like to know the HOWs and WHYs :)

Nefarion commented 9 years ago

The Wildcard Symbol * only works In AssemblyVersion, if there is no AssemblyFileVersion, and yes, they both get the same value then.

Nefarion commented 9 years ago

@joaofarias, i merged with optimizations and squashed the commits.

joaofarias commented 9 years ago

Why can't I see the diff in the CustomPathFind file? O_O

Nefarion commented 9 years ago

Here you go: http://www.mergely.com/MPm8c7b6/ And for the record, caching structs is not a good idea -.-

joaofarias commented 9 years ago

I'm just curious as to why github doesn't show the changes... Never happened before.

Nefarion commented 9 years ago

Its too much changes for one file at about 1000 lines they don't show it i think.

joaofarias commented 9 years ago

Well... that sucks. :/

I'm working on making vehicles take road changes into account immediately. It's working nicely for vehicle restrictions but the intersection routings are causing weird despawns - I'm not changing anything in the pathfinding, only in the AI classes so it shouldn't create any conflict.

When I get that working, I'll review your changes and get back to you if I find anything for us to discuss.

Thanks for your work :)

Nefarion commented 9 years ago

You're welcome :smile:

And here you have the info for the diffs (100KB max to view)

joaofarias commented 9 years ago

Thanks for the info.

Just a heads-up: I found out that the problem is actually in the routing, not what I was working on. If you see buses despawning and/or vehicles avoiding some road, that's what's happening.

Nefarion commented 9 years ago

Hmm, i think we need to understand how they are doing their pathfinding, maybe we can optimize it a bit (reguarding our new rules)

joaofarias commented 9 years ago

Hey. So, I've been reviewing and trying your changes and it looks like the pathfinding is broken. :(

Some cars are stopping causing huge piles of traffic. Also, times are getting upwards of 6ms/path with your version.

In line 559, you added "TODO: Do these checks even work? (those are flags)". What do you mean? Why wouldn't they work?

Nefarion commented 9 years ago

Regarding the Todo: comment, because they are checked with == and not with value & flag != 0, but they work, i forgot to remove it.

The other thing, i am currently playing a huge town without traffic jams, with the build i uploaded :sweat_smile:

joaofarias commented 9 years ago

I'll give it another try tomorrow, then.

Basically, I left your version running to see what time I got with it and when I came back I had a van stopped sideways in the middle of the highway and a huge line behind it. I checked the rest of the city and it was happening in a few more places. I restarted the game to try again but same thing happened.

Have you compared the times from your version with the ones from the latest version on this branch?

Nefarion commented 9 years ago

To be honest, i did not, but i will, (after tuesday, got exam on tuesday) :>

Edit: e9ae2fbe779448def14a04d7eb02540cfee1e823 Average is 2.5ms - 2.9ms for me Edit2: my branch is worse(5-15ms), will look into it after tuesday

Nefarion commented 9 years ago

I had to revert a lot to get it working, I think its best if i open a new PR with the working changes, and keep the good comments from this one!