Skip to content

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

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e0fe203
Don't limit replay list to 100
helmutbuhler Mar 8, 2025
db63913
Add stupid TheSuperHackers @bugfix
helmutbuhler Apr 24, 2025
658e31d
Fix whitespace
helmutbuhler Apr 24, 2025
86a03e9
Port replay max 100 fix to Gen
helmutbuhler Apr 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions Generals/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/ReplayMenu.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,16 @@ void PopulateReplayFileListbox(GameWindow *listbox)
}

Int insertionIndex = GadgetListBoxAddEntryText(listbox, replayNameToShow, color, -1, 0);
if (insertionIndex == -1)
{
// TheSuperHackers @bugfix helmutbuhler 08/03/2025
// The list originally had a maximum length of 100. If we fail here we probably
// exceeded that and just double the max here and try again.
Int length = GadgetListBoxGetNumEntries(listbox);
GadgetListBoxSetListLength(listbox, length * 2);
Copy link

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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).

Copy link

@jaapdeheer jaapdeheer Apr 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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 🙃

Copy link

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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.)

Copy link

@xezon xezon May 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.)

Copy link

Choose a reason for hiding this comment

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

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.


insertionIndex = GadgetListBoxAddEntryText(listbox, replayNameToShow, color, -1, 0);
}
GadgetListBoxAddEntryText(listbox, displayTimeBuffer, color, insertionIndex, 1);
GadgetListBoxAddEntryText(listbox, header.versionString, color, insertionIndex, 2);
GadgetListBoxAddEntryText(listbox, mapStr, color, insertionIndex, 3);
Expand Down
10 changes: 10 additions & 0 deletions GeneralsMD/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/ReplayMenu.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,16 @@ void PopulateReplayFileListbox(GameWindow *listbox)
}

Int insertionIndex = GadgetListBoxAddEntryText(listbox, replayNameToShow, color, -1, 0);
if (insertionIndex == -1)
{
// TheSuperHackers @bugfix helmutbuhler 08/03/2025
// The list originally had a maximum length of 100. If we fail here we probably
// exceeded that and just double the max here and try again.
Int length = GadgetListBoxGetNumEntries(listbox);
GadgetListBoxSetListLength(listbox, length * 2);

insertionIndex = GadgetListBoxAddEntryText(listbox, replayNameToShow, color, -1, 0);
}
GadgetListBoxAddEntryText(listbox, displayTimeBuffer, color, insertionIndex, 1);
GadgetListBoxAddEntryText(listbox, header.versionString, color, insertionIndex, 2);
GadgetListBoxAddEntryText(listbox, mapStr, color, insertionIndex, 3);
Expand Down
Loading