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

Added option to change the Maximum Autosave turns stored #12790

Open
wants to merge 27 commits into
base: master
Choose a base branch
from

Conversation

Emandac
Copy link
Contributor

@Emandac Emandac commented Jan 11, 2025

Added a drop down box to change the Maximum Autosave turns stored.

@SomeTroglodyte
Copy link
Collaborator

The failing check is not your fault (39ebaad).

@SomeTroglodyte
Copy link
Collaborator

Failing checks: Use the "Details" link - you see it's a matter of a deprecation in the vanilla rulesets.

That commit history is far too long for the limited scope of the PR (small scope is good) - that will disappear once merged, but if you are curious on how to avoid that: Work with a local clone, use branches, rebase each branch onto master until ready for first push, only then start using merge (and avoid rebase like the plague once it's a PR, though deleting the online copy of a branch keeping local then rebasing is OK). There's some texts on my "Scratchpad" repo that may help, even if ages old.

Other than that, the diff looks well done on a glance. The offered values seem a bit jumpy. As alternative to a selectbox, one could use an UncivSlider, either mapping abstract stops to non-linear output values (lots more code) or using setSnapToValues - More code as well, but allowing arbitrary values by holding shift... Nah, dropdown is fine and easier.

@SomeTroglodyte
Copy link
Collaborator

Oh, and as an interesting aside: The way you initialize that Array is fine, but begs the question "why is there no simple literal notation"?
a) It's a Gdx Array not a Java Array or a kotlin Array - I don't like the name shadowing and have used import as GdxArray in the past, though the direct import was already present in that file
b) The oneliners that do work aren't really prettier:
* Array(intArrayOf(1, 2, 3).toTypedArray())
* intArrayOf(1, 2, 3).asIterable().toGdxArray()

@Emandac
Copy link
Contributor Author

Emandac commented Jan 11, 2025

So i should change it to intArrayOf(1, 2, 5, 10, 15, etc..).asIterable().toGdxArray() ?

@SomeTroglodyte
Copy link
Collaborator

SomeTroglodyte commented Jan 11, 2025

So i should change it to intArrayOf(1, 2, 5, 10, 15, etc..).asIterable().toGdxArray() ?

No! My input is only ever a nudge to think for yourself. And in this case I said "aren't really prettier". If you really think one of them is prettier, go ahead. I don't.

Oh - and look up what happens: The one with toTypedArray goes to a different constructor within Gdx's Array, and re-copies the values twice; while toGdxArray is an extension in Unciv and re-copies at least once. The one with the least copying and memory allocations may well happen to be yours - though I'm rusty and might not guess what the compiler would output correctly.

@@ -486,7 +489,8 @@ class Autosaves(val files: UncivFiles) {
fun getAutosaves(): Sequence<FileHandle> {
return files.getSaves().filter { it.name().startsWith(AUTOSAVE_FILE_NAME) }
}
while (getAutosaves().count() > 10) {
// added the plus 1 to avoid player choosing 6,11,21,51,101, etc.. in options.
Copy link
Owner

Choose a reason for hiding this comment

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

I don't get this comment - with this config, if you say "I want 10 files" what you'll actually get is 11 files, why is that better?

Copy link
Contributor Author

@Emandac Emandac Jan 11, 2025

Choose a reason for hiding this comment

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

with the old version with 10 has example, it would start overriding after 9 instead of 10.
like from autosave-1 to autosave-9 after the autosave-9 the autosave-1 would override to autosave-2.
For me it should be after autosave-10 that it should start overriding old autosaves.

Copy link
Owner

Choose a reason for hiding this comment

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

OHH I KNOW
It's because the numberless "autosave" also gets counted...
So add that into the comment please :)

@@ -51,6 +51,8 @@ class AdvancedTab(
init {
pad(10f)
defaults().pad(5f)

addmaxAutosavesStored()
Copy link
Owner

Choose a reason for hiding this comment

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

Capitalize Max



private fun addmaxAutosavesStored() {
add("Maximum autosave turns stored".toLabel()).left().fillX()
Copy link
Owner

Choose a reason for hiding this comment

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

"autosave turns" makes it seem like this is dependent on the game turns, when actually it's dependent on the number of files
A better phrasing would be "Number of autosaves stored" or perhaps "Number of autosave files stored"

Copy link
Owner

@yairm210 yairm210 left a comment

Choose a reason for hiding this comment

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

Looks good :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants