MCprep 3.5 Rewritten Core #401
Conversation
As we're dropping 2.7x support, this is no longer needed. Most of the compatability functions will remain though since they're more readable.
Replaced some instances of os.path with the Path object from pathlib for readability. Also added type annotations to MCprepEnv as we're moving towards that. This also means MCprep will not work at all in Blender 2.7x due to the use of new syntax. This will be the first in a long process of modernizing MCprep's code with 2.8 style code. Blender 2.7x users may not be happy, but it's for the better. If anything, 6 years worth of 2.7x support was a mistake (in my opinion), as it limited what we could do and opened MCprep to even more bugs (like in Blender 2.93 with make_annotations, which ironically now is deprecated).
Replaced the old = syntax with the new : syntax. This also removes the need for make_annotations, hence why they're removed.
As we're removing 2.7x support, it makes sense to remove Blender Internal compatability as the engine is not present in Blender 2.8 and above. Even UPBGE (a fork of the Blender game engine that's still maintained) uses EEVEE, so there's no reason to continue supporting Blender Internal.
The bang at the top meant Linux users couldn't run the test suite. This changes now allows Linux users to run tests
|
Btw @TheDuckCow, what errors did you get when testing? I can't run the test suite on my laptop (due to some OSX specific parts of the testing code) |
|
I think for this branch I'll also start simplifying the code. For example, the following function: def matprep_cycles(
mat, passes, use_reflections, use_principled, only_solid, pack_format, use_emission_nodes)Could be adjusted as: @dataclass
class PrepOptions:
passes: dict[str]
use_reflections: bool
use_principled: bool
only_solid: bool
pack_format: str
use_emission_nodes: bool
def matprep_cycles(mat, options: PrepOptions) |
Deprecation warning function was moved in MCprepEnv since it made more sense to put it there
Originally matprep_cycles used to take multiple arguments, each one representing a seperate option. This was annoying as it meant the function has an unfathomable amount of arguments to keep track of. This has been replaced with a dataclass (called PrepOptions) that keeps track of all the options selected by the user. Not only does it make it slightly easier for developers, it also gives the following benefits: - More info on what types the different options are - Better organization Previously we couldn't do this due to 2.7x support, hence this being in mcprep-3_5-recore.
To avoid polluting output
To avoid polluting output
|
Yeah I think we should do that. It would also give developers the benefit of the new changes. Just one problem: the addon updater is complaining a lot: Traceback (most recent call last):
File "/home/mahid/.config/blender/3.5/scripts/addons/MCprep_addon/mcprep_ui.py", line 687, in draw
addon_updater_ops.check_for_update_background()
File "/home/mahid/.config/blender/3.5/scripts/addons/MCprep_addon/addon_updater_ops.py", line 816, in check_for_update_background
updater.set_check_interval(enabled=settings.auto_check_update,
File "/home/mahid/.config/blender/3.5/scripts/addons/MCprep_addon/addon_updater.py", line 558, in set_check_interval
raise ValueError("Enable must be a boolean value")
ValueError: Enable must be a boolean value
Traceback (most recent call last):
File "/home/mahid/.config/blender/3.5/scripts/addons/MCprep_addon/mcprep_ui.py", line 1623, in draw
addon_updater_ops.check_for_update_background()
File "/home/mahid/.config/blender/3.5/scripts/addons/MCprep_addon/addon_updater_ops.py", line 816, in check_for_update_background
updater.set_check_interval(enabled=settings.auto_check_update,
File "/home/mahid/.config/blender/3.5/scripts/addons/MCprep_addon/addon_updater.py", line 558, in set_check_interval
raise ValueError("Enable must be a boolean value")
ValueError: Enable must be a boolean value
Traceback (most recent call last):
File "/home/mahid/.config/blender/3.5/scripts/addons/MCprep_addon/mcprep_ui.py", line 687, in draw
addon_updater_ops.check_for_update_background()
File "/home/mahid/.config/blender/3.5/scripts/addons/MCprep_addon/addon_updater_ops.py", line 816, in check_for_update_background
updater.set_check_interval(enabled=settings.auto_check_update,
File "/home/mahid/.config/blender/3.5/scripts/addons/MCprep_addon/addon_updater.py", line 558, in set_check_interval
raise ValueError("Enable must be a boolean value")
ValueError: Enable must be a boolean value
Traceback (most recent call last):
File "/home/mahid/.config/blender/3.5/scripts/addons/MCprep_addon/mcprep_ui.py", line 1623, in draw
addon_updater_ops.check_for_update_background()
File "/home/mahid/.config/blender/3.5/scripts/addons/MCprep_addon/addon_updater_ops.py", line 816, in check_for_update_background
updater.set_check_interval(enabled=settings.auto_check_update,
File "/home/mahid/.config/blender/3.5/scripts/addons/MCprep_addon/addon_updater.py", line 558, in set_check_interval
raise ValueError("Enable must be a boolean value")
ValueError: Enable must be a boolean value |
|
@StandingPadAnimations Can you try first merging the base milestone branch into this to resolve issues? Then should be quick to resolve anything else like if something got messed up with updater preferences (I have no other reports spearately that there's anything wrong in blender 3.5 and the updater). If you needed, I could also help out with this step. But the conflicts cover changes you made, so I feel you're a better judge of how to do the merge. Then I can swoop in to fix whatever with the updater here. |
|
Just one problem: the world panel is not appearing D: No errors in the console as far as I can tell, nor any UI changes that would suggest an issue. Anyway do you also want me to close the |
Additionally removes all usage of the make_annotations workaround, this commit does not affect the updater code in any way (which is treated as a library). Preferences now draw, but mcprep scene props are not currently registering, breaking most of the addon. Pre-existing before this commit.
There was a problem hiding this comment.
Given this PR is blocked on this now, @StandingPadAnimations we'll want to prioritize getting this ready and merged.
I've made a commit which adds what I believe are the remaining annotations, so we are fully moving to that and dropping the make_annotations workaround. Preferences initially wasn't working, now it is. However, something is still causing the bpy.types.Scene.mcprep_props to fail or otherwise not actually register, as I am getting UI draw errors saying:
Traceback (most recent call last):
File "/Users/patrickcrawford/Library/Application Support/Blender/3.5/scripts/addons/MCprep_addon/mcprep_ui.py", line 908, in draw
scn_props = context.scene.mcprep_props
AttributeError: 'Scene' object has no attribute 'mcprep_props'
I think this still may be annotation related - I recall errors regarding that were typically quite cryptic and misleading (I don't see any registration errors).
I'll keep debugging too and see what I can find, but feel free to try to beat me to it.
MCprep scene props still not registering, so still unable to run automated tests.
MCprep scene props still not registering, so still unable to run automated tests.
With this change, the UI now draws where needed. Many unit tests are still failing.
|
Alright, now we're in better shape. All UI seems to draw, though many of the unit tests are still failing, something we'll need to look into. Some live testing didn't show anything terribly off, could just be test code needs updating. @StandingPadAnimations let me know once you do some testing on this branch yourself (after doing a git pull), and see if it's looking good for you aside from these unit tests. If so, I'd say we can merge and then I can tackle the unit test fixing separately. |
If that's indeed the case, those changes are fully here, then sure you can close that PR. |
Why? Poetry means we can easily define Python version, dependencies, etc. without having to force developers to do everything by hand (provided they want to use Poetry, it's fine if they don't c:). The reason I'm doing it now is due to an upgrade on Arch Linux from Python 3.10 to Python 3.11, breaking all virtual environments. Poetry is useful especially on Linux where using the system Python tends to be an annoyence. The options I chose are based on MCprep 3.5, which mean the following: - Blender 2.8 bpy - Python 3.7 (as that's what 2.8 uses) These are not set in stone, but I think those are reasonable for MCprep
|
@TheDuckCow I've added a It's just for the development environment, not runtime |
The MC part was not capitalized
Beyond the deprecation warnings regarding the use of |
|
Ahh yes: factor_width = 0.3
if util.bv28():
factor_width = 0.3 |
By removing all of the `bv28()` calls, we've gotten rid of all deprecation warnings at startup and exit, which is awesome. Now we can actually start live testing to find even more deprecation warnings without having to deal with the insane output for the UI.
|
I've removed all deprecation warnings at startup and exit, which means now we can now start improving and testing the codebase (and live test to find more use of deprecated functions at the same time) I think we can now perform the first merge with the milestone branch |
Previouly Linux didn't have file population for the MCprep compile scripts, so I've went ahead and added it to make our lives easier, especially after that time an MCprep build script made a folder called ~ that ended in my home directory being wiped
The compile scripts had some small errors (like a missing exit after cd) but also 2 big errors: SC2308 and SC2115 https://www.shellcheck.net/wiki/SC2308 https://www.shellcheck.net/wiki/SC2115 SC2308 was due to the use of `expr substr` when checking to see if the OS in use was Linux. According to the shellcheck wiki, this is not portable and as such shouldn't be used. This was replaced with a pipe expression that passes to `cut -c 1-5` SC2115 is due to an `rm -rf "$i/$NAME/" statement, which if the variable is not set or null, a person's system can be massively messed up. Here's a case where due to this exact issue, the Steam client ended up deleting everything on the root directory owned by the user: ValveSoftware/steam-for-linux#3671 While it's great that these issues were caught ahead of time, these should have never been in MCprep's build scripts. We already know that MCprep's build scripts can create folders named ~, causing their own headaches (like I dealt with a while back). We seriously need to start enforcing shellcheck for MCprep's build scripts.
|
Pushed some fixes for Tldr: We were using some syntax that if done wrong could have done some serious damage to a user's system (on Linux)
|
Great to have these fixes - but let's try to not expand the scope of this PR any more than it already has. It would be better to merge this into Even changes like the So, let's do no more new changes on this branch. I need to spot check the recent changes/additions to see if anything needs addressing, and then based on my approval, let's merge, and create new branches PRs off of newly updated |
| @@ -915,7 +893,7 @@ class MCPREP_PT_world_tools(bpy.types.Panel): | |||
| """World settings and tools""" | |||
| bl_label = "World Tools" | |||
| bl_space_type = 'VIEW_3D' | |||
| bl_region_type = 'TOOLS' if not util.bv28() else 'UI' | |||
| bl_region_type = 'UI' | |||
There was a problem hiding this comment.
Nit on extra space at end of line. Not that there aren't more egregious code style issues pre-existing 😆
There was a problem hiding this comment.
I'm considering adding https://github.com/akaihola/darker in a later PR so that we can gradually add better formatting, so hopefully this will be resolved in the future
| @@ -1713,7 +1688,7 @@ class MCPREP_PT_mob_spawner(bpy.types.Panel): | |||
| bl_label = "Mob spawner" | |||
| bl_parent_id = "MCPREP_PT_spawn" | |||
| bl_space_type = "VIEW_3D" | |||
| bl_region_type = 'TOOLS' if not util.bv28() else 'UI' | |||
| bl_region_type = 'UI' | |||
| @@ -0,0 +1,18 @@ | |||
| # This file is automatically @generated by Poetry 1.4.2 and should not be changed by hand. | |||
There was a problem hiding this comment.
What's the consequence of this file if, for example, you were to step away and I didn't update this with future updates? Will this file need to change with releases, or is it expected to remain mostly static? Would these hashes be universal, or system specific to you?
There was a problem hiding this comment.
According to the poetry documentation, the hashes are based on the dependencies. It's automatically handled by poetry
|
Ight, hopefully I'll merge this later today |
Alright, this is a new branch tracking deprecations and migrating 2.7x style code to 2.8. The term "Rewritten Core" I think acts as a nice theme for this migration, as it requires going into parts of MCprep that have long been forgotten about.
Faq (for anyone that stumbles apon this)
Plans
PropertyGroupsto use annotationsoswithpathlibfor file operationsDeprecations
util.make_annotations(cls)andutil.bv28()Dropped as 2.7x support will also be dropped. Just use type annotations for properties instead of the old 2.7x syntax and don't target 2.7x when writing code.
Docstrings for type info
Made redundent by type annotations. Docstrings are nice for explaining what each return value means, but type info should be done with type annotations.
Note that type annotations must be Python 3.7 compatible for maximum support. Also we won't be removing all docstring-based type information from older code, just from the functions that gain annotations.