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

Improved Pathfinder Debugging #370

Closed
wants to merge 1 commit into from
Closed

Conversation

VictorPhilipp
Copy link
Collaborator

  • Path-find logging is batched
  • switch 26 toggles whether only failed path-finds should be logged

@VictorPhilipp VictorPhilipp added LABS TM:PE LABS branch STABLE TM:PE STABLE branch technical Tasks that need to be performed in order to improve quality and maintainability labels Jun 9, 2019
@VictorPhilipp VictorPhilipp requested a review from krzychu124 June 9, 2019 17:49
@krzychu124
Copy link
Member

Testing...

Copy link
Member

@krzychu124 krzychu124 left a comment

Choose a reason for hiding this comment

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

I think you forgot to change Global config version number because I'm getting Array index is out of range 😉

@krzychu124
Copy link
Member

krzychu124 commented Jun 9, 2019

First, I am not sure how it supposed to work.

I thought that after that little change we will see only log with list of unreachable segments with pathfinding history stack but I can see that I was wrong 😄

  • ~15min game running and TMPE.log has 22m lines which takes ~0.6GB of disk space
    Do you have some sort of processing tool to cut out useful info, because it seems that is too much data to do that manually with just scrolling and searching tool or I just don't have 'that' skill yet 😅

String.Formatis killing performance of logging, that's for sure, but I am sure you know that and I think that performance gain of logging wasn't the main intention of this PR 😉

@originalfoo originalfoo added this to the 10.21 milestone Jun 10, 2019
@VictorPhilipp
Copy link
Collaborator Author

VictorPhilipp commented Jun 10, 2019

I have not raised the version because extending the Debug class does not affect regular users. Should I increase it nevertheless?

First, I am not sure how it supposed to work.

The synchronization code in Log._Debug adds a siginificant overhead that I wanted to avoid. That's why I make it accumulate log entries until path-finding is complete. Then, I decide (based on the outcome of path-finding and based on Switches[26]) whether to save the accumulated entries to the log file in one single batch. Use case: In order to investigate on #361 I wanted to know why taxi path-finds failed. Now, with Switches[26] we can now decide to only log failed path-finds.

* ~15min game running and TMPE.log has 22m lines which takes ~0.6GB of disk space
  Do you have some sort of processing tool to cut out useful info, because it seems that is too much data to do that manually with just scrolling and searching tool or I just don't have 'that' skill yet 😅

Well, I do it manually indeed. I choose a segment in-game and check whether path-finding ever considered this segment.

It would be really nice to have a visual in-game path-find debugger!

String.Formatis killing performance of logging, that's for sure, but I am sure you know that and I think that performance gain of logging wasn't the main intention of this PR 😉

I thought that String.Format uses a StringBuilder internally? I expect it to have a certain overhead but it should not be a performance killer? Lighten me up! :-)

@krzychu124
Copy link
Member

I will check if version change is necessary because I remember that I changed switch[0] to true in xml config but forgot to add new one for log failed path-finds only.

I thought that String.Format uses a StringBuilder internally? I expect it to have a certain overhead but it should not be a performance killer? Lighten me up! :-)

Yeah, you were right, String.Format uses StringBuilder internally (just checked source code 😉 )

Visual in-game path-find debugger 😍

@krzychu124
Copy link
Member

krzychu124 commented Jun 10, 2019

Yup, it seems that the mod is not seeing that additional flag from Debug class because it was overwritten by deserializer. Line 171 not called to reset Debug instance with new switches.
https://github.com/krzychu124/Cities-Skylines-Traffic-Manager-President-Edition/blob/cda3cd9ebb748fc4922f6a25ae4f68dcd1bd1628/TLM/TLM/State/GlobalConfig.cs#L160-L172

@FireController1847 FireController1847 changed the title Improved path-find debugging Improved Pathfinder Debugging Jun 16, 2019
@originalfoo
Copy link
Member

Any updates on this? All the other stuff for 10.21 release is close to merge and this looks like it might be a straggler?

@FireController1847 FireController1847 modified the milestones: 10.21, 11.0 Jun 30, 2019
@originalfoo originalfoo added code cleanup Refactor code, remove old code, improve maintainability PATHFINDER Pathfinding tweaks or issues labels Aug 13, 2019
Copy link
Member

@originalfoo originalfoo left a comment

Choose a reason for hiding this comment

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

Just noticed this PR uses switch 26 for pathfinder stuff, however merged PR #513 is now using that switch for speed limits.

This PR #370 will need updating to use switch 27.

@kvakvs
Copy link
Collaborator

kvakvs commented Sep 5, 2019

This might be a great moment to give names to XML values and then the conflict will be gone.

@originalfoo originalfoo modified the milestones: 11.0, 11.1 Jan 24, 2020
@originalfoo originalfoo modified the milestones: 11.1, 11.2 Feb 7, 2020
@originalfoo originalfoo removed this from the 11.2 milestone Feb 26, 2020
@DaEgi01
Copy link
Contributor

DaEgi01 commented Apr 8, 2021

Closing this PR for now, since there was no progress for more than a year.
In case this is still relevant PR can be reopened.

@DaEgi01 DaEgi01 closed this Apr 8, 2021
@originalfoo originalfoo added this to the Abandoned milestone Jan 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code cleanup Refactor code, remove old code, improve maintainability LABS TM:PE LABS branch PATHFINDER Pathfinding tweaks or issues STABLE TM:PE STABLE branch technical Tasks that need to be performed in order to improve quality and maintainability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants