Skip to content

Conversation

Atakku
Copy link

@Atakku Atakku commented Jul 27, 2025

About the PR

Drastically reduced the amount of system.dungeon log spam
Instead of ~1k lines of log spam, it now prints the line, the ore vein in question, and the amount of times it had the issue

Why / Balance

It was sometimes severely bloating console logs, therefore I decided to fix it

Technical details

Made the log count the amount of groups with remaining group size and print at the end of the function, instead of printing every single time there was a remaining group size, reducing the total amount of lines printed by this function from ~5-10k to 5-10 per round
Screenshot 2025-07-27 183059

Media

Requirements

Breaking changes

Changelog

@PJBot PJBot added S: Needs Review Status: Requires additional reviews before being fully accepted. Not to be replaced by S: Approved. S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. labels Jul 27, 2025
@github-actions github-actions bot added the size/S Denotes a PR that changes 10-99 lines. label Jul 27, 2025
@perryprog
Copy link
Contributor

Fixes #39055.

@perryprog perryprog added T: Bugfix Type: Bugs and/or bugfixes P2: Raised Priority: Item has a raised priority, indicating it might get increased maintainer attention. D3: Low Difficulty: Some codebase knowledge required. A: Core Tech Area: Underlying core tech for the game and the Github repository. and removed S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. labels Jul 27, 2025
@github-actions github-actions bot added size/XS Denotes a PR that changes 0-9 lines. and removed size/S Denotes a PR that changes 10-99 lines. labels Jul 28, 2025
@github-actions github-actions bot added size/S Denotes a PR that changes 10-99 lines. and removed size/XS Denotes a PR that changes 0-9 lines. labels Jul 28, 2025
@slarticodefast
Copy link
Member

Does this fix the bug or just combine the error messages into one?

@Atakku
Copy link
Author

Atakku commented Jul 28, 2025

Does this fix the bug or just combine the error messages into one?

It merges the errors into one + count

@slarticodefast
Copy link
Member

I guess that saves us some disk space for all the error logs, but I would have rather have the bug itself fixed so that we get no errors at all 😄

@slarticodefast slarticodefast added the S: Concern A maintainer has raised concerns regarding the idea. The PR may be closed by another maintainer. label Jul 28, 2025
@perryprog
Copy link
Contributor

I feel like considering it's procgen code, that's (potentially) not exactly trivial since it's not exactly clear what the issue is since ore generation seems to be behaving fine. Considering how brutally this spams logs this feels like a fine solution as long as we have a tracking issue that this still needs properly fixed.

@SlamBamActionman
Copy link
Member

We'd rather see the underlying error fixed than reducing the errors that happen; while it's nice to have them collected in the log it also risks hiding the prevalence of the underlying error. There are also concerns about allocating dictionaries for this purpose.
Because of this, we will be closing the PR. Thank you for your contribution!

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

Labels

A: Core Tech Area: Underlying core tech for the game and the Github repository. D3: Low Difficulty: Some codebase knowledge required. P2: Raised Priority: Item has a raised priority, indicating it might get increased maintainer attention. S: Concern A maintainer has raised concerns regarding the idea. The PR may be closed by another maintainer. S: Needs Review Status: Requires additional reviews before being fully accepted. Not to be replaced by S: Approved. size/S Denotes a PR that changes 10-99 lines. T: Bugfix Type: Bugs and/or bugfixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants