From e755f1a0b5b588f51b26e8ac9eddd69fba044567 Mon Sep 17 00:00:00 2001 From: Fabian Dill Date: Wed, 12 Jun 2024 02:14:30 +0200 Subject: [PATCH 01/18] SC2: don't close all SC2 instances when one quits (#3507) --- worlds/_sc2common/bot/sc2process.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/worlds/_sc2common/bot/sc2process.py b/worlds/_sc2common/bot/sc2process.py index e366321659..f74ed9c18f 100644 --- a/worlds/_sc2common/bot/sc2process.py +++ b/worlds/_sc2common/bot/sc2process.py @@ -28,6 +28,11 @@ class kill_switch: logger.debug("kill_switch: Add switch") cls._to_kill.append(value) + @classmethod + def kill(cls, value): + logger.info(f"kill_switch: Process cleanup for 1 process") + value._clean(verbose=False) + @classmethod def kill_all(cls): logger.info(f"kill_switch: Process cleanup for {len(cls._to_kill)} processes") @@ -116,7 +121,7 @@ class SC2Process: async def __aexit__(self, *args): logger.exception("async exit") await self._close_connection() - kill_switch.kill_all() + kill_switch.kill(self) signal.signal(signal.SIGINT, signal.SIG_DFL) @property From 7299891bdf86d5134692ae55fac92ad32ba17059 Mon Sep 17 00:00:00 2001 From: Natalie Weizenbaum Date: Tue, 11 Jun 2024 18:22:14 -0700 Subject: [PATCH 02/18] Allow worlds to add options to prebuilt groups (#3509) Previously, this crashed because `typing.NamedTuple` fields such as `group.name` aren't assignable. Now it will only fail for group names that are actually incorrectly cased, and will fail with a better error message. --- worlds/AutoWorld.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/worlds/AutoWorld.py b/worlds/AutoWorld.py index 6e17f023f6..bed375cf08 100644 --- a/worlds/AutoWorld.py +++ b/worlds/AutoWorld.py @@ -123,8 +123,8 @@ class WebWorldRegister(type): assert group.options, "A custom defined Option Group must contain at least one Option." # catch incorrectly titled versions of the prebuilt groups so they don't create extra groups title_name = group.name.title() - if title_name in prebuilt_options: - group.name = title_name + assert title_name not in prebuilt_options or title_name == group.name, \ + f"Prebuilt group name \"{group.name}\" must be \"{title_name}\"" if group.name == "Item & Location Options": assert not any(option in item_and_loc_options for option in group.options), \ From b9e454ab4ec7903a66e71324805f93b0332bc286 Mon Sep 17 00:00:00 2001 From: Silvris <58583688+Silvris@users.noreply.github.com> Date: Tue, 11 Jun 2024 20:23:46 -0500 Subject: [PATCH 03/18] TS: add indirect connections (#3490) --- worlds/timespinner/Regions.py | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/worlds/timespinner/Regions.py b/worlds/timespinner/Regions.py index 4f53f75eff..757a41c388 100644 --- a/worlds/timespinner/Regions.py +++ b/worlds/timespinner/Regions.py @@ -70,7 +70,7 @@ def create_regions_and_locations(world: MultiWorld, player: int, precalculated_w logic = TimespinnerLogic(world, player, precalculated_weights) connect(world, player, 'Lake desolation', 'Lower lake desolation', lambda state: flooded.flood_lake_desolation or logic.has_timestop(state) or state.has('Talaria Attachment', player)) - connect(world, player, 'Lake desolation', 'Upper lake desolation', lambda state: logic.has_fire(state) and state.can_reach('Upper Lake Serene', 'Region', player)) + connect(world, player, 'Lake desolation', 'Upper lake desolation', lambda state: logic.has_fire(state) and state.can_reach('Upper Lake Serene', 'Region', player), "Upper Lake Serene") connect(world, player, 'Lake desolation', 'Skeleton Shaft', lambda state: flooded.flood_lake_desolation or logic.has_doublejump(state)) connect(world, player, 'Lake desolation', 'Space time continuum', logic.has_teleport) connect(world, player, 'Upper lake desolation', 'Lake desolation') @@ -80,7 +80,7 @@ def create_regions_and_locations(world: MultiWorld, player: int, precalculated_w connect(world, player, 'Eastern lake desolation', 'Space time continuum', logic.has_teleport) connect(world, player, 'Eastern lake desolation', 'Library') connect(world, player, 'Eastern lake desolation', 'Lower lake desolation') - connect(world, player, 'Eastern lake desolation', 'Upper lake desolation', lambda state: logic.has_fire(state) and state.can_reach('Upper Lake Serene', 'Region', player)) + connect(world, player, 'Eastern lake desolation', 'Upper lake desolation', lambda state: logic.has_fire(state) and state.can_reach('Upper Lake Serene', 'Region', player), "Upper Lake Serene") connect(world, player, 'Library', 'Eastern lake desolation') connect(world, player, 'Library', 'Library top', lambda state: logic.has_doublejump(state) or state.has('Talaria Attachment', player)) connect(world, player, 'Library', 'Varndagroth tower left', logic.has_keycard_D) @@ -185,7 +185,7 @@ def create_regions_and_locations(world: MultiWorld, player: int, precalculated_w if is_option_enabled(world, player, "GyreArchives"): connect(world, player, 'The lab (upper)', 'Ravenlord\'s Lair', lambda state: state.has('Merchant Crow', player)) connect(world, player, 'Ravenlord\'s Lair', 'The lab (upper)') - connect(world, player, 'Library top', 'Ifrit\'s Lair', lambda state: state.has('Kobo', player) and state.can_reach('Refugee Camp', 'Region', player)) + connect(world, player, 'Library top', 'Ifrit\'s Lair', lambda state: state.has('Kobo', player) and state.can_reach('Refugee Camp', 'Region', player), "Refugee Camp") connect(world, player, 'Ifrit\'s Lair', 'Library top') @@ -242,11 +242,19 @@ def connectStartingRegion(world: MultiWorld, player: int): def connect(world: MultiWorld, player: int, source: str, target: str, - rule: Optional[Callable[[CollectionState], bool]] = None): + rule: Optional[Callable[[CollectionState], bool]] = None, + indirect: str = ""): sourceRegion = world.get_region(source, player) targetRegion = world.get_region(target, player) - sourceRegion.connect(targetRegion, rule=rule) + entrance = sourceRegion.connect(targetRegion, rule=rule) + + if indirect: + indirectRegion = world.get_region(indirect, player) + if indirectRegion in world.indirect_connections: + world.indirect_connections[indirectRegion].add(entrance) + else: + world.indirect_connections[indirectRegion] = {entrance} def split_location_datas_per_region(locations: List[LocationData]) -> Dict[str, List[LocationData]]: From 3b9b9353b7e7588cb59c496fd295397dcbcdf9b6 Mon Sep 17 00:00:00 2001 From: Fabian Dill Date: Wed, 12 Jun 2024 15:34:46 +0200 Subject: [PATCH 04/18] WebHost: delete old docs files (#3503) --- WebHost.py | 1 + 1 file changed, 1 insertion(+) diff --git a/WebHost.py b/WebHost.py index afacd6288e..08ef3c4307 100644 --- a/WebHost.py +++ b/WebHost.py @@ -58,6 +58,7 @@ def create_ordered_tutorials_file() -> typing.List[typing.Dict[str, typing.Any]] worlds[game] = world base_target_path = Utils.local_path("WebHostLib", "static", "generated", "docs") + shutil.rmtree(base_target_path, ignore_errors=True) for game, world in worlds.items(): # copy files from world's docs folder to the generated folder target_path = os.path.join(base_target_path, game) From 2daccded365258633bd8ac268c5a58ae9ee31a43 Mon Sep 17 00:00:00 2001 From: Fabian Dill Date: Wed, 12 Jun 2024 15:35:51 +0200 Subject: [PATCH 05/18] Core: don't lock progression (#3501) --- Fill.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Fill.py b/Fill.py index d8147b2eac..4967ff0736 100644 --- a/Fill.py +++ b/Fill.py @@ -483,15 +483,15 @@ def distribute_items_restrictive(multiworld: MultiWorld, if panic_method == "swap": fill_restrictive(multiworld, multiworld.state, defaultlocations, progitempool, swap=True, - on_place=mark_for_locking, name="Progression", single_player_placement=multiworld.players == 1) + name="Progression", single_player_placement=multiworld.players == 1) elif panic_method == "raise": fill_restrictive(multiworld, multiworld.state, defaultlocations, progitempool, swap=False, - on_place=mark_for_locking, name="Progression", single_player_placement=multiworld.players == 1) + name="Progression", single_player_placement=multiworld.players == 1) elif panic_method == "start_inventory": fill_restrictive(multiworld, multiworld.state, defaultlocations, progitempool, swap=False, allow_partial=True, - on_place=mark_for_locking, name="Progression", single_player_placement=multiworld.players == 1) + name="Progression", single_player_placement=multiworld.players == 1) if progitempool: for item in progitempool: logging.debug(f"Moved {item} to start_inventory to prevent fill failure.") From acf85eb9abb3de4863fe9350a6cdddee71c9be44 Mon Sep 17 00:00:00 2001 From: black-sliver <59490463+black-sliver@users.noreply.github.com> Date: Wed, 12 Jun 2024 18:54:59 +0200 Subject: [PATCH 06/18] Speedups: remove dependency on c++ (#2796) * Speedups: remove dependency on c++ * Speedups: intset: handle malloc failing * Speedups: intset: fix corner case for int64 on 32bit systems original idea was to only use bucket->val if int(-1) # this is all 0xff... adding 1 results in 0, but it's not negative +# configure INTSET for player +cdef extern from *: + """ + #define INTSET_NAME ap_player_set + #define INTSET_TYPE uint32_t // has to match ap_player_t + """ + +# create INTSET for player +cdef extern from "intset.h": + """ + #undef INTSET_NAME + #undef INTSET_TYPE + """ + ctypedef struct ap_player_set: + pass + + ap_player_set* ap_player_set_new(size_t bucket_count) nogil + void ap_player_set_free(ap_player_set* set) nogil + bint ap_player_set_add(ap_player_set* set, ap_player_t val) nogil + bint ap_player_set_contains(ap_player_set* set, ap_player_t val) nogil + cdef struct LocationEntry: # layout is so that @@ -185,7 +206,7 @@ cdef class LocationStore: def find_item(self, slots: Set[int], seeked_item_id: int) -> Generator[Tuple[int, int, int, int, int], None, None]: cdef ap_id_t item = seeked_item_id cdef ap_player_t receiver - cdef std_set[ap_player_t] receivers + cdef ap_player_set* receivers cdef size_t slot_count = len(slots) if slot_count == 1: # specialized implementation for single slot @@ -197,13 +218,20 @@ cdef class LocationStore: yield entry.sender, entry.location, entry.item, entry.receiver, entry.flags elif slot_count: # generic implementation with lookup in set - for receiver in slots: - receivers.insert(receiver) - with nogil: - for entry in self.entries[:self.entry_count]: - if entry.item == item and receivers.count(entry.receiver): - with gil: - yield entry.sender, entry.location, entry.item, entry.receiver, entry.flags + receivers = ap_player_set_new(min(1023, slot_count)) # limit top level struct to 16KB + if not receivers: + raise MemoryError() + try: + for receiver in slots: + if not ap_player_set_add(receivers, receiver): + raise MemoryError() + with nogil: + for entry in self.entries[:self.entry_count]: + if entry.item == item and ap_player_set_contains(receivers, entry.receiver): + with gil: + yield entry.sender, entry.location, entry.item, entry.receiver, entry.flags + finally: + ap_player_set_free(receivers) def get_for_player(self, slot: int) -> Dict[int, Set[int]]: cdef ap_player_t receiver = slot diff --git a/_speedups.pyxbld b/_speedups.pyxbld index e1fe19b2ef..974eaed03b 100644 --- a/_speedups.pyxbld +++ b/_speedups.pyxbld @@ -1,8 +1,10 @@ -# This file is required to get pyximport to work with C++. -# Switching from std::set to a pure C implementation is still on the table to simplify everything. +# This file is used when doing pyximport +import os def make_ext(modname, pyxfilename): from distutils.extension import Extension return Extension(name=modname, sources=[pyxfilename], - language='c++') + depends=["intset.h"], + include_dirs=[os.getcwd()], + language="c") diff --git a/intset.h b/intset.h new file mode 100644 index 0000000000..fac84fb6f8 --- /dev/null +++ b/intset.h @@ -0,0 +1,135 @@ +/* A specialized unordered_set implementation for literals, where bucket_count + * is defined at initialization rather than increased automatically. + */ +#include +#include +#include +#include + +#ifndef INTSET_NAME +#error "Please #define INTSET_NAME ... before including intset.h" +#endif + +#ifndef INTSET_TYPE +#error "Please #define INTSET_TYPE ... before including intset.h" +#endif + +/* macros to generate unique names from INTSET_NAME */ +#ifndef INTSET_CONCAT +#define INTSET_CONCAT_(a, b) a ## b +#define INTSET_CONCAT(a, b) INTSET_CONCAT_(a, b) +#define INTSET_FUNC_(a, b) INTSET_CONCAT(a, _ ## b) +#endif + +#define INTSET_FUNC(name) INTSET_FUNC_(INTSET_NAME, name) +#define INTSET_BUCKET INTSET_CONCAT(INTSET_NAME, Bucket) +#define INTSET_UNION INTSET_CONCAT(INTSET_NAME, Union) + +#if defined(_MSC_VER) +#pragma warning(push) +#pragma warning(disable : 4200) +#endif + + +typedef struct { + size_t count; + union INTSET_UNION { + INTSET_TYPE val; + INTSET_TYPE *data; + } v; +} INTSET_BUCKET; + +typedef struct { + size_t bucket_count; + INTSET_BUCKET buckets[]; +} INTSET_NAME; + +static INTSET_NAME *INTSET_FUNC(new)(size_t buckets) +{ + size_t i, size; + INTSET_NAME *set; + + if (buckets < 1) + buckets = 1; + if ((SIZE_MAX - sizeof(INTSET_NAME)) / sizeof(INTSET_BUCKET) < buckets) + return NULL; + size = sizeof(INTSET_NAME) + buckets * sizeof(INTSET_BUCKET); + set = (INTSET_NAME*)malloc(size); + if (!set) + return NULL; + memset(set, 0, size); /* gcc -fanalyzer does not understand this sets all buckets' count to 0 */ + for (i = 0; i < buckets; i++) { + set->buckets[i].count = 0; + } + set->bucket_count = buckets; + return set; +} + +static void INTSET_FUNC(free)(INTSET_NAME *set) +{ + size_t i; + if (!set) + return; + for (i = 0; i < set->bucket_count; i++) { + if (set->buckets[i].count > 1) + free(set->buckets[i].v.data); + } + free(set); +} + +static bool INTSET_FUNC(contains)(INTSET_NAME *set, INTSET_TYPE val) +{ + size_t i; + INTSET_BUCKET* bucket = &set->buckets[(size_t)val % set->bucket_count]; + if (bucket->count == 1) + return bucket->v.val == val; + for (i = 0; i < bucket->count; ++i) { + if (bucket->v.data[i] == val) + return true; + } + return false; +} + +static bool INTSET_FUNC(add)(INTSET_NAME *set, INTSET_TYPE val) +{ + INTSET_BUCKET* bucket; + + if (INTSET_FUNC(contains)(set, val)) + return true; /* ok */ + + bucket = &set->buckets[(size_t)val % set->bucket_count]; + if (bucket->count == 0) { + bucket->v.val = val; + bucket->count = 1; + } else if (bucket->count == 1) { + INTSET_TYPE old = bucket->v.val; + bucket->v.data = (INTSET_TYPE*)malloc(2 * sizeof(INTSET_TYPE)); + if (!bucket->v.data) { + bucket->v.val = old; + return false; /* error */ + } + bucket->v.data[0] = old; + bucket->v.data[1] = val; + bucket->count = 2; + } else { + size_t new_bucket_size; + INTSET_TYPE* new_bucket_data; + + new_bucket_size = (bucket->count + 1) * sizeof(INTSET_TYPE); + new_bucket_data = (INTSET_TYPE*)realloc(bucket->v.data, new_bucket_size); + if (!new_bucket_data) + return false; /* error */ + bucket->v.data = new_bucket_data; + bucket->v.data[bucket->count++] = val; + } + return true; /* success */ +} + + +#if defined(_MSC_VER) +#pragma warning(pop) +#endif + +#undef INTSET_FUNC +#undef INTSET_BUCKET +#undef INTSET_UNION diff --git a/test/cpp/CMakeLists.txt b/test/cpp/CMakeLists.txt new file mode 100644 index 0000000000..927b7494da --- /dev/null +++ b/test/cpp/CMakeLists.txt @@ -0,0 +1,49 @@ +cmake_minimum_required(VERSION 3.5) +project(ap-cpp-tests) + +enable_testing() + +find_package(GTest REQUIRED) + +if (CMAKE_CXX_COMPILER_ID STREQUAL "MSVC") + add_definitions("/source-charset:utf-8") + set(CMAKE_CXX_FLAGS_DEBUG "/MTd") + set(CMAKE_CXX_FLAGS_RELEASE "/MT") +elseif (CMAKE_CXX_COMPILER_ID STREQUAL "GNU") + # enable static analysis for gcc + add_compile_options(-fanalyzer -Werror) + # disable stuff that gets triggered by googletest + add_compile_options(-Wno-analyzer-malloc-leak) + # enable asan for gcc + add_compile_options(-fsanitize=address) + add_link_options(-fsanitize=address) +endif () + +add_executable(test_default) + +target_include_directories(test_default + PRIVATE + ${GTEST_INCLUDE_DIRS} +) + +target_link_libraries(test_default + ${GTEST_BOTH_LIBRARIES} +) + +add_test( + NAME test_default + COMMAND test_default +) + +set_property( + TEST test_default + PROPERTY ENVIRONMENT "ASAN_OPTIONS=allocator_may_return_null=1" +) + +file(GLOB ITEMS *) +foreach(item ${ITEMS}) + if(IS_DIRECTORY ${item} AND EXISTS ${item}/CMakeLists.txt) + message(${item}) + add_subdirectory(${item}) + endif() +endforeach() diff --git a/test/cpp/README.md b/test/cpp/README.md new file mode 100644 index 0000000000..792b9be77e --- /dev/null +++ b/test/cpp/README.md @@ -0,0 +1,32 @@ +# C++ tests + +Test framework for C and C++ code in AP. + +## Adding a Test + +### GoogleTest + +Adding GoogleTests is as simple as creating a directory with +* one or more `test_*.cpp` files that define tests using + [GoogleTest API](https://google.github.io/googletest/) +* a `CMakeLists.txt` that adds the .cpp files to `test_default` target using + [target_sources](https://cmake.org/cmake/help/latest/command/target_sources.html) + +### CTest + +If either GoogleTest is not suitable for the test or the build flags / sources / libraries are incompatible, +you can add another CTest to the project using add_target and add_test, similar to how it's done for `test_default`. + +## Running Tests + +* Install [CMake](https://cmake.org/). +* Build and/or install GoogleTest and make sure + [CMake can find it](https://cmake.org/cmake/help/latest/module/FindGTest.html), or + [create a parent `CMakeLists.txt` that fetches GoogleTest](https://google.github.io/googletest/quickstart-cmake.html). +* Enter the directory with the top-most `CMakeLists.txt` and run + ```sh + mkdir build + cmake -S . -B build/ -DCMAKE_BUILD_TYPE=Release + cmake --build build/ --config Release && \ + ctest --test-dir build/ -C Release --output-on-failure + ``` diff --git a/test/cpp/intset/CMakeLists.txt b/test/cpp/intset/CMakeLists.txt new file mode 100644 index 0000000000..175e0bd0b9 --- /dev/null +++ b/test/cpp/intset/CMakeLists.txt @@ -0,0 +1,4 @@ +target_sources(test_default + PRIVATE + ${CMAKE_CURRENT_SOURCE_DIR}/test_intset.cpp +) diff --git a/test/cpp/intset/test_intset.cpp b/test/cpp/intset/test_intset.cpp new file mode 100644 index 0000000000..2f85bea960 --- /dev/null +++ b/test/cpp/intset/test_intset.cpp @@ -0,0 +1,105 @@ +#include +#include +#include + +// uint32Set +#define INTSET_NAME uint32Set +#define INTSET_TYPE uint32_t +#include "../../../intset.h" +#undef INTSET_NAME +#undef INTSET_TYPE + +// int64Set +#define INTSET_NAME int64Set +#define INTSET_TYPE int64_t +#include "../../../intset.h" + + +TEST(IntsetTest, ZeroBuckets) +{ + // trying to allocate with zero buckets has to either fail or be functioning + uint32Set *set = uint32Set_new(0); + if (!set) + return; // failed -> OK + + EXPECT_FALSE(uint32Set_contains(set, 1)); + EXPECT_TRUE(uint32Set_add(set, 1)); + EXPECT_TRUE(uint32Set_contains(set, 1)); + uint32Set_free(set); +} + +TEST(IntsetTest, Duplicate) +{ + // adding the same number again can't fail + uint32Set *set = uint32Set_new(2); + ASSERT_TRUE(set); + EXPECT_TRUE(uint32Set_add(set, 0)); + EXPECT_TRUE(uint32Set_add(set, 0)); + EXPECT_TRUE(uint32Set_contains(set, 0)); + uint32Set_free(set); +} + +TEST(IntsetTest, SetAllocFailure) +{ + // try to allocate 100TB of RAM, should fail and return NULL + if (sizeof(size_t) < 8) + GTEST_SKIP() << "Alloc error not testable on 32bit"; + int64Set *set = int64Set_new(6250000000000ULL); + EXPECT_FALSE(set); + int64Set_free(set); +} + +TEST(IntsetTest, SetAllocOverflow) +{ + // try to overflow argument passed to malloc + int64Set *set = int64Set_new(std::numeric_limits::max()); + EXPECT_FALSE(set); + int64Set_free(set); +} + +TEST(IntsetTest, NullFree) +{ + // free(NULL) should not try to free buckets + uint32Set_free(NULL); + int64Set_free(NULL); +} + +TEST(IntsetTest, BucketRealloc) +{ + // add a couple of values to the same bucket to test growing the bucket + uint32Set* set = uint32Set_new(1); + ASSERT_TRUE(set); + EXPECT_FALSE(uint32Set_contains(set, 0)); + EXPECT_TRUE(uint32Set_add(set, 0)); + EXPECT_TRUE(uint32Set_contains(set, 0)); + for (uint32_t i = 1; i < 32; ++i) { + EXPECT_TRUE(uint32Set_add(set, i)); + EXPECT_TRUE(uint32Set_contains(set, i - 1)); + EXPECT_TRUE(uint32Set_contains(set, i)); + EXPECT_FALSE(uint32Set_contains(set, i + 1)); + } + uint32Set_free(set); +} + +TEST(IntSet, Max) +{ + constexpr auto n = std::numeric_limits::max(); + uint32Set *set = uint32Set_new(1); + ASSERT_TRUE(set); + EXPECT_FALSE(uint32Set_contains(set, n)); + EXPECT_TRUE(uint32Set_add(set, n)); + EXPECT_TRUE(uint32Set_contains(set, n)); + uint32Set_free(set); +} + +TEST(InsetTest, Negative) +{ + constexpr auto n = std::numeric_limits::min(); + static_assert(n < 0, "n not negative"); + int64Set *set = int64Set_new(3); + ASSERT_TRUE(set); + EXPECT_FALSE(int64Set_contains(set, n)); + EXPECT_TRUE(int64Set_add(set, n)); + EXPECT_TRUE(int64Set_contains(set, n)); + int64Set_free(set); +} diff --git a/test/netutils/test_location_store.py b/test/netutils/test_location_store.py index a7f117255f..f3e83989be 100644 --- a/test/netutils/test_location_store.py +++ b/test/netutils/test_location_store.py @@ -1,4 +1,5 @@ # Tests for _speedups.LocationStore and NetUtils._LocationStore +import os import typing import unittest import warnings @@ -7,6 +8,8 @@ from NetUtils import LocationStore, _LocationStore State = typing.Dict[typing.Tuple[int, int], typing.Set[int]] RawLocations = typing.Dict[int, typing.Dict[int, typing.Tuple[int, int, int]]] +ci = bool(os.environ.get("CI")) # always set in GitHub actions + sample_data: RawLocations = { 1: { 11: (21, 2, 7), @@ -24,6 +27,9 @@ sample_data: RawLocations = { 3: { 9: (99, 4, 0), }, + 5: { + 9: (99, 5, 0), + } } empty_state: State = { @@ -45,14 +51,14 @@ class Base: store: typing.Union[LocationStore, _LocationStore] def test_len(self) -> None: - self.assertEqual(len(self.store), 4) + self.assertEqual(len(self.store), 5) self.assertEqual(len(self.store[1]), 3) def test_key_error(self) -> None: with self.assertRaises(KeyError): _ = self.store[0] with self.assertRaises(KeyError): - _ = self.store[5] + _ = self.store[6] locations = self.store[1] # no Exception with self.assertRaises(KeyError): _ = locations[7] @@ -71,7 +77,7 @@ class Base: self.assertEqual(self.store[1].get(10, (None, None, None)), (None, None, None)) def test_iter(self) -> None: - self.assertEqual(sorted(self.store), [1, 2, 3, 4]) + self.assertEqual(sorted(self.store), [1, 2, 3, 4, 5]) self.assertEqual(len(self.store), len(sample_data)) self.assertEqual(list(self.store[1]), [11, 12, 13]) self.assertEqual(len(self.store[1]), len(sample_data[1])) @@ -85,13 +91,26 @@ class Base: self.assertEqual(sorted(self.store[1].items())[0][1], self.store[1][11]) def test_find_item(self) -> None: + # empty player set self.assertEqual(sorted(self.store.find_item(set(), 99)), []) + # no such player, single + self.assertEqual(sorted(self.store.find_item({6}, 99)), []) + # no such player, set + self.assertEqual(sorted(self.store.find_item({7, 8, 9}, 99)), []) + # no such item self.assertEqual(sorted(self.store.find_item({3}, 1)), []) - self.assertEqual(sorted(self.store.find_item({5}, 99)), []) + # valid matches self.assertEqual(sorted(self.store.find_item({3}, 99)), [(4, 9, 99, 3, 0)]) self.assertEqual(sorted(self.store.find_item({3, 4}, 99)), [(3, 9, 99, 4, 0), (4, 9, 99, 3, 0)]) + self.assertEqual(sorted(self.store.find_item({2, 3, 4}, 99)), + [(3, 9, 99, 4, 0), (4, 9, 99, 3, 0)]) + # test hash collision in set + self.assertEqual(sorted(self.store.find_item({3, 5}, 99)), + [(4, 9, 99, 3, 0), (5, 9, 99, 5, 0)]) + self.assertEqual(sorted(self.store.find_item(set(range(2048)), 13)), + [(1, 13, 13, 1, 0)]) def test_get_for_player(self) -> None: self.assertEqual(self.store.get_for_player(3), {4: {9}}) @@ -196,18 +215,20 @@ class TestPurePythonLocationStoreConstructor(Base.TestLocationStoreConstructor): super().setUp() -@unittest.skipIf(LocationStore is _LocationStore, "_speedups not available") +@unittest.skipIf(LocationStore is _LocationStore and not ci, "_speedups not available") class TestSpeedupsLocationStore(Base.TestLocationStore): """Run base method tests for cython implementation.""" def setUp(self) -> None: + self.assertFalse(LocationStore is _LocationStore, "Failed to load _speedups") self.store = LocationStore(sample_data) super().setUp() -@unittest.skipIf(LocationStore is _LocationStore, "_speedups not available") +@unittest.skipIf(LocationStore is _LocationStore and not ci, "_speedups not available") class TestSpeedupsLocationStoreConstructor(Base.TestLocationStoreConstructor): """Run base constructor tests and tests the additional constraints for cython implementation.""" def setUp(self) -> None: + self.assertFalse(LocationStore is _LocationStore, "Failed to load _speedups") self.type = LocationStore super().setUp() From c108845d1ff26979ddb4a5168faec82b1206292e Mon Sep 17 00:00:00 2001 From: black-sliver <59490463+black-sliver@users.noreply.github.com> Date: Wed, 12 Jun 2024 18:55:48 +0200 Subject: [PATCH 07/18] CI: more checks in build and rework compression (#3336) * CI: build: fail fast if setup.py fails on windows * CI: build: fail for missing uploads, rework compression Upload-artifact allows setting compression level now. The change speeds up both upload and extraction. * CI: match build gz in release * CI: build: verify worlds all load * CI: build: generate a game * Generate: move worlds loaded exception to allow settings to init from worlds * CI: build: build setup before running tests --- .github/workflows/build.yml | 60 +++++++++++++++++++++++++++++++---- .github/workflows/release.yml | 2 +- Generate.py | 8 +++-- 3 files changed, 59 insertions(+), 11 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 80aaf70c21..dd88d8d7d7 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -40,6 +40,10 @@ jobs: run: | python -m pip install --upgrade pip python setup.py build_exe --yes + if ( $? -eq $false ) { + Write-Error "setup.py failed!" + exit 1 + } $NAME="$(ls build | Select-String -Pattern 'exe')".Split('.',2)[1] $ZIP_NAME="Archipelago_$NAME.7z" echo "$NAME -> $ZIP_NAME" @@ -49,12 +53,6 @@ jobs: Rename-Item "exe.$NAME" Archipelago 7z a -mx=9 -mhe=on -ms "../dist/$ZIP_NAME" Archipelago Rename-Item Archipelago "exe.$NAME" # inno_setup.iss expects the original name - - name: Store 7z - uses: actions/upload-artifact@v4 - with: - name: ${{ env.ZIP_NAME }} - path: dist/${{ env.ZIP_NAME }} - retention-days: 7 # keep for 7 days, should be enough - name: Build Setup run: | & "${env:ProgramFiles(x86)}\Inno Setup 6\iscc.exe" inno_setup.iss /DNO_SIGNTOOL @@ -65,11 +63,38 @@ jobs: $contents = Get-ChildItem -Path setups/*.exe -Force -Recurse $SETUP_NAME=$contents[0].Name echo "SETUP_NAME=$SETUP_NAME" >> $Env:GITHUB_ENV + - name: Check build loads expected worlds + shell: bash + run: | + cd build/exe* + mv Players/Templates/meta.yaml . + ls -1 Players/Templates | sort > setup-player-templates.txt + rm -R Players/Templates + timeout 30 ./ArchipelagoLauncher "Generate Template Options" || true + ls -1 Players/Templates | sort > generated-player-templates.txt + cmp setup-player-templates.txt generated-player-templates.txt \ + || diff setup-player-templates.txt generated-player-templates.txt + mv meta.yaml Players/Templates/ + - name: Test Generate + shell: bash + run: | + cd build/exe* + cp Players/Templates/Clique.yaml Players/ + timeout 30 ./ArchipelagoGenerate + - name: Store 7z + uses: actions/upload-artifact@v4 + with: + name: ${{ env.ZIP_NAME }} + path: dist/${{ env.ZIP_NAME }} + compression-level: 0 # .7z is incompressible by zip + if-no-files-found: error + retention-days: 7 # keep for 7 days, should be enough - name: Store Setup uses: actions/upload-artifact@v4 with: name: ${{ env.SETUP_NAME }} path: setups/${{ env.SETUP_NAME }} + if-no-files-found: error retention-days: 7 # keep for 7 days, should be enough build-ubuntu2004: @@ -110,7 +135,7 @@ jobs: echo -e "setup.py dist output:\n `ls dist`" cd dist && export APPIMAGE_NAME="`ls *.AppImage`" && cd .. export TAR_NAME="${APPIMAGE_NAME%.AppImage}.tar.gz" - (cd build && DIR_NAME="`ls | grep exe`" && mv "$DIR_NAME" Archipelago && tar -czvf ../dist/$TAR_NAME Archipelago && mv Archipelago "$DIR_NAME") + (cd build && DIR_NAME="`ls | grep exe`" && mv "$DIR_NAME" Archipelago && tar -cv Archipelago | gzip -8 > ../dist/$TAR_NAME && mv Archipelago "$DIR_NAME") echo "APPIMAGE_NAME=$APPIMAGE_NAME" >> $GITHUB_ENV echo "TAR_NAME=$TAR_NAME" >> $GITHUB_ENV # - copy code above to release.yml - @@ -118,15 +143,36 @@ jobs: run: | source venv/bin/activate python setup.py build_exe --yes + - name: Check build loads expected worlds + shell: bash + run: | + cd build/exe* + mv Players/Templates/meta.yaml . + ls -1 Players/Templates | sort > setup-player-templates.txt + rm -R Players/Templates + timeout 30 ./ArchipelagoLauncher "Generate Template Options" || true + ls -1 Players/Templates | sort > generated-player-templates.txt + cmp setup-player-templates.txt generated-player-templates.txt \ + || diff setup-player-templates.txt generated-player-templates.txt + mv meta.yaml Players/Templates/ + - name: Test Generate + shell: bash + run: | + cd build/exe* + cp Players/Templates/Clique.yaml Players/ + timeout 30 ./ArchipelagoGenerate - name: Store AppImage uses: actions/upload-artifact@v4 with: name: ${{ env.APPIMAGE_NAME }} path: dist/${{ env.APPIMAGE_NAME }} + if-no-files-found: error retention-days: 7 - name: Store .tar.gz uses: actions/upload-artifact@v4 with: name: ${{ env.TAR_NAME }} path: dist/${{ env.TAR_NAME }} + compression-level: 0 # .gz is incompressible by zip + if-no-files-found: error retention-days: 7 diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 2d7f1253b7..3f8651d408 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -69,7 +69,7 @@ jobs: echo -e "setup.py dist output:\n `ls dist`" cd dist && export APPIMAGE_NAME="`ls *.AppImage`" && cd .. export TAR_NAME="${APPIMAGE_NAME%.AppImage}.tar.gz" - (cd build && DIR_NAME="`ls | grep exe`" && mv "$DIR_NAME" Archipelago && tar -czvf ../dist/$TAR_NAME Archipelago && mv Archipelago "$DIR_NAME") + (cd build && DIR_NAME="`ls | grep exe`" && mv "$DIR_NAME" Archipelago && tar -cv Archipelago | gzip -8 > ../dist/$TAR_NAME && mv Archipelago "$DIR_NAME") echo "APPIMAGE_NAME=$APPIMAGE_NAME" >> $GITHUB_ENV echo "TAR_NAME=$TAR_NAME" >> $GITHUB_ENV # - code above copied from build.yml - diff --git a/Generate.py b/Generate.py index 0cef081120..1fbb9e76a4 100644 --- a/Generate.py +++ b/Generate.py @@ -66,13 +66,15 @@ def get_seed_name(random_source) -> str: def main(args=None): + # __name__ == "__main__" check so unittests that already imported worlds don't trip this. + if __name__ == "__main__" and "worlds" in sys.modules: + raise Exception("Worlds system should not be loaded before logging init.") + if not args: args = mystery_argparse() seed = get_seed(args.seed) - # __name__ == "__main__" check so unittests that already imported worlds don't trip this. - if __name__ == "__main__" and "worlds" in sys.modules: - raise Exception("Worlds system should not be loaded before logging init.") + Utils.init_logging(f"Generate_{seed}", loglevel=args.log_level) random.seed(seed) seed_name = get_seed_name(random) From da34800f43a445470820778e3a7e0c5be5b7d4d3 Mon Sep 17 00:00:00 2001 From: JoshuaEagles Date: Thu, 13 Jun 2024 00:53:01 -0400 Subject: [PATCH 08/18] Fix Incorrect Link Syntax in SA2B Linux Setup (#3524) --- worlds/sa2b/docs/setup_en.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/worlds/sa2b/docs/setup_en.md b/worlds/sa2b/docs/setup_en.md index 354ef4bbe9..f32001a678 100644 --- a/worlds/sa2b/docs/setup_en.md +++ b/worlds/sa2b/docs/setup_en.md @@ -48,7 +48,7 @@ 7. Install protontricks, on the Steam Deck this can be done via the Discover store, on other distros instructions vary, [see its github page](https://github.com/Matoking/protontricks). -8. Download the [.NET 7 Desktop Runtime for x64 Windows](https://dotnet.microsoft.com/en-us/download/dotnet/thank-you/runtime-desktop-7.0.17-windows-x64-installer}. If this link does not work, the download can be found on [this page](https://dotnet.microsoft.com/en-us/download/dotnet/7.0). +8. Download the [.NET 7 Desktop Runtime for x64 Windows](https://dotnet.microsoft.com/en-us/download/dotnet/thank-you/runtime-desktop-7.0.17-windows-x64-installer). If this link does not work, the download can be found on [this page](https://dotnet.microsoft.com/en-us/download/dotnet/7.0). 9. Right click the .NET 7 Desktop Runtime exe, and assuming protontricks was installed correctly, the option to "Open with Protontricks Launcher" should be available. Click that, and in the popup window that opens, select SAModManager.exe. Follow the prompts after this to install the .NET 7 Desktop Runtime for SAModManager. Once it is done, you should be able to successfully launch SAModManager to steam. From f6e3113af6805c38e1e5c322c5747b89465722f1 Mon Sep 17 00:00:00 2001 From: NewSoupVi <57900059+NewSoupVi@users.noreply.github.com> Date: Thu, 13 Jun 2024 10:39:16 +0200 Subject: [PATCH 09/18] WebHost: Fix "Add" button for custom option values causing a weird redirect (#3518) * WebHost: Fix "Add" button for Progression Balancing causing a weird redirect This "add" button is part of a form, which causes it to submit the form, because the default type for a button is "submit". This PR changes the type of the button to "button", which causes it to not submit the form and just execute its normal effect. (An alternative would be `event.preventDefault()` but that seems less clean to me, but also I'm not a HTML/JS dev) * There's also multiple. --- WebHostLib/templates/weightedOptions/macros.html | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/WebHostLib/templates/weightedOptions/macros.html b/WebHostLib/templates/weightedOptions/macros.html index 55a56e3285..c7a5d4174b 100644 --- a/WebHostLib/templates/weightedOptions/macros.html +++ b/WebHostLib/templates/weightedOptions/macros.html @@ -47,7 +47,7 @@ {% endif %}
- +
@@ -72,7 +72,7 @@ This option allows custom values only. Please enter your desired values below.
- +
@@ -89,7 +89,7 @@ Custom values are also allowed for this option. To create one, enter it into the input box below.
- +
From 2ae51364d9052731ea3fae2b25aa61d5a7e5a8ae Mon Sep 17 00:00:00 2001 From: NewSoupVi <57900059+NewSoupVi@users.noreply.github.com> Date: Thu, 13 Jun 2024 18:24:56 +0200 Subject: [PATCH 10/18] WebHost: Fix default values that are 2 or more words in Weighted Options (#3519) * WeightedOptions: Fix default values that are 2 or more words * So much simpler --- WebHostLib/templates/weightedOptions/macros.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/WebHostLib/templates/weightedOptions/macros.html b/WebHostLib/templates/weightedOptions/macros.html index c7a5d4174b..4d9e7ca4d3 100644 --- a/WebHostLib/templates/weightedOptions/macros.html +++ b/WebHostLib/templates/weightedOptions/macros.html @@ -19,7 +19,7 @@ {% for id, name in option.name_lookup.items() %} {% if name != 'random' %} {% if option.default != 'random' %} - {{ RangeRow(option_name, option, option.get_option_name(id), name, False, name if option.get_option_name(option.default)|lower == name else None) }} + {{ RangeRow(option_name, option, option.get_option_name(id), name, False, name if option.default == id else None) }} {% else %} {{ RangeRow(option_name, option, option.get_option_name(id), name) }} {% endif %} @@ -97,7 +97,7 @@ {% for id, name in option.name_lookup.items() %} {% if name != 'random' %} {% if option.default != 'random' %} - {{ RangeRow(option_name, option, option.get_option_name(id), name, False, name if option.get_option_name(option.default)|lower == name else None) }} + {{ RangeRow(option_name, option, option.get_option_name(id), name, False, name if option.default == id else None) }} {% else %} {{ RangeRow(option_name, option, option.get_option_name(id), name) }} {% endif %} From 533395d336073081bcf8a7cba461f7529a49d8ff Mon Sep 17 00:00:00 2001 From: NewSoupVi <57900059+NewSoupVi@users.noreply.github.com> Date: Thu, 13 Jun 2024 23:29:39 +0200 Subject: [PATCH 11/18] WebHost: Fix Named Range displays on Player Options page (#3521) * Player Options: Fix Named Range displays * Also add validation to the NamedRange class itself * Don't break Stardew * Comment * Do replace first so title works correctly * Bring change to Weighted Options as well --- Options.py | 6 ++++++ WebHostLib/templates/playerOptions/macros.html | 4 ++-- WebHostLib/templates/weightedOptions/macros.html | 2 +- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/Options.py b/Options.py index 40a6996d32..2d3ef99d64 100644 --- a/Options.py +++ b/Options.py @@ -735,6 +735,12 @@ class NamedRange(Range): elif value > self.range_end and value not in self.special_range_names.values(): raise Exception(f"{value} is higher than maximum {self.range_end} for option {self.__class__.__name__} " + f"and is also not one of the supported named special values: {self.special_range_names}") + + # See docstring + for key in self.special_range_names: + if key != key.lower(): + raise Exception(f"{self.__class__.__name__} has an invalid special_range_names key: {key}. " + f"NamedRange keys must use only lowercase letters, and ideally should be snake_case.") self.value = value @classmethod diff --git a/WebHostLib/templates/playerOptions/macros.html b/WebHostLib/templates/playerOptions/macros.html index b34ac79a02..2187ffe913 100644 --- a/WebHostLib/templates/playerOptions/macros.html +++ b/WebHostLib/templates/playerOptions/macros.html @@ -57,9 +57,9 @@