Internal Refactoring for 3.4.3

[StandingPadAnimations]

List of Deprecated Features Planned for MCprep 3.4.3

State: Alpha, changes are not final

conf.dev -> conf.env.dev_build

from .conf import env
env.dev_build

conf.v -> conf.env.verbose

from .conf import env
env.verbose

conf.vv -> conf.env.very_verbose

from .conf import env
env.very_verbose

conf.data -> conf.env.data

from .conf import env
env.data

conf.json_data -> conf.env.json_data

from .conf import env
env.json_data

conf.json_path -> conf.env.json_path

from .conf import env
env.json_path

conf.use_icons -> conf.env.use_icons

from .conf import env
env.use_icons

conf.preview_collections -> conf.env.preview_collections

from .conf import env
env.preview_collections

conf.icons_init() -> conf.env.icons_init()

from .conf import env
env.icons_init()

conf.loaded_all_spawners -> conf.env.loaded_all_spawners

from .conf import env
env.loaded_all_spawners

conf.skin_list -> conf.env.skin_list

from .conf import env
env.skin_list 

conf.rig_categories -> conf.env.rig_categories

from .conf import env
env.rig_categories 

conf.entity_list -> conf.env.entity_list

from .conf import env
env.entity_list

conf.material_sync_cache -> conf.env.material_sync_cache

from .conf import env
env.material_sync_cache

conf.register() ->conf.MCprepEnv.__init__(self)

# conf.py
env = MCprepEnv()

conf.log(statement, vv_only=False) -> conf.env.log(self, statement, vv_only=False)

from .conf import env
env.log("Debug statement!")

[TheDuckCow]

Realize this is still in review, but calling out now that I'd prefer the name lower-case ENV instead of uppercase; uppercase is generally referred to treat something as a constant, but env is something that will be mutable (verbose can be turned off and on for instance), so will ask you to turn it into lowercase when you can.

Holding off on further review in the meantime - thanks for getting a head start here!

[StandingPadAnimations]

Realize this is still in review, but calling out now that I'd prefer the name lower-case ENV instead of uppercase; uppercase is generally referred to treat something as a constant, but env is something that will be mutable (verbose can be turned off and on for instance), so will ask you to turn it into lowercase when you can.

Sure thing, maybe something like mcprep_env since env is typically used for .env files.

One interesting thing I might add to MCprepEnv is checking the version of Blender the user is using at initialization. This would make the bv28() and bv30() functions obsolete in favor of some variables that are set when Blender loads MCprep (minBV() would still have a use for arbitrary versions of Blender that aren't 2.8 or 3.0). Since MCprepEnv is a class, there's a whole load of interesting stuff that can be done with it.

[TheDuckCow]

Yup that makes sense to have those vars initialized in __init__; we can do a number of interesting things indeed, though we want to make sure that it stays lightweight.

As for the name of the variable instance, I'd like a convention for naming that is short and easy to use in the other classes. mcprep_env feels a bit long, I feel like we could use env since it's not used anywhere else. We don't have any env files at the moment, and even if we did, it would be read and parse by the MCprepEnv class anyways, so it feels accurate enough that you would call the instance overall as env anyhow.

[StandingPadAnimations]

Sorry for the lack of updates, hopefully I'll be able to get some changes done by late December/early January. I'll be busy for the next couple of weeks

[StandingPadAnimations]

Alright, @TheDuckCow, I've renamed ENV to env. Hopefully by the end of this week I can get some more progress done on this

[StandingPadAnimations]

As mentioned here, I won't be working on MCprep anymore, so this PR will just be left in this state.

I believe this will eventually get done in the future, either by the open source community or myself (if I decide to come back), but who knows

[StandingPadAnimations]

Ok, so I don't know if I'll reopen this, but I'm curious about whether we'll use type annotations (which are supported in Blender 2.8 and above), since it would give the following benefits:

• Better completions in IDES
• More info for static analyzers, which MCprep would massively benefit from
• Tells developers what arguments a function can take and return
• etc

Alternatively we could add type annotations at a later date (not preferable, but better late then never) or figure out a way to use it in older versions of Blender (perhaps with the futures module?)

[StandingPadAnimations]

I think now I'll be working on this. I'll look into porting the use of OS to Pathlib (for file operations) since it's more readable and concise

[StandingPadAnimations]

Though now I got to sign all of these commits for verification purposes. Not too annoying though since it's not merged

[StandingPadAnimations]

Alright with #399 (and this PR is for MCprep 3.5), I think it's now safe to deprecate the use of os for filepaths and file operations, as well as add type annotations to the env class

[StandingPadAnimations]

Alright, I'll merge dev with this locally (for updates) ajd get started. I'll be doing the following:

• Add a deprecation warning function
• Deprecate 2.7x compatability code
• Remove 2.7x compatablity code where possible
• Add type hints to functions and variables

@TheDuckCow just want to know your thoughts

[TheDuckCow]

Just to be clear, you mean merge dev into this branch just to sync it up right?

I'm ok with this. We can use internal-rewrites as an actual "feature branch". I don't want to merge breaking compatibility into dev yet, as we may have more patch releases between now and 3.5 (we will for sure have at least a 3.5.2, based on your report about mtl conversion errors). But it would be good to get a head start on some of these updates.

I'd start with removing 2.7x compatibility code, anything to do with Blender Internal.

I'm wary about adding type hints everywhere just yet. It will just create more merge conflicts in the future if we annotate every function, whereas if we do this a little closer to the 3.5 release, we'd deal with the state of the code at that time. But this could also be mitigated by diligently re-merging dev into this internal-rewrites feature branch.

[StandingPadAnimations]

Just to be clear, you mean merge dev into this branch just to sync it up right?

Yep

I'd start with removing 2.7x compatibility code, anything to do with Blender Internal.

Sure thing

[StandingPadAnimations]

Alright, went ahead and made a new branch called mcprep-3_5-recore (recore being short for rewritten core, as many parts are being rewritten)

[StandingPadAnimations]

So should we move these deprecations to MCprep 3.5?

[TheDuckCow]

I'm fine to proceed with merging these changes to a dedicated MCprep 3.5 branch (not Dev, but newly branched from current dev)Let me set this up later when I have time (give me a day or so). Idea is it will be a branch we treat similar to dev. We merge approved PR changes that are 3.5x only into that, and then only when we are totally ready to move ahead with 3.5x code (ie we know we won't need any further patch releases), then we merge this mcprep35 branch into dev. Until then, I'll be diligent and merge dev changes back into this mcprep35 branch so it doesn't get too out of sync.On Apr 6, 2023 9:45 AM, StandingPad ***@***.***> wrote: So should we move these deprecations to MCprep 3.5? —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

[TheDuckCow]

I've updated the base since this branch as-is doesn't work in 2.7x (first error is print(f"\"{header}\"")), and this is ok. I'm hopeful we can make 3.5.0 the next release anyways.

Some changes still needed though, errors on startup with I think some remaining ENV variables where the class instance is now named env (which I think is good). e.g.

  File "/Users/patrickcrawford/Library/Application Support/Blender/3.5/scripts/addons/MCprep_addon/mcprep_ui.py", line 934, in draw
    elif env.skin_list and len(ENV.skin_list) <= sind:
NameError: name 'ENV' is not defined. Did you mean: 'env'?

and

Traceback (most recent call last):
  File "/Applications/Blender 3.5/blender.app/Contents/Resources/3.5/scripts/modules/addon_utils.py", line 369, in enable
    mod.register()
  File "/Users/patrickcrawford/Library/Application Support/Blender/3.5/scripts/addons/MCprep_addon/__init__.py", line 64, in register
    load_modules.register(bl_info)
  File "/Users/patrickcrawford/Library/Application Support/Blender/3.5/scripts/addons/MCprep_addon/load_modules.py", line 181, in register
    addon_updater_ops.register(bl_info)
  File "/Users/patrickcrawford/Library/Application Support/Blender/3.5/scripts/addons/MCprep_addon/addon_updater_ops.py", line 1360, in register
    from .conf import ENV
ImportError: cannot import name 'ENV' from 'MCprep_addon.conf' (/Users/patrickcrawford/Library/Application Support/Blender/3.5/scripts/addons/MCprep_addon/conf.py)

And some others.

Once we resolve these, I think it would be good to merge into the newly-assigned feature branch, so that other changes can be reviewed on their own.

[StandingPadAnimations]

I've updated the base since this branch as-is doesn't work in 2.7x (first error is print(f"\"{header}\"")), and this is ok. I'm hopeful we can make 3.5.0 the next release anyways.

I think we can ignore that if we merge this for MCprep 3.5

Some changes still needed though, errors on startup with I think some remaining ENV variables where the class instance is now named env (which I think is good). e.g.

Whoops, looks like I missed some places, good catch

Once we resolve these, I think it would be good to merge into the newly-assigned feature branch, so that other changes can be reviewed on their own.

The MCprep 3.5 Recore branch or a new branch? If the latter, it might be easier to just keep them here.

…

On 0406, Patrick W. Crawford wrote: I've updated the base since this branch as-is doesn't work in 2.7x (first error is print(f"\"{header}\"")), and this is ok. I'm hopeful we can make 3.5.0 the next release anyways. Some changes still needed though, errors on startup with I think some remaining ENV variables where the class instance is now named env (which I think is good). e.g. File "/Users/patrickcrawford/Library/Application Support/Blender/3.5/scripts/a ddons/MCprep_addon/mcprep_ui.py", line 934, in draw elif env.skin_list and len(ENV.skin_list) <= sind: NameError: name 'ENV' is not defined. Did you mean: 'env'? and Traceback (most recent call last): File "/Applications/Blender 3.5/blender.app/Contents/Resources/3.5/scripts/mod ules/addon_utils.py", line 369, in enable mod.register() File "/Users/patrickcrawford/Library/Application Support/Blender/3.5/scripts/a ddons/MCprep_addon/__init__.py", line 64, in register load_modules.register(bl_info) File "/Users/patrickcrawford/Library/Application Support/Blender/3.5/scripts/a ddons/MCprep_addon/load_modules.py", line 181, in register addon_updater_ops.register(bl_info) File "/Users/patrickcrawford/Library/Application Support/Blender/3.5/scripts/a ddons/MCprep_addon/addon_updater_ops.py", line 1360, in register from .conf import ENV ImportError: cannot import name 'ENV' from 'MCprep_addon.conf' (/Users/patrickcr awford/Library/Application Support/Blender/3.5/scripts/addons/MCprep_addon/conf. py) And some others. Once we resolve these, I think it would be good to merge into the newly-assigned feature branch, so that other changes can be reviewed on their own. -- Reply to this email directly, [1]view it on GitHub, or [2]unsubscribe. You are receiving this because you were mentioned. Message ID: ***@***.***> References 1. #348 (comment) 2. https://github.com/notifications/unsubscribe-auth/AR4UXCRQQHLMXCKFOY4XWOLW73W3DANCNFSM6AAAAAARGRBB4E

[StandingPadAnimations]

Now that there's a milestone branch for 3.5, should we merge this branch?

[TheDuckCow]

Now that there's a milestone branch for 3.5, should we merge this branch?

Let's fix the couple things mentioned where you missed first, and resolve the conflicts. I'll take one more quick look then, and then yes we can merge into the milestone branch.

[StandingPadAnimations]

Alright, fixed the remaining uses of ENV and also removed the use of conf.load() in load_modules.py (I love the new deprecation warnings I've added). I've also updated the deprecation warning to say 3.5.1 (I'm 50% we'll just push back the removal, but at the very least we should start throwing around deprecation warnings)

@TheDuckCow just need some final checks on this to make sure everything is working as expected

[StandingPadAnimations]

Alright I've now pushed the dev build feature to compile.sh. Just know that -fast has also now become -f

[StandingPadAnimations]

Ok I just realized that we also have #401, which makes this branch a bit weird (#401 is also based off of this). Should we merge this completely with recore or should we merge this with the milestone branch and then merge recore later on?

I'm leaning more towards the latter so that the changes are more incremental, but I want to know everyone's thoughts

[TheDuckCow]

Ok I just realized that we also have #401, which makes this branch a bit weird (#401 is also based off of this). Should we merge this completely with recore or should we merge this with the milestone branch and then merge recore later on?

I'm leaning more towards the latter so that the changes are more incremental, but I want to know everyone's thoughts

It's not a problem, both can be merged into milestone-3-5-0, in whichever order (both will likely have a couple merge conflicts, so dealers choice which you want to deal with in which order). So I guess the latter of your two options is what I agree with. The sooner we merge this one though, the fewer problems we'll have. If we need to make future fixes, they can be branched off of the milestone-3-5-0 and PR-merged back into the same milesotne-3-5-0

[StandingPadAnimations]

Closing this as this is no longer needed with #401