Archipelago Randomizer Support - #1433
Conversation
Refactored a bunch to remove footprint in the omf c code. New ap_alert module — extracted alert/notification display from arena New ap_sgmanager module — AP-aware save game manager extracted from core sgmanager New ap_melee module — character select AP logic extracted Major refactor of ap_mechlab and ap_arena Mechlab trade/customize/training menus improved Windows build support added
# Conflicts: # src/game/scenes/arena.c # src/game/scenes/mechlab.c
# Conflicts: # src/game/scenes/mechlab/lab_menu_customize.c
Fix tournament-goal persistence and stale forfeit scoring ap_on_tournament_win now persists APSave to disk immediately after setting the tournament-won bit, instead of leaving it to the next autosave — mechlab reloads APSave from disk on entry, so the just-set bit could get clobbered by a stale on-disk copy first. Also force fight_stats.winner to a real value on forfeit so arena_end()'s AP match-check can't read a leftover value from the previous match. Split AP-only tests into a separate openomf_test_ap binary (testing/test_ap_*.c convention) since they link Archipelago-only symbols, and add test_ap_tournament_goal.c covering the fix above.
| #if ARCHIPELAGO_ENABLED | ||
| settings_archipelago archipelago; | ||
| #endif |
There was a problem hiding this comment.
Do not use compile-time guards in settings.c/.h, as binaries built without ARCHIPELAGO_ENABLED will be unaware of these settings & silently remove them from the .conf file.
If we want binaries built without ARCHIPELAGO_ENABLED to be entirely archipelago-unaware, then its settings will have to go in a separate archipelago.conf file.
There was a problem hiding this comment.
Where are the files under resources/archipelago from?
Are they under a compatible license (e.g. MIT or CC0), or are they your own work?
There was a problem hiding this comment.
I don't like the prospect of adding submodules for dependencies, especially conditionally-enabled ones such as these.
If someone wants to check the various package managers we're using for our other deps (apt, brew, vcpkg) and see if these packages are all available (or, in the case of vcpkg, easily added via our vcpkg-ports folder), we could start looking at moving these deps into Dependencies.cmake.
| enable_language(CXX) | ||
| set(CMAKE_CXX_STANDARD 17) | ||
| set(CMAKE_CXX_STANDARD_REQUIRED ON) |
There was a problem hiding this comment.
This seems unnecessary.
| enable_language(CXX) | |
| set(CMAKE_CXX_STANDARD 17) | |
| set(CMAKE_CXX_STANDARD_REQUIRED ON) |
There was a problem hiding this comment.
These Dockerfiles seem like build infrastructure that is entirely independent of the rest of the PR.
If you'd like to introduce these, I would suggest creating a separate PR for them.
Nopey
left a comment
Nopey
left a comment
There was a problem hiding this comment.
I haven't read the rest of the .c changes, but these changes in textinput.c stood out to me.
| str tmp; | ||
| str_from_c(&tmp, " "); | ||
| text_set_from_str(ti->text, &tmp); | ||
| str_free(&tmp); |
There was a problem hiding this comment.
You don't need to construct a str here.
| str tmp; | |
| str_from_c(&tmp, " "); | |
| text_set_from_str(ti->text, &tmp); | |
| str_free(&tmp); | |
| text_set_from_c(ti->text, " "); |
There was a problem hiding this comment.
Could you explain or motivate the changes to this file?
|
feel free to undraft when this is ready for review 👍 |
3 participants
No description provided.