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

Core: log fill progress #2382

Merged
merged 5 commits into from
Oct 30, 2023
Merged

Core: log fill progress #2382

merged 5 commits into from
Oct 30, 2023

Conversation

Berserker66
Copy link
Member

What is this fixing or adding?

Some alive signals during a long fill step

How was this tested?

with 300 Factorio worlds

If this makes graphical changes, please attach screenshots.

image

@black-sliver
Copy link
Member

Filling the world with 52800 items.

Current fill step at x/16134 items placed.

I feel like this is confusing if you don't know that there are multiple fills gonna happen.
So we should either have a message for each fill step (where the sum always ends up being the total item count), or we should hide it by default (verbose?).

@Berserker66
Copy link
Member Author

I was hoping "current fill step" would indicate there are multiple. Printing for each step is quite verbose, as we have many. I don't like hiding it either. I could add another for remaining_fill, which would almost always at least do it twice, hence making it more obvious there's multiple.

@ScootyPuffJr1 ScootyPuffJr1 added is: refactor/cleanup Improvements to code/output readability or organizization. affects: core Issues/PRs that touch core and may need additional validation. labels Oct 27, 2023
@black-sliver
Copy link
Member

So, my two suggestions are either
put it behind level = verbose or
have it announce what it's filling.

@Berserker66
Copy link
Member Author

image
done

Fill.py Outdated Show resolved Hide resolved
Fill.py Outdated Show resolved Hide resolved
Fill.py Outdated Show resolved Hide resolved
Fill.py Outdated Show resolved Hide resolved
worlds/alttp/Dungeons.py Outdated Show resolved Hide resolved
@black-sliver
Copy link
Member

I agree with the first 2 suggestions from el-u. For 3-4, I am not sure how it's formatted elsewhere, but preferably it should be consistent. For 5, I think the current naming is as good as the suggested one, as it fills (some) dungeon items into (some) dungeons.

Lgtm otherwise.

@Berserker66 Berserker66 merged commit f81e726 into main Oct 30, 2023
21 checks passed
@Berserker66 Berserker66 deleted the core_log_fill_progress branch October 30, 2023 00:22
FlySniper pushed a commit to FlySniper/Archipelago that referenced this pull request Nov 14, 2023
* Core: log fill progress

* Add names to common fill steps

* Update Fill.py

Co-authored-by: el-u <[email protected]>

* Apply suggestions from code review

Co-authored-by: el-u <[email protected]>

* cleanup default name

---------

Co-authored-by: el-u <[email protected]>
Jouramie pushed a commit to Jouramie/Archipelago that referenced this pull request Feb 28, 2024
* Core: log fill progress

* Add names to common fill steps

* Update Fill.py

Co-authored-by: el-u <[email protected]>

* Apply suggestions from code review

Co-authored-by: el-u <[email protected]>

* cleanup default name

---------

Co-authored-by: el-u <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: core Issues/PRs that touch core and may need additional validation. is: refactor/cleanup Improvements to code/output readability or organizization.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants