Fix bug in spawn_util #405

CFeeney333 · 2023-03-21T18:19:44Z

Return False not True in order to skip the current file (this_file)

CFeeney333 · 2023-03-21T18:27:59Z

So it seems it was just a case of a logic error. If the version of 'afile' is greater than the blender version, then this is the version that should be used. 'this_file' should be discarded, so False should be returned

This works in blender 2.93.15 now. However, this is the only version that I have tested it with

TheDuckCow · 2023-03-21T22:58:43Z

Wow not sure how that snuck through. I do find it rather confusing though that I can't produce the error locally - but this indeed works for you locally? Can you share a screenshot of your hostile mobs folder to show all files present?

And thanks a million for diving in and proactively making a PR, hoping we can approve and merge this today (just bear with me while I attempt another live test later).

CFeeney333 · 2023-03-21T23:32:46Z

I'm away from the machine where I did the testing, so I can't take screenshots now (it's a college computer). However, I can confirm from memory that there were three warden rigs in the hostile mobs folder - pre-2.8.0, pre-3.0.0 and an unversioned one. Reload mobs worked on blender 3.4, and I downloaded 2.93 specifically to test the error, and that's the screenshot in #403. I can also confirm that after the changes, reload mobs worked in blender 3.3 and 2.93, when I tested it with the changes on my own machine. I can only imagine there's a discrepancy between the rigs we have or the blender versions we're using...

TheDuckCow · 2023-03-22T02:31:39Z

Thanks for sharing the details. Do you remember the detail of whether it was an MCprep upgrade or a fresh install in that 2.93 instance?

I'm looking back, we had this line of code/entire function last modified ahead of the 3.4.0 release - which was the same release that the warden was introduced (#318) with 3 split up files even at that time. Even the lines of code where we call check_blend_eligible was unmodified since that release 8 months ago. I'm really head scratching why we haven't gotten this bug in v3.4.0 and v3.4.1 before, and yet it's such a sudden surge now.

Furthermore, testing this change just based on our unit tests, I actually start getting this assertion to fail in our unit test runner. It's true there could be something wrong with the unit test, but I need to figure out first what is going wrong better.
https://github.com/TheDuckCow/MCprep/blob/master/test_files/addon_tests.py#L718

Also, I head turned slightly - funny that pull/405 is for issue 403, so close to being one and the same!

TheDuckCow · 2023-03-22T03:04:26Z

Alright, I've figured out the culprit - the algorithm is too greedy it seems up front in returning "true, is eligible", and shouldn't really be exiting early until it's tested all files in the list. I'm going to check out your branch and make some contributions, so you can still get the code contribution credit, just be aware I'm making a number of changes.

I'm creating a new unit test called check_blend_eligible_middle which tests this function for this scenario of 2.9x user, when there are 3 files present, the equivalent of pre2.8, pre3.0, and (current live), asserting that it only returns true for the pre3.0 scenario (right now, it's returning true for the "current" one as well, hence blender 2.9x users files are trying to open the post 3.0 versions, as far as I understand it).

Still doesn't explain why this hasn't been an issue till now, but I at least understand where things are going wrong. Could always be the order that files were listed or loaded in rig lists? So it more just happened to work before.

CFeeney333 · 2023-03-22T07:46:45Z

OK, so when I ran it in 2.93, I ran it from pycharm. I cloned the repo to my projects folder. From a while back, I had copied the mcprep_resources from the 3.4.1 addon to this repo. I used a pycharm plugin blend charm to mark the mcprep addon dir as a blender addon which meant I could run/debug the addon from the ide. The plugin installs the addon to the instance of blender you are using when you hit run. So if you were hoping I used a conventional method of running blender, I didn't. It would be curious to see, though, if its just the order of the blend files in the folder that causes some users to get an error as you were saying, or maybe the way different os handle spaces in filenames?

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.

CFeeney333 · 2023-03-22T10:19:26Z

Ok. After another look, I realised that the code only allowed for 1 check for an eligible versioned file, even if there were two. So I guess my previous fix just happened to work because of the order of the files in the list. The new fix should work, regardless of the order of the files, as it was originally intended to do. At this stage, I wouldn't be surprised at all if the error had only popped up depending on the order of the files in the list. I wonder if a simple sort() would have worked as a hacky fix

MCprep_addon/spawner/spawn_util.py

StandingPadAnimations

Mostly some stuff regardding comments, but otherwise looks good

TheDuckCow · 2023-03-22T22:23:09Z

Thanks for reviewing, and @CFeeney333 for making further changes. I'm going to ask to hold on for a bit until I can tack on a couple changes I've made and am continuing on with locally, I agree with your assessment of it being more chance that it was working at all before.

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.

TheDuckCow · 2023-03-23T05:12:35Z

Hey @CFeeney333 can you give me write/push access to your MCprep fork? Then I can push my one commit up and we can merge, and you'll appear in the history correctly.

TheDuckCow

So of course I'd approve the changes I've made. Seeking input from @StandingPadAnimations

I don't like how complicated I made this function become, but even after spending some time looking at it, I didn't see a way to (substantially) simplify it. We have to look at all filenames in the list before we can determine if this_file should return true. That was really the crux of the issue before, it was returning true (I imagine) for the warden.blend when it hadn't yet checked to see if it should prefer warden pre3.0.0.blend. Unit tests now demonstrate this working as intended.

StandingPadAnimations · 2023-03-23T14:23:27Z

MCprep_addon/spawner/spawn_util.py

-	# If no matches (the most common case), then this file is eligible.
-	return True
+			res = util.min_bv(tuple_ver, inclusive=True)
+			if res is False:


inclusive is set to True by default

Yup, I spent a lot of time starting and it and it ended up helping me read it having the argument specifically there. I nearly just removed the function call in favor of bpy.app.version to make it easier to read, but we can do that in the 3.5 release (even though bpy.app.version is safely available in 2.79, no reason to break the compatibility lower just for this).

StandingPadAnimations · 2023-03-23T14:24:13Z

MCprep_addon/spawner/spawn_util.py

+	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:


Same thing here

StandingPadAnimations

I'll have to look further in depth later today, but here's the things I found at this moment

TheDuckCow · 2023-03-25T02:40:54Z

FYI @StandingPadAnimations likely to merge tomorrow, no sweat if you don't have time. Happy to hear input too from you @CFeeney333 to verify this still makes it work well on your side, that would actually be a good sanity check since I still have yet to produce the error myself.

StandingPadAnimations · 2023-03-25T02:44:06Z

Whoops, forgot about this. Looking over it again I think it should be fine to merge (we can ignore min_bv for now, I'll be doing some version check changes in 3.5 anyway)

CFeeney333 · 2023-03-25T09:18:51Z

2023-03-25.09-02-29.mp4

Ig this is the quickest way to verify. My dev branch was updated with your changes before recording that, and blender 3.3.5 works with both branches

order of blend files in list (2.93, dev branch)

TheDuckCow · 2023-03-25T15:32:02Z

Darn, even with my unittest still not working. I'm quite sure though we still need to loop over files. Thanks for the screenshot, I've extract that and will do some more testing on it today, creating or updating the test script.

This test now correctly fails on older versions of blender, while it passes on 3.0 and up, it fails under 2.90.

TheDuckCow · 2023-03-25T15:50:09Z

Alright, this is progress at least: The unit tests now fail for the expected versions of blender, consistent with your screen recording (even though blender 2.93 still works fine for me). Next stop: Actually making the function work as expected.

Test results csv

blender	failed_test	short_err
(3, 5, 0)	ALL PASSED	-
(3, 4, 0)	ALL PASSED	-
(3, 2, 1)	ALL PASSED	-
(3, 3, 1)	ALL PASSED	-
(3, 1, 0)	ALL PASSED	-
(3, 0, 0)	ALL PASSED	-
(2, 93, 0)	check_blend_eligible_real	DiaDanAnimates.blend
(2, 90, 1)	check_blend_eligible_real	DiaDanAnimates.blend
(2, 80, 75)	check_blend_eligible_real	DiaDanAnimates.blend

This is after running the command ./run_tests.sh -all -run check_blend_eligible_real

I realize the "run all" control does not exist yet in the windows bat script, so one would have to manually specify the first entry in their blender_execs.txt file to make it run that version of blender. For context, this is my "blender_execs.txt" file, and hence these are the versions that get tested when I run the script above.

/Applications/blender 3.5/Blender.app/Contents/MacOS/Blender
/Applications/blender 3.4/Blender.app/Contents/MacOS/Blender
/Applications/blender 3.2/Blender.app/Contents/MacOS/Blender
/Applications/blender 3.3/Blender.app/Contents/MacOS/Blender
/Applications/blender 3.1/Blender.app/Contents/MacOS/blender
/Applications/blender 3.0/Blender.app/Contents/MacOS/Blender
/Applications/blender 2.93/Blender.app/Contents/MacOS/Blender
/Applications/blender 2.90/Blender.app/Contents/MacOS/Blender
/Applications/blender 2.80/Blender.app/Contents/MacOS/Blender

CFeeney333 · 2023-03-25T16:00:27Z

Wait, the occasion that the exception occurs is the code without the changes. You are aware of that? ie. doesn't that mean the function now works as expected? (The exception occurs on my master branch as I hadn't merged the changes)

I just noticed in the test results it says the rig is by 'DiaDanAnimates' and not 'DigDanAnimates'. Does that mean anything?

TBH surprised this was the only typo from my image-to-text copy.

TheDuckCow · 2023-03-25T16:52:08Z

Alright I take it back, the only reason my tests were failing was because I had a typo in the rig list I copied from you (my image-to-text reader picked up a instead of g in one spot). Once that typo was fixed, all tests now pass. And this new test is a live integration test essentially, opening in multiple versions of blender and validating the right file is picked. I've also checked in the UI that in 2.93 for me it's picking up the right one (screenshot below).

@CFeeney333 I'm really hoping this is somehow it was not working on your machine due to __pycache__ files or something like that in the addons directory. Can you try completely uninstalling MCPrep (delete the addon from the addons folder entirely), restart, and then re-open again after having done a git pull once more? (I technically only changed test files though, but just to be safe).

I don't want to push this release out if we think this isn't working. And I know the original logic was flawed for sure, it needs to operate over the entire list before checking if a given file is the right one. So even though your initial change was working for you, I believe it's still only by chance in essence (no different to how it was working before). Though let me know if you somehow see it differently and I'm misreading all of this still somehow.

TheDuckCow · 2023-03-25T16:53:02Z

Wait, the occasion that the exception occurs is the code without the changes. You are aware of that? ie. doesn't that mean the function now works as expected? (The exception occurs on my master branch as I hadn't merged the changes)

I just noticed in the test results it says the rig is by 'DiaDanAnimates' and not 'DigDanAnimates'. Does that mean anything?

OH just now saw this, totally misunderstood that! So, to your understanding, it's all working now? And yes, twas a typo.

CFeeney333 · 2023-03-25T17:01:26Z

No worries. Yes, it's all working now... sorry for any confusion

Fix bug in spawn_util

1cf3335

Return False not True in order to skip the current file (this_file)

StandingPadAnimations requested a review from TheDuckCow March 21, 2023 18:37

StandingPadAnimations approved these changes Mar 21, 2023

View reviewed changes

CFeeney333 mentioned this pull request Mar 21, 2023

Warden causing Reload Mob errors in 2.8x and 2.9x #403

Closed

StandingPadAnimations linked an issue Mar 21, 2023 that may be closed by this pull request

Warden causing Reload Mob errors in 2.8x and 2.9x #403

Closed

TheDuckCow added this to the v3.4.3 milestone Mar 21, 2023

CFeeney333 requested review from StandingPadAnimations and removed request for TheDuckCow March 22, 2023 10:19

StandingPadAnimations reviewed Mar 22, 2023

View reviewed changes

MCprep_addon/spawner/spawn_util.py Outdated Show resolved Hide resolved

StandingPadAnimations reviewed Mar 22, 2023

View reviewed changes

MCprep_addon/spawner/spawn_util.py Outdated Show resolved Hide resolved

StandingPadAnimations requested changes Mar 22, 2023

View reviewed changes

CFeeney333 added 2 commits March 22, 2023 20:31

Remove commented out code

323028e

Wrap long comments

a51f45b

TheDuckCow self-requested a review March 22, 2023 22:23

TheDuckCow added 2 commits March 22, 2023 22:06

Fixing check_blend_eligible and adding more tests

1f6c957

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.

Merge remote-tracking branch 'origin/dev' into dev

8cb4d75

TheDuckCow approved these changes Mar 23, 2023

View reviewed changes

TheDuckCow requested a review from StandingPadAnimations March 23, 2023 14:19

StandingPadAnimations reviewed Mar 23, 2023

View reviewed changes

StandingPadAnimations requested changes Mar 23, 2023

View reviewed changes

StandingPadAnimations approved these changes Mar 25, 2023

View reviewed changes

Creating a more real integration test for the warden rig selection.

3270d08

This test now correctly fails on older versions of blender, while it passes on 3.0 and up, it fails under 2.90.

Fixed typo in test list.

09bbc5f

TBH surprised this was the only typo from my image-to-text copy.

TheDuckCow merged commit 7585b6f into Moo-Ack-Productions:dev Mar 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug in spawn_util #405

Fix bug in spawn_util #405

CFeeney333 commented Mar 21, 2023

CFeeney333 commented Mar 21, 2023

TheDuckCow commented Mar 21, 2023

CFeeney333 commented Mar 21, 2023

TheDuckCow commented Mar 22, 2023

TheDuckCow commented Mar 22, 2023

CFeeney333 commented Mar 22, 2023 •

edited

Loading

CFeeney333 commented Mar 22, 2023 •

edited

Loading

StandingPadAnimations left a comment

TheDuckCow commented Mar 22, 2023

TheDuckCow commented Mar 23, 2023

TheDuckCow left a comment •

edited

Loading

StandingPadAnimations Mar 23, 2023

TheDuckCow Mar 23, 2023

StandingPadAnimations Mar 23, 2023

StandingPadAnimations left a comment

TheDuckCow commented Mar 25, 2023

StandingPadAnimations commented Mar 25, 2023

CFeeney333 commented Mar 25, 2023 •

edited

Loading

TheDuckCow commented Mar 25, 2023

TheDuckCow commented Mar 25, 2023

CFeeney333 commented Mar 25, 2023 •

edited

Loading

TheDuckCow commented Mar 25, 2023

TheDuckCow commented Mar 25, 2023

CFeeney333 commented Mar 25, 2023 •

edited

Loading

Fix bug in spawn_util #405

Fix bug in spawn_util #405

Conversation

CFeeney333 commented Mar 21, 2023

CFeeney333 commented Mar 21, 2023

TheDuckCow commented Mar 21, 2023

CFeeney333 commented Mar 21, 2023

TheDuckCow commented Mar 22, 2023

TheDuckCow commented Mar 22, 2023

CFeeney333 commented Mar 22, 2023 • edited Loading

CFeeney333 commented Mar 22, 2023 • edited Loading

StandingPadAnimations left a comment

Choose a reason for hiding this comment

TheDuckCow commented Mar 22, 2023

TheDuckCow commented Mar 23, 2023

TheDuckCow left a comment • edited Loading

Choose a reason for hiding this comment

StandingPadAnimations Mar 23, 2023

Choose a reason for hiding this comment

TheDuckCow Mar 23, 2023

Choose a reason for hiding this comment

StandingPadAnimations Mar 23, 2023

Choose a reason for hiding this comment

StandingPadAnimations left a comment

Choose a reason for hiding this comment

TheDuckCow commented Mar 25, 2023

StandingPadAnimations commented Mar 25, 2023

CFeeney333 commented Mar 25, 2023 • edited Loading

TheDuckCow commented Mar 25, 2023

TheDuckCow commented Mar 25, 2023

CFeeney333 commented Mar 25, 2023 • edited Loading

TheDuckCow commented Mar 25, 2023

TheDuckCow commented Mar 25, 2023

CFeeney333 commented Mar 25, 2023 • edited Loading

CFeeney333 commented Mar 22, 2023 •

edited

Loading

CFeeney333 commented Mar 22, 2023 •

edited

Loading

TheDuckCow left a comment •

edited

Loading

CFeeney333 commented Mar 25, 2023 •

edited

Loading

CFeeney333 commented Mar 25, 2023 •

edited

Loading

CFeeney333 commented Mar 25, 2023 •

edited

Loading