Add level progress menu to main menu

[differentmatt]

Introduces a new 'PROGRESS' button in the main menu, allowing players to view their progress across accessible levels. The progress menu displays each level's status, title, and remaining enemies, and enables switching to any discovered but uncleared level. Supporting code includes new menu logic, button definitions, and a helper for gathering level statistics.

[Copilot]

Pull request overview

This PR adds a level progress menu to the main menu, enabling players to view and navigate their campaign progress. The feature displays accessible levels with their completion status, titles, and enemy counts, allowing players to switch to any discovered uncleared level.

Changes:

• Added a new "PROGRESS" button to the main menu (between BACK and SET LEVEL buttons)
• Implemented create_progress_menu() function with scrollable level list and level selection functionality
• Added helper function get_accessible_levels() to determine which levels the player can access
• Exposed getLevelStats() function in level_picker.h header for reuse

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
src/picker.cpp	Implemented progress menu UI, button navigation updates, and accessible level calculation logic
src/level_picker.h	Exported getLevelStats function for use in progress menu
src/button.h	Added CREATE_PROGRESS_MENU constant and function declaration
src/button.cpp	Added case handler for CREATE_PROGRESS_MENU in button dispatch

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Memory leak: A new std::list is allocated but never freed. Pass a temporary list object on the stack instead: std::list<int> temp_exits; getLevelStats(ld, NULL, NULL, &num_enemies, NULL, temp_exits);

[Copilot]

Buffer overflow risk: The title array is declared as char[24] at line 1106, but strncpy copies 20 characters and sets position 20 to null terminator. If the string is exactly 20 characters, this is correct, but the subsequent code (lines 1195-1199) checks strlen and modifies indices 17-19, which requires at least 21 bytes for proper null termination. Either copy 23 characters to match BrowserEntry pattern at level_picker.cpp:201, or reduce the array size to match the copy length.

[Copilot]

The BACK button width was changed from 60 to 50 pixels without explanation. This change appears unnecessary since the new PROGRESS button has its own space allocation. If this resize is intentional to accommodate the PROGRESS button layout, consider adding a comment explaining the UI layout constraints.

Suggested change

	// NOTE: BACK button width is intentionally 50px (instead of 60px) to fit alongside
	// the PROGRESS / SET LEVEL / SET CAMPAIGN buttons within the available horizontal space.

[claude]

PR Review: Add level progress menu to main menu

Overview

This PR adds a new "PROGRESS" button to the main menu that allows players to view their progress across accessible levels. The implementation includes menu logic, button definitions, and level statistics gathering.

Code Quality & Best Practices

Positive Aspects

✅ Good separation of concerns: The getLevelStats function is properly declared in the header and reused
✅ Consistent code style: Follows the existing codebase patterns
✅ Clear UI structure: The progress menu has well-organized columns and status indicators

Issues & Suggestions

1. Memory Leak ⚠️ CRITICAL

// Line 1202 in picker.cpp
getLevelStats(ld, NULL, NULL, &num_enemies, NULL, *(new std::list<int>()));

This creates a temporary std::list<int> on the heap that is never deleted. Should be:

std::list<int> exits;
getLevelStats(ld, NULL, NULL, &num_enemies, NULL, exits);

2. Inconsistent Function Signature ⚠️

The getLevelStats function signature in level_picker.h takes a reference for exits:

void getLevelStats(LevelData& level_data, int* max_enemy_level, float* average_enemy_level,
                   int* num_enemies, float* difficulty, std::list<int>& exits);

But the call in line 1202 tries to pass a temporary. This may compile due to C++ binding rules for temporaries, but it's semantically incorrect and wastes the collected exit data.

3. Buffer Overflow Risk ⚠️

// Line 1193
strncpy(lp.title, ld.title.c_str(), 20);
lp.title[20] = '\0';

The LevelProgress::title field is declared as char title[24], but only 20 characters are copied, then index 20 is set to null. This is correct, but the comment suggests it should handle up to 20 chars + ellipsis (3 chars) = 23 chars total. The logic that follows (lines 1194-1198) correctly handles this, but the initial strncpy should use 21 to be clearer:

strncpy(lp.title, ld.title.c_str(), 23);
lp.title[23] = '\0';
if (strlen(ld.title.c_str()) > 23) {
    lp.title[20] = '.';
    lp.title[21] = '.';
    lp.title[22] = '.';
}

4. Inefficient Level Discovery Algorithm

The get_accessible_levels() function at line 1112 has potential performance issues:

• It processes cleared levels but only adds their exits, not recursively discovering new levels through those exits
• The while (!to_process.empty()) loop only processes cleared levels from to_process, but never adds newly discovered levels back to to_process
• This means levels accessible through 2+ hops from cleared levels won't be discovered

The logic should be:

while (!to_process.empty()) {
    int level_id = *to_process.begin();
    to_process.erase(to_process.begin());

    if (myscreen->save_data.is_level_completed(level_id)) {
        LevelData ld(level_id);
        if (ld.load()) {
            std::list<int> exits;
            getLevelStats(ld, NULL, NULL, NULL, NULL, exits);
            for (int exit_id : exits) {
                if (accessible.find(exit_id) == accessible.end()) {
                    accessible.insert(exit_id);
                    to_process.insert(exit_id);  // Add this line to process discovered levels
                }
            }
        }
    }
}

5. Code Duplication

The button bounds checking code (lines 1245-1248 and 1252-1255) is duplicated. Consider extracting to a helper function.

Potential Bugs

1. Incorrect Button Index Updates

In createmenu_buttons array (lines 346-354), the navigation indices are updated, but there's a potential issue:

• Button 5 ("GO") now has MenuNav::UpDownLeft(2, 8, 4) which references button 8 as down
• Button 8 is "SET LEVEL", but the GO button should probably go down to button 6 ("BACK") or 7 ("PROGRESS")

2. Missing Null Pointer Checks

The code assumes myscreen is always valid but doesn't check before dereferencing in multiple places. This is consistent with the rest of the codebase, but worth noting.

Performance Considerations

1. Repeated Level Loading

Each accessible level is loaded via LevelData::load() which likely involves file I/O. For campaigns with many levels, this could cause noticeable lag when opening the progress menu.

Suggestion: Consider caching level metadata (title, enemy count) or loading it asynchronously.

2. Inefficient Set Operations

The get_accessible_levels() function uses std::set::find() in a loop. While O(log n), consider using std::unordered_set for O(1) lookups if the level count is large.

3. String Formatting in Tight Loop

Lines 1296-1299 use snprintf in the render loop. While not critical, consider pre-formatting strings during level info loading.

Security Concerns

1. No Input Validation on Level IDs

When switching levels via the GO button (line 1278), the code sets myscreen->save_data.scen_num = lp.id without validating that lp.id is a valid, loadable level. If the level data is corrupted or maliciously crafted, this could cause issues.

2. Buffer Operations

While the strncpy usage is mostly safe, consider using safer alternatives like snprintf for all string operations.

Test Coverage

❌ No tests included: This PR doesn't include any automated tests.

Recommended Test Cases:

1. Unit tests for get_accessible_levels():

   • Empty campaign (only level 1 accessible)
   • Linear campaign (1→2→3→...)
   • Branching campaign (1→{2,3})
   • Circular/re-converging paths (1→2→3, 1→4→3)

2. Integration tests for progress menu:

   • Display with 0 cleared levels
   • Display with all levels cleared
   • Scrolling behavior with 10+ levels
   • GO button functionality
   • Navigation with keyboard/gamepad

3. Edge cases:

   • Level with missing/corrupted title
   • Level files that fail to load
   • Very long level titles (24+ chars)
   • Campaigns with 100+ levels

Summary

This is a useful feature that enhances gameplay by giving players visibility into their progress. However, there are several issues that should be addressed:

Must Fix:

• Memory leak on line 1202
• Level discovery algorithm doesn't properly traverse multi-hop exits

Should Fix:

• Buffer overflow risk (use full buffer size)
• Button navigation indices may be incorrect
• Add null checks for loaded level data

Consider:

• Performance optimization for loading many levels
• Add automated tests
• Cache level metadata

Estimated Risk: Medium - The memory leak and discovery algorithm issues could impact user experience.

Let me know if you'd like me to provide code snippets for any of these fixes!