[GEN][ZH] Fix limit of 100 in Replay Menu

[helmutbuhler]

This PR lifts the limit of 100 replays in the Replay Menu.

This was originally part of #398

[roossienb]

Tested this solution in ZH. Beyond 706 replays, the scrollbar bugs out and does not want to scroll to the 707th item, instead it shows the first item again and the scrollbar positioner jumps back to the top position.

This fix would be temporary fix to increase the replay list size from 100 to a maximum of 706.
Could therefore also argue to initialize GadgetListBoxSetListLength once to 706.

[xezon]

Is this change currently very much needed for testing or can we hold off merging this?

[helmutbuhler]

@roossienb Thanks for testing! I can look into why it stops working after 706 replays, but it's very likely an independent issue. So I suggest we merge this first and I'll try to fix that in a separate PR.

@xezon Well this PR isn't directly needed for testing, but my changes in the Replay Menu are kind of piling up. I will likely need to change there something for the headless replay checking as well. And since it's already tested, merged with Gen and is a very simple change, why not merge it?

[xezon]

Because this is not the right time to merge small fixes when we are still building up the foundation. I understand we are eager to fix and improve all the things, but it will be cleaner if the bulk of the fixes will be applied to a unified Core. It will make reverting changes much easier for example. It will also make it easier for us to focus on making the right decisions for how to fix things. Right now, we need to concentrate on unifying, platform, architecture, stability and CRC testability and there is plenty to do in that regard.

[xezon]

This approach looks not ideal.

Problems:

• reallocates list exponentially as the loop progresses
• overallocates the list at last. 408 replays will allocate a list of 800

Is the replay menu rebuilt from scratch when reopening the replay menu?

The better implementation is to create the list with 0 initial elements, then get the number of replays, allocate the list for that amount, then populate it.

Calling readReplayHeader twice to generate the accurate list count is too expensive, but we can simply allocate the list for replayFilenames.size() which should be accurate enough.

With this approach we avoid allocating more than once and we avoid overallocating, and logically this is a lot cleaner.

[roossienb]

Is the replay menu rebuilt from scratch when reopening the replay menu?

Yes it is. It is how I increased/decreased the number of replay files. Very noticeable if you have 1.000 replay files, as it needs time to read the meta-data of all of them (takes about 3 - 5 seconds).

[jaapdeheer]

FYI I'm looking into implementing these suggestions. First fighting vscode vs2022, trying to get a build working again etc, so might take a while.

[jaapdeheer]

Ok afaiu the initial length of 100 is defined in ReplayMenu.wnd (that took me some digging 😅), which I assume we don't want to change for this since it's not part of the code. So then initially creating the listbox with 0 elements goes out the window.
So my plan now is to just resize it once, i.e. GadgetListBoxSetListLength(listbox, replayFilenames.size()) in PopulateReplayFileListbox(); that seems to work fine.

Will test a bit more and then push it here, probably Friday (busy day tomorrow).
@helmutbuhler do you mind if I rebase this PR on main first? (I did that locally because some build configuration stuff was changed on main in the meantime, and switching between old & new drove me crazy.)

BTW,

Is the replay menu rebuilt from scratch when reopening the replay menu?

I mean, kind of, but I think the listbox itself (the thing we're resizing to >100) persists between menu reopenings: PopulateReplayFileListbox(listbox) starts off by calling GadgetListBoxReset(listbox), and PopulateReplayFileListbox's caller ReplayMenuInit() fetches the listbox as listboxReplayFiles = TheWindowManager->winGetWindowFromId(...). So I assume the adjusted listbox size will persist, until the next time the menu is loaded, when it will just be resized again. Doesn't seem like a problem to me, but then again I know nothing 🙃

[xezon]

If the initial size is already controllable by ReplayMenu.wnd, why do we need to change the behaviour in code then?

[xezon]

You can create local INI edits to accommodate code. The code is not meant to be a playground, unless that code is put behind debug macros.

[helmutbuhler]

Playground? Seriously? This code is fine, it's already tested and works as advertised. It's no harm to the user, even if we forget about it and it remains.

I want this to be fixed for everyone who is using the repository. If everyone first needs to make local edits to INI files, it's pointless. There is also nothing unusual about fixing stuff in code until we figure out a way to fix and sync data files.

[jaapdeheer]

Just to be clear, unless I'm mistaken this cannot be fixed in data, simply because the list length is only known at run time. There is no length value we could set in the data that would accomodate all runtime situations, unless we set the length to something idiotically large, which would be very dirty imo.

It seems to me that this is simply a bug, impacting both users and developers, that can only be fixed in code.

And yes, ideally one day we'd drop the initial length of 100 from the data, and just set it in code. But that is just some bonus cleanup and an (imo very insignificant and thus irrelevant) optimization that we can do on top of this change, one day, if we really want to. For now, this change fixes the bug, in the right place, and is all we need.

My 2c 🙃

(Regardless of all this, I'll be pushing my changes here to implement @xezon's above suggestion, i.e. change the length once, instead of doubling the length until it fits. Nice practice material for me, and I do concur that it's slightly prettier, although you could argue about how important that is.)

[xezon]

Data setups needs to be fixed in our data branch, which is here:
https://github.com/TheSuperHackers/GeneralsGamePatch

The list box length is set here:
https://github.com/TheSuperHackers/GeneralsGamePatch/blob/b8f68f905e16fdaae2beda7dbea56ce753e1ab3c/Patch104pZH/GameFilesOriginalZH/Window/Menus/ReplayMenu.wnd#L190

I disagree with overriding INI settings by code. INI settings need to mean something. LENGTH: 100 says that the max length is 100. This should not be violated by code.

Options to change list lengths

1. Increase LENGTH number in WND file, for example LISTBOXDATA = LENGTH: 1000
2. Set LENGTH number in WND file to 0 and then allow the code to allocate its own limit
3. Let code allocate the list length and override INI length, but put that behind DEBUG or INTERNAL

I tested setting LENGTH to 0 and that works and will not show any replays. (Setting it to -1 will crash the client.)

[jaapdeheer]

Ooooh I thought GeneralsGamePatch was unrelated to this project and just the repo for the balance-incompatible 1.06 patch (or whatever it's called). But now I found TheSuperHackers/GeneralsGamePatch#2596; apparently I thought wrong :)

So okay I have no opinion on this until I get more acquainted with the plans. I'll ask around. In the meantime I guess I'll stop messing with this PR and see if I can help out with #651 when I have time.

As an aside, either I've just been daft for the last few weeks or we really need some toplevel documentation on the plans and how to contribute, to enable more devs to join. I've spent many hours trying to figure everything out and still the whole status/plan is in large parts a mystery to me. Not how to build but just what the idea is and which tasks need doing and why.
Even just a 10 line up-to-date CONTRIBUTING.md saying "our main priorities now are x and y; we want to keep 1.04 compatibility for now; we want to release after z; we will bundle this release with foo from GeneralsGamePatch; we'll need a launcher for that because bar" would have saved me much time and frustration.
I don't feel equipped yet to write this, but obviously I'd be happy to help if someone else wants to.