Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Annotate documment functions #425

Merged

Conversation

zNightlord
Copy link
Contributor

Based on #418

Copy link
Member

@TheDuckCow TheDuckCow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't go all the way through, but did notice we have breakage on blender 2.80 with some of the typing used. TL;DR will need to replace any list, dict, tuple's with the variants from the typing module. Might be easier if you manage to test with a version of blender 2.80 on your machine, as startup errors tell you right away where anything may still be a problem :)

@@ -0,0 +1,362 @@
### Utilities
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either the filename or location seems off. README generally refers to a user, whereas CONTRIBUTING.md in the root level is for developers. Since this is developer documentation, I think it should probably live here and call it dev utils, but open to other ideas.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay I rename and move it there for now.

MCprep_addon/conf.py Outdated Show resolved Hide resolved
MCprep_addon/materials/default_materials.py Outdated Show resolved Hide resolved
@zNightlord
Copy link
Contributor Author

I think the List situation is good now, unless I miss some.

Is it just me or mcprep_ui has some issues with register properties?
I have to comment out default=addon_prefs.<attr> line for it to stop prompting error

@zNightlord zNightlord requested a review from TheDuckCow May 11, 2023 10:30
With the recent change in 4.0, it's clear that most addon developers do
not keep track of deprecations and breaking changes. Therefore, it makes
sense that MCprep should have a public list of deprecations and breaking
changes.

This list only goes back to Blender 3.0, and only concerns changes that
MCprep may have to deal with. As such, I have linked the wiki pages for
each version of Blender for developers that want a full list.

This list should be updated as often as possible.
@StandingPadAnimations
Copy link
Collaborator

Looks like I still need to review this lol, sorry about the wait

@StandingPadAnimations
Copy link
Collaborator

Please also do a git merge from dev, so you have the latest files already in your branch, so when it comes to merging to dev, we have confidence it is going to work well.

Personally if it's to keep a branch up to date, I think a rebase would make more sense (merges should be reserved for feature branches being merged upstream, not just to update a branch), but that brings up the whole merge vs rebase debate

@zNightlord
Copy link
Contributor Author

I was a bit confused by entities.py has another FaceStruct, so it is removed.

@TheDuckCow
Copy link
Member

Personally if it's to keep a branch up to date, I think a rebase would make more sense (merges should be reserved for feature branches being merged upstream, not just to update a branch), but that brings up the whole merge vs rebase debate

It depends on how you do it, I've made the mistake of using a rebase where it should have been a git merge and results in merge conflicts which basically duplciates commits in the history. It's worth calling out that when pressing "resolve conflicts" in the actual github interface, it is the equivalent to running: git checkout annotate-documment; git merge dev which leaves both branches in place but just brings any new changes in the history of dev into the chain of annotate-documment

@zNightlord and I should actually say, the could should be to merger theduckcow:milestone-3-5-0 into your local branch, so that I have the fixes to the build script so I don't borq my addons folder again.

Please also do a check where you run and install MCprep with Blender 2.80 - I think some annotations might not be valid back that far.

@TheDuckCow
Copy link
Member

I'd like to get this merged in soon, and avoid having it drag on too much more! Thanks for putting up for the back and forth feedback here, moving forward I'll suggest we try and break changes into multiple separate parts to avoid these sorts of mega commits of 1K+ lines (refactoring will always be large, but there are ways to break it up).

@TheDuckCow TheDuckCow linked an issue Jul 5, 2023 that may be closed by this pull request
@TheDuckCow
Copy link
Member

Just checking @zNightlord do you have time to make updates in the next week and a half (before the dev meeting kickoff)? If not, I can try putting some time aside to fix issues I've run into with older blender versions. You might still need to add me as a direct collaborator on your fork though.

@zNightlord
Copy link
Contributor Author

Alright, Literal did not exist in py3.7 so using typing extension instead and for NodesModifier that is for 3.0

There are some lines like default=addon_prefs.skin_path in mcprep_ui causing some register problem. I'm not really sure why but comment it out, annotate is good with 2.80.

@TheDuckCow
Copy link
Member

Thanks for making the updates, I'm going through now also trying to see what is causing that line to fail. Issues during property registration are usually somewhat opaque and hard to track down as it's a sign of an issue somewhere else, but I'll let you know what I come across.

Removing unused imports, and fixing spacing inconsistencies in imports.
Additionally, performed sorting on subsections for each import (e.g.
all built in modules sorted among themselves).
@TheDuckCow
Copy link
Member

TheDuckCow commented Jul 17, 2023

FYI I pushed one commit @zNightlord, note how I removed spaces in some of your imports which was making pep8 have a riot. It makes it work quite a bit better for me now.

But now when I load on blender 2.80, I'm getting this error - are you testing on 2.80 and having it work? I get this as soon as I try to enable the addon.

Traceback (most recent call last):
  File "/Applications/Blender 2.80/blender.app/Contents/Resources/2.80/scripts/modules/addon_utils.py", line 351, in enable
    mod = __import__(module_name)
  File "/Users/patrickcrawford/Library/Application Support/Blender/2.80/scripts/addons/MCprep_addon/__init__.py", line 59, in <module>
    from . import load_modules
  File "/Users/patrickcrawford/Library/Application Support/Blender/2.80/scripts/addons/MCprep_addon/load_modules.py", line 28, in <module>
    from . import conf
  File "/Users/patrickcrawford/Library/Application Support/Blender/2.80/scripts/addons/MCprep_addon/conf.py", line 28, in <module>
    from typing_extensions import Literal
ModuleNotFoundError: No module named 'typing_extensions'

For what it's worth, I have Blender 2.80 (sub 75) local.

I'm also having tests fail, but I'm not sure why several fail when it seems like the actual code is working on. I'm investigating there.

@TheDuckCow
Copy link
Member

TheDuckCow commented Jul 17, 2023

As a compatibility reminder, blender 2.80's python is 3.7.0 so keep in mind the feature set we have available with that.

Unfortunately, based on this release date, I think that means we can't use the Literal keyword anywhere. I'd propose replacing all uses of Literal with just global static variables defined in conf where relevant.

@TheDuckCow
Copy link
Member

Doing some reading, seems like one option could be to have a pip3 install typing_extensions version packaged with the addon, and then do a relative import to get the typing extensions that way (I see that typing_extensions is referenced in some places, but we shouldn't presume or attempt to ad hoc install pip, I'd rather we just package the code with the addon itself in a modules or similar folder).

But all of that just seems horribly complicated. What if instead of using Literal anywhere, we just define python Enum values to use instead? (and utilize their own built in ability to fetch the string name, if ever we need to pass as a string)? What do you think @zNightlord?

Only files currently using Literal are: MCprep_addon/util.py, MCprep_addon/conf.py, and MCprep_addon/materials/sequences.py and all feel like they should easily be Enums (not exactly sure of the typing implication though)

@StandingPadAnimations
Copy link
Collaborator

I've only used Python enums once but they make me miss C++ enums (which in turn make me miss Rust enums). I don't know what it is about them (maybe the Class.ENUM syntax) but I dislike working with Python enums. That being said, they're a better option in this circumstance.

@zNightlord
Copy link
Contributor Author

I'm not sure why my download in the first place somehow has typing extension in it.
I'm good with using enum option.

@TheDuckCow
Copy link
Member

Ok great - let me take a stab at making this change then, as I will likely run into some other fixes for getting things working. thanks both for weighing in!

Blender 2.80 is at this moment, generally speaking working much better
now. There are still several unit tests that appear to be failing, but
we can improve incrementally up until the release. As an end user, most
functions are working ok (but not all.
Without this, tests were failing due to the addon being enabled and
re-enabled. Users who did this would have also faced the same issue.
Expecting to remove entirely in the first release after MCprep 3.5+ has
gained more than 50% adoption.
Copy link
Member

@TheDuckCow TheDuckCow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We made it folks. After 51 comments, 38 commits, and yes over a thousand lines changed, I'm ready to approve and merge this.

Since this is just going into the milestone branch, I'm using my owner authority to go ahead and merge. I should say, while all automated tests pass, I do have some errors or issues that cropped up that I'll be creating a fix list for, but the good news is Blender 2.80 is definitely working fine for the most common use cases. There are some issues affecting all versions (e.g. an issue with animate textures exists that I didn't get to). We can see about resolving these in new, dedicated branches for each issue so we can split up the work. I'll also be working on re-creating the test suit anyways, to use a more standard library. This will make it easier for others to run tests and validate their own work as well.

Proof of all tests passing for me locally, noting:

  • Loss of data is nothing to be overcome
  • img_sequence_effect_spawner Apparently this crashing is nothing new, perhaps the feature should just be disabled for blender 2.8x.

Raw test results:

blender	failed_test	short_err
(3, 6, 0)	ALL PASSED	-
(3, 5, 1)	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)	qa_effects	 loss of data!\n \n 
(2, 93, 0)	qa_rigs	 loss of data!\n \n 
(2, 80, 75)	img_sequence_effect_spawner	 consistent crashing
(2, 80, 75)	qa_effects	 loss of data!\n \n 
(2, 80, 75)	qa_rigs	 loss of data!\n \n 

@TheDuckCow TheDuckCow merged commit fcafc2c into Moo-Ack-Productions:milestone-3-5-0 Jul 17, 2023
@TheDuckCow TheDuckCow added this to the v3.5.0 milestone Jul 23, 2023
@zNightlord zNightlord deleted the annotate-documment branch July 30, 2023 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Dropping 2.7x support in MCprep 3.5
3 participants