From 1cf33352bd7f10b26a64489084fd8a50e492c42c Mon Sep 17 00:00:00 2001 From: ChippySpud <96747360+CFeeney333@users.noreply.github.com> Date: Tue, 21 Mar 2023 18:18:04 +0000 Subject: [PATCH 1/7] Fix bug in spawn_util Return False not True in order to skip the current file (this_file) --- MCprep_addon/spawner/spawn_util.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MCprep_addon/spawner/spawn_util.py b/MCprep_addon/spawner/spawn_util.py index 90651d41..ecc8abe4 100755 --- a/MCprep_addon/spawner/spawn_util.py +++ b/MCprep_addon/spawner/spawn_util.py @@ -151,7 +151,7 @@ def tuple_from_match(match): # If the suffix = 3.0 in `afile` file, and current blender is # below 3, then `afile` is the file that should be loaded, and the # current file `this_file` is to be skipped. - return res + return not res # If no matches (the most common case), then this file is eligible. return True From f49fe67aaa99e202521d53d0a0fd88150718b427 Mon Sep 17 00:00:00 2001 From: chippyspud Date: Wed, 22 Mar 2023 10:08:42 +0000 Subject: [PATCH 2/7] Fix bug in spawn_util.py If we find a versioned file with the same basename, check recursively to see if it's eligible. If it's not, we have to allow for a check to see if there are any more versioned files that are eligible. The previous version only allowed for one check, and could return a result without finding an eligible versioned file, even if there was an eligible versioned file (unchecked), so the previous fix could still have produced incorrect result. If we do find an eligible versioned file, we can return False, because we want that file to be used instead. Otherwise we can just continue to the next iteration for the next check. If there are no eligible versioned files, it will fall through to return True anyway. --- MCprep_addon/spawner/spawn_util.py | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/MCprep_addon/spawner/spawn_util.py b/MCprep_addon/spawner/spawn_util.py index ecc8abe4..c67bb24c 100755 --- a/MCprep_addon/spawner/spawn_util.py +++ b/MCprep_addon/spawner/spawn_util.py @@ -146,12 +146,24 @@ def tuple_from_match(match): base_afile = os.path.splitext(afile)[0] matches = find_suffix.search(base_afile) if matches: - tuple_ver = tuple_from_match(matches) - res = util.min_bv(tuple_ver) - # If the suffix = 3.0 in `afile` file, and current blender is - # below 3, then `afile` is the file that should be loaded, and the - # current file `this_file` is to be skipped. - return not res + # afile is guaranteed to have the same basename, not be the same file, and has a suffix + # because it has a suffix, the recursively called function will not reach the containing for loop + if not check_blend_eligible(afile, all_files): + # if afile is not eligible, we can continue to the next iteration to see if there is another + # file with a suffix that is eligible + # previously this only allowed for 1 other file with a suffix to be checked + continue + else: + # if a file with a suffix is eligible here, we should not use the current file, but should use afile + # instead, so return False for the current file + return False + + # tuple_ver = tuple_from_match(matches) + # res = util.min_bv(tuple_ver) + # # If the suffix = 3.0 in `afile` file, and current blender is + # # below 3, then `afile` is the file that should be loaded, and the + # # current file `this_file` is to be skipped. + # return not res # If no matches (the most common case), then this file is eligible. return True From 323028eac9854041ff47e8d7318ee08c7bf15a01 Mon Sep 17 00:00:00 2001 From: ChippySpud <96747360+CFeeney333@users.noreply.github.com> Date: Wed, 22 Mar 2023 20:31:42 +0000 Subject: [PATCH 3/7] Remove commented out code --- MCprep_addon/spawner/spawn_util.py | 7 ------- 1 file changed, 7 deletions(-) diff --git a/MCprep_addon/spawner/spawn_util.py b/MCprep_addon/spawner/spawn_util.py index c67bb24c..7877c70b 100755 --- a/MCprep_addon/spawner/spawn_util.py +++ b/MCprep_addon/spawner/spawn_util.py @@ -158,13 +158,6 @@ def tuple_from_match(match): # instead, so return False for the current file return False - # tuple_ver = tuple_from_match(matches) - # res = util.min_bv(tuple_ver) - # # If the suffix = 3.0 in `afile` file, and current blender is - # # below 3, then `afile` is the file that should be loaded, and the - # # current file `this_file` is to be skipped. - # return not res - # If no matches (the most common case), then this file is eligible. return True From a51f45b5d3678856f8ff7622c56ccc15f19f0561 Mon Sep 17 00:00:00 2001 From: ChippySpud <96747360+CFeeney333@users.noreply.github.com> Date: Wed, 22 Mar 2023 20:35:17 +0000 Subject: [PATCH 4/7] Wrap long comments --- MCprep_addon/spawner/spawn_util.py | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/MCprep_addon/spawner/spawn_util.py b/MCprep_addon/spawner/spawn_util.py index 7877c70b..e96ab16c 100755 --- a/MCprep_addon/spawner/spawn_util.py +++ b/MCprep_addon/spawner/spawn_util.py @@ -146,15 +146,20 @@ def tuple_from_match(match): base_afile = os.path.splitext(afile)[0] matches = find_suffix.search(base_afile) if matches: - # afile is guaranteed to have the same basename, not be the same file, and has a suffix - # because it has a suffix, the recursively called function will not reach the containing for loop + # afile is guaranteed to have the same basename, + # not be the same file, and has a suffix + # Because it has a suffix, the recursively called + # function will not reach the containing for loop if not check_blend_eligible(afile, all_files): - # if afile is not eligible, we can continue to the next iteration to see if there is another + # if afile is not eligible, we can continue to the next + # iteration to see if there is another # file with a suffix that is eligible - # previously this only allowed for 1 other file with a suffix to be checked + # previously this only allowed for 1 other file with a + # suffix to be checked continue else: - # if a file with a suffix is eligible here, we should not use the current file, but should use afile + # if a file with a suffix is eligible here, we should not + # use the current file, but should use afile # instead, so return False for the current file return False From 1f6c9576a17584b20e0ee27b1f15e66336f61315 Mon Sep 17 00:00:00 2001 From: "Patrick W. Crawford" Date: Wed, 22 Mar 2023 22:06:44 -0700 Subject: [PATCH 5/7] Fixing check_blend_eligible and adding more tests Previously this function would try to exit early true after only inspecting this_file on its own, when in reality it would always need to compare against all files in the list. --- MCprep_addon/spawner/spawn_util.py | 99 +++++++++++++++++++----------- test_files/addon_tests.py | 44 +++++++++++++ 2 files changed, 106 insertions(+), 37 deletions(-) diff --git a/MCprep_addon/spawner/spawn_util.py b/MCprep_addon/spawner/spawn_util.py index e96ab16c..fb1103da 100755 --- a/MCprep_addon/spawner/spawn_util.py +++ b/MCprep_addon/spawner/spawn_util.py @@ -98,21 +98,29 @@ def filter_collections(data_from): def check_blend_eligible(this_file, all_files): - """Returns true if the path blend file is ok for this blender version. + """Returns true if this_file is the BEST blend file variant for this rig. Created to better support older blender versions without having to - compeltely remake new rig contirbutions from scratch. See for more details: + completely remake new rig contributions from scratch. See for more details: https://github.com/TheDuckCow/MCprep/issues/317 - If the "pre#.#.#" suffix is found in any similar prefix named blend files, - evaluate whether the target file itself is eligible. If pre3.0.0 is found - in path, then this function returns False if the current blender version is - 3.0 to force loading the non pre3.0.0 version instead, and if pre3.0.0 is - not in the path variable but pre#.#.# is found in another file, then it - returns true. If no pre#.#.# found in any files of the common base, would - always return True. + This function searches for "pre#.#.#" in the filename of passed in files, + where if found, it indicates that file should only be used if the current + running blender version is below that version (non inclusive). The function + then finds the highest versioned file and checks if this_file is that + the same one. If so, it returns true, otherwise returns false. Most rigs + do not have versioned names are are assumed to work with the latest blender + version, hence they are treated as a "max_ver" to never get thrown out. + + Examples, presuming current blender is v2.93 + for list ["rig pre2.8.0", "rig"] + -> returns true for "rig" + for list ["rig pre2.8.0", "rig pre3.0.0", "rig"] + -> returns true for "rig pre3.0.0" + """ basename = os.path.splitext(this_file)[0] + max_ver = (99, 99, 99) # Standin for an impossibly high blender version. # Regex code to any matches of pre#.#.# at very end of name. # (pre) for exact match, @@ -129,42 +137,59 @@ def tuple_from_match(match): return tuple_ver matches = find_suffix.search(basename) + base_match = basename + + # Exit early, or find the common base after splitting out " pre#.#.#.blend" if matches: - tuple_ver = tuple_from_match(matches) - res = util.min_bv(tuple_ver) - # If the suffix = 3.0 and we are BELOW 3.0, return True (use this file) - return not res + base_match = re.split(code, basename)[0].strip() + this_ver = tuple_from_match(matches) + res = util.min_bv(this_ver, inclusive=True) + if res is True: + # E.g. rig pre3.0.0 in blender 3.0.1 or 3.0.0, so file ineligible. + return False + else: + # ie rig pre3.0.0 with current blender 2.9, so this is eligible - + # but another file might still be a better match. + pass + else: + this_ver = max_ver - # Remaining case: no suffix found in this target, but see if any other - # files have the same base and *do* have a suffix indicator. + # Iterate over all files, structure of: + # List of list[filepath, tuple detected] + other_eligible = [] for afile in all_files: - if not afile.startswith(basename): - continue - if afile == this_file: - continue # Already checked above. + if not afile.startswith(base_match): + continue # Not part of the same base name, skip it base_afile = os.path.splitext(afile)[0] matches = find_suffix.search(base_afile) if matches: - # afile is guaranteed to have the same basename, - # not be the same file, and has a suffix - # Because it has a suffix, the recursively called - # function will not reach the containing for loop - if not check_blend_eligible(afile, all_files): - # if afile is not eligible, we can continue to the next - # iteration to see if there is another - # file with a suffix that is eligible - # previously this only allowed for 1 other file with a - # suffix to be checked - continue - else: - # if a file with a suffix is eligible here, we should not - # use the current file, but should use afile - # instead, so return False for the current file - return False + tuple_ver = tuple_from_match(matches) + res = util.min_bv(tuple_ver, inclusive=True) + if res is False: + # E.g. rig pre3.0.0 in blender 2.9 is an eligible choice + other_eligible.append([tuple_ver, afile]) + else: + # This would be the typical case of a "latest, non versioned" file. + # E.g. rig.blend with no "pre3.3.0" text, also a eligible choice. + # Give an artificial version to compare against for next step. + other_eligible.append([max_ver, afile]) + + if len(other_eligible) == 1: + # Only this_file is eligible, and thus must be "the one". + return True + + # If multiple files, then we should use the one that is the largest version + # without being equal to or greater than the current running blender ver. + sorted_eligible = sorted(other_eligible, key=lambda x: x[0]) + latest_allowed = None + for tple, afile in sorted_eligible: + if util.min_bv(tple, inclusive=True) is False: + latest_allowed = afile + else: + break - # If no matches (the most common case), then this file is eligible. - return True + return latest_allowed != this_file def attemptScriptLoad(path): diff --git a/test_files/addon_tests.py b/test_files/addon_tests.py index 696e83cb..4d334e38 100644 --- a/test_files/addon_tests.py +++ b/test_files/addon_tests.py @@ -58,6 +58,7 @@ def __init__(self): self.spawn_mob, self.spawn_mob_linked, self.check_blend_eligible, + self.check_blend_eligible_middle, self.change_skin, self.import_world_split, self.import_world_fail, @@ -733,6 +734,49 @@ def check_blend_eligible(self): if res is not False: return "Should have been false since we are above this min blender" + def check_blend_eligible_middle(self): + # Warden-like example, where we have equiv of pre2.80, pre3.0, and + # live blender 3.0+ (presuming we want to test a 2.93-like user) + from MCprep.spawner import spawn_util + fake_base = "WardenExample" + + # Assume the "current" version of blender is like 9.1 + # To make test not be flakey, actual version of blender can be anything + # in range of 2.7.0 upwards to 8.999. + suffix_old = " pre2.7.0" # Force active blender instance as older. + suffix_mid = " pre9.0.0" # Force active blender instance as older. + suffix_new = "" # presume "latest" version + + p_old = fake_base + suffix_old + ".blend" + p_mid = fake_base + suffix_mid + ".blend" + p_new = fake_base + suffix_new + ".blend" + + # Test in order + filelist = [p_old, p_mid, p_new] + + res = spawn_util.check_blend_eligible(p_old, filelist) + if res is True: + return "Older file should not match (in order)" + res = spawn_util.check_blend_eligible(p_mid, filelist) + if res is not True: + return "Mid file SHOULD match (in order)" + res = spawn_util.check_blend_eligible(p_new, filelist) + if res is True: + return "Newer file should not match (in order)" + + # Test out of order + filelist = [p_mid, p_new, p_old] + + res = spawn_util.check_blend_eligible(p_old, filelist) + if res is True: + return "Older file should not match (out of order)" + res = spawn_util.check_blend_eligible(p_mid, filelist) + if res is not True: + return "Mid file SHOULD match (out of order)" + res = spawn_util.check_blend_eligible(p_new, filelist) + if res is True: + return "Newer file should not match (out of order)" + def change_skin(self): """Test scenarios for changing skin after adding a character.""" self._clear_scene() From 3270d08d3a147a13234a292e08631f22414103ee Mon Sep 17 00:00:00 2001 From: "Patrick W. Crawford" Date: Sat, 25 Mar 2023 08:45:39 -0700 Subject: [PATCH 6/7] Creating a more real integration test for the warden rig selection. This test now correctly fails on older versions of blender, while it passes on 3.0 and up, it fails under 2.90. --- test_files/addon_tests.py | 52 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/test_files/addon_tests.py b/test_files/addon_tests.py index 4d334e38..d7c101fb 100644 --- a/test_files/addon_tests.py +++ b/test_files/addon_tests.py @@ -59,6 +59,7 @@ def __init__(self): self.spawn_mob_linked, self.check_blend_eligible, self.check_blend_eligible_middle, + self.check_blend_eligible_real, self.change_skin, self.import_world_split, self.import_world_fail, @@ -777,6 +778,57 @@ def check_blend_eligible_middle(self): if res is True: return "Newer file should not match (out of order)" + def check_blend_eligible_real(self): + # This order below matches a user's who was encountering an error + # (the actual in-memory python list order) + riglist = [ + "bee - Boxscape.blend", + "Blaze - Trainguy.blend", + "Cave Spider - Austin Prescott.blend", + "creeper - TheDuckCow.blend", + "drowned - HissingCreeper-thefunnypie2.blend", + "enderman - Trainguy.blend", + "Ghast - Trainguy.blend", + "guardian - Trainguy.blend", + "hostile - boxscape.blend", + "illagers - Boxscape.blend", + "mobs - Rymdnisse.blend", + "nether hostile - Boxscape.blend", + "piglin zombified piglin - Boxscape.blend", + "PolarBear - PixelFrosty.blend", + "ravager - Boxscape.blend", + "Shulker - trainguy.blend", + "Skeleton - Trainguy.blend", + "stray - thefunnypie2.blend", + "Warden - DigDanAnimates pre2.80.0.blend", + "Warden - DigDanAnimates pre3.0.0.blend", + "Warden - DiaDanAnimates.blend", + "Zombie - Hissing Creeper.blend", + "Zombie Villager - Hissing Creeper-thefunnypie2.blend" + ] + target_list = [ + "Warden - DigDanAnimates pre2.80.0.blend", + "Warden - DigDanAnimates pre3.0.0.blend", + "Warden - DiaDanAnimates.blend", + ] + + from MCprep.spawner import spawn_util + if bpy.app.version < (2, 80): + correct = "Warden - DigDanAnimates pre2.80.0.blend" + elif bpy.app.version < (3, 0): + correct = "Warden - DigDanAnimates pre3.0.0.blend" + else: + correct = "Warden - DiaDanAnimates.blend" + + for rig in target_list: + res = spawn_util.check_blend_eligible(rig, riglist) + if rig == correct: + if res is not True: + return "Did not pick {} as correct rig".format(rig) + else: + if res is True: + return "Should not have returned correct for {}".format(rig) + def change_skin(self): """Test scenarios for changing skin after adding a character.""" self._clear_scene() From 09bbc5f901c26184e57724c986090e51f2f346cd Mon Sep 17 00:00:00 2001 From: "Patrick W. Crawford" Date: Sat, 25 Mar 2023 09:39:40 -0700 Subject: [PATCH 7/7] Fixed typo in test list. TBH surprised this was the only typo from my image-to-text copy. --- test_files/addon_tests.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test_files/addon_tests.py b/test_files/addon_tests.py index d7c101fb..34b1fb46 100644 --- a/test_files/addon_tests.py +++ b/test_files/addon_tests.py @@ -802,14 +802,14 @@ def check_blend_eligible_real(self): "stray - thefunnypie2.blend", "Warden - DigDanAnimates pre2.80.0.blend", "Warden - DigDanAnimates pre3.0.0.blend", - "Warden - DiaDanAnimates.blend", + "Warden - DigDanAnimates.blend", "Zombie - Hissing Creeper.blend", "Zombie Villager - Hissing Creeper-thefunnypie2.blend" ] target_list = [ "Warden - DigDanAnimates pre2.80.0.blend", "Warden - DigDanAnimates pre3.0.0.blend", - "Warden - DiaDanAnimates.blend", + "Warden - DigDanAnimates.blend", ] from MCprep.spawner import spawn_util @@ -818,7 +818,7 @@ def check_blend_eligible_real(self): elif bpy.app.version < (3, 0): correct = "Warden - DigDanAnimates pre3.0.0.blend" else: - correct = "Warden - DiaDanAnimates.blend" + correct = "Warden - DigDanAnimates.blend" for rig in target_list: res = spawn_util.check_blend_eligible(rig, riglist) @@ -827,7 +827,7 @@ def check_blend_eligible_real(self): return "Did not pick {} as correct rig".format(rig) else: if res is True: - return "Should not have returned correct for {}".format(rig) + return "Should have said {} was correct - not {}".format(correct, rig) def change_skin(self): """Test scenarios for changing skin after adding a character."""