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

Change Skin Swap's behavior for more nuanced swapping #498

Merged
merged 6 commits into from
Jan 4, 2024

Conversation

StandingPadAnimations
Copy link
Collaborator

Skin swap originally would change all image textures on all materials of the selected object, regardless of whether the user wanted to or not. This is not ideal for rigs that need multiple separate textures. For example, the ardoni rigs in Black Plasma Studio's (now Squared Media) Songs of War series used multiple textures to handle stuff like emission. In addition, speaking from personal experience, I have several textures I use on my rigs these days, and skin swap's current behavior makes it unusable in my case.

To combat this, skin swap will now use a more nuanced method of swapping skins based on node name, which is an already established precedent from the MCprep Cycles Optimizer (README). MCprep will now search for a node called MCPREP_SKIN_SWAP, and treat it as a diffuse node. The old behavior will still remain as a legacy option, but will be deprecated in 3.5.2/3.6 (depending on the circumstances) and will be removed completely in 3.6/3.7 (again, depending on circumstances)

Skin swap originally would change all image textures on all materials of
the selected object, regardless of whether the user wanted to or not.
This is not ideal for rigs that need multiple seperate textures.

To expand the use case of skin swap, skin swap will now use a more
nuanced method of swapping skins based on node name, which is an already
established precedent from the MCprep Cycles Optimizer
@StandingPadAnimations StandingPadAnimations requested a review from a team November 1, 2023 14:01
@StandingPadAnimations
Copy link
Collaborator Author

This does quality as a breaking change, hence why we have the option for legacy behavior

@StandingPadAnimations StandingPadAnimations changed the title Changing Skin Swap's behavior for more nuanced swapping Change Skin Swap's behavior for more nuanced swapping Nov 1, 2023
@StandingPadAnimations StandingPadAnimations added this to the v3.5.1 milestone Nov 1, 2023
@StandingPadAnimations StandingPadAnimations self-assigned this Nov 1, 2023
Copy link
Contributor

@zNightlord zNightlord left a comment

Choose a reason for hiding this comment

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

Looks good now.

This part doesn't impact much but i think it should be dicussed.

Using label might be better than name. Name has that need to be unique
For example: having 2 nodes for swap would be MCPREP_SKIN_SWAP_A MCPREP_SKIN_SWAP_B while label would be MCPREP_SKIN_SWAP for both

Optionally having an extra `MCPREP_SKIN_SWAP' property in the material properties and used that string for checking is also good for someone have their own naming convention, don't have to follow a hardcode one if it exists. Without it it is fine.

@StandingPadAnimations
Copy link
Collaborator Author

Using label might be better than name. Name has that need to be unique For example: having 2 nodes for swap would be MCPREP_SKIN_SWAP_A MCPREP_SKIN_SWAP_B while label would be MCPREP_SKIN_SWAP

It should be fine since we're checking if the string contains at least MCPREP_SKIN_SWAP (and should ignore the .### generated by Blender). MCprep does have a function to remove the .### for duplicates, so we could probably just use that to make sure. However, the node name being "unique" only applies to the material (2 nodes that are named MCPREP_SKINSWAP` but on seperate materials won't be counted as duplicates)

@StandingPadAnimations StandingPadAnimations requested a review from a team November 3, 2023 13:22
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.

I think this is an ok change, but some requests from me:

  • I think we should rename the "legacy" option to "Swap all textures".
  • Make this "Swap all textures" be default-on, and we should not intend to remove it in the future
  • And finally, add a warning message if "Swap all textures" (ie this new mode you've created) is False AND no texture was modified. This message should say: No image node "MCPREP_SKIN_SWAP" found, nothing swapped.

Rationale:

For any users who have not prepped materials on their rig (though this is the default in MCprep on spawn), skin swap will stop working. Favoring the advanced use case will confuse new users. If the skin swap operation requires you to go into a material to edit a node to make it work after bringing in a rig you downloaded somewhere (and likely doesn't have this label), it's defeating much of the purpose of the operator in the first place since it's for people who don't want to bother (or know how) to modify shader nodes.

So, I'd rather this be an advanced feature that can be toggled on. You'd only want this behavior if you already knew you had previously set up the rig material with this label, and therefore you would know to expect un-tick the box in the redo last menu. For everyone else, they'd be unaware it was an option but they'd still get skin swapping as expected.

@TheDuckCow TheDuckCow modified the milestones: v3.5.1, v3.5.2 Nov 15, 2023
@TheDuckCow TheDuckCow self-requested a review December 26, 2023 18:59
@TheDuckCow TheDuckCow modified the milestones: v3.5.3, v3.5.2 Dec 26, 2023
@TheDuckCow
Copy link
Member

I'm suggesting we move this to v3.5.2, I'd be happy to make the changes I recommended above to ensure that it is backwards compatible. The more nuanced swapping should be a user preference anyways to opt in, already better to have it rolled out as an option.

We have loose plans for a wider UI overhaul which would include a more overall scene settings configuration, this could eventually live there in addition to user default preferences.

@TheDuckCow
Copy link
Member

FYI I'm powering through the changes here, to wrap up the release.

Adding positional args to fix ordered references and set, while making
custom nuanced swapping still possible.
@TheDuckCow
Copy link
Member

Alright, I'm pretty happy with the current state of this PR - @StandingPadAnimations not sure if you want to do a quick sanity check, ironically reviewing your own PR with my coauthored commit, but I realized there were some mistakes with some use of the positional arguments (and a weird indentation with one function).

All tests pass locally for me:

-------------------------------------------------------------------------------
bversion   	ran_tests	ran	skips	failed	errors
-------------------------------------------------------------------------------
(3.6.2)   	all_tests	64	2	0	No errors
(4.0.2)   	all_tests	64	2	0	No errors
(3.5.1)   	all_tests	64	2	0	No errors
(3.4.0)   	all_tests	64	2	0	No errors
(3.3.1)   	all_tests	64	2	0	No errors
(3.2.1)   	all_tests	64	2	0	No errors
(3.1.0)   	all_tests	64	2	0	No errors
(3.0.0)   	all_tests	64	2	0	No errors
(2.93.0)   	all_tests	63	3	0	No errors
(2.90.1)   	all_tests	63	3	0	No errors
(2.80.75)   	all_tests	62	4	0	No errors
tests took 196s to run, ending with code 0

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.

Meant to press approve for the record yesterday

@TheDuckCow
Copy link
Member

Presuming this is working fine-enough, I'm going to go ahead and merge so we can cut a release soon.

@TheDuckCow TheDuckCow merged commit 37a4510 into dev Jan 4, 2024
@TheDuckCow TheDuckCow deleted the skinswap-changes branch January 4, 2024 01:52
StandingPadAnimations added a commit that referenced this pull request Apr 28, 2024
* Update Asset-Submission.yaml to add asset-submission (#449)

Moving forward, using this label to apply to asset submissions to make it easier to identify, and to potentially identify community members to review.

* Change bpy build yaml config

* Migrating more critical tests ahead of release.

This includes QA checking scripts, which have already uncovered one
issue with one of the updated rigs.

* Migrated tests deleted from prior commit.

* Removed last bv28 calls and no longer necessary qa test limit

* Fixed geo node effect spawning for blender 4.0+ and migrated tests.

* Resolved most lint errors in the mcmodel file.

* Removed already irrelevant old unit tests.

* Updated mapping data

* Enable unittest -t flag to match entire test classes or files.

* Fix running util_test class on its own.

* Fix bug where the "discover" unittest function would delete data json.

This was a weird one. Essentially, running the unittests would always
result in the `mcprep_data_update.json` file being renamed to
`mcprep_data.json` (and since that file isn't tracked in git, would look
like the file was deleted wrt to local changes). This was happening, out
of all things, due to this line in the unittest runner:

unittest.TestLoader().discover(MODULE_DIR, "*_test.py")

This is because the discover command clearly has to attempt loading of
the modules it's scanning over, and at some point we import the env
module and thus the __init__ code on module load would trigger,
resulting in the file rename. By moving the rename into the register
function, we ensure it only happens in the context of an addon, which
is all we really need for it.

* Migrated all remaining tests, with all tests passing.

Unfortunately, we do still have the issue that the test runner renames
the mcprep_data_update.json file, and modifies checked-in mtl files,
but we can address this in the future.

* Removed the old test script and shell files.

The new test runner in python is straight forward enough to use that I
feel there is no longer a need for any system-specific shell files.

* Upversion MCprep to v3.5

* Ensure exit code of test runner is non-zero if any failure

* Create the initial release-drafting shell script

* Incrementing version for dev branch

* Added disclaimer in README

* Adopting py 3.7+ syntax and removing most lint warnings and errors

Some can't be avoided, such as those around bpy property definitions.

* Proactively identify bad analytics ID and replace with new one

Addresses an issue introduced in MCprep v3.5.0

* Raise more specific error with runtime error wrapper in background.

* Establish baseline tracker.py unittest

* Ensure bad parent id is overwritten by good local id + exhaustive tests

* Renaming SKIP_IDS to be INVALID_IDS.

* Updated BlenderChanges.md for Blender 4.0

* fix overlay geometry

* Updated for safer and more common bash script styling

* Add comment, slight face offset for overlay geometry

* Offset the overlay texture only

* Update load material function to support principled BSDF v2 in blender 4.0

* Add bpy-build to gitignore

* Revert "Add bpy-build to gitignore"

This reverts commit 03ac1d2.

* Revert "Change bpy build yaml config"

This reverts commit 8f2f793.

* Reducing overlay offset and merge by distance margin

* Upversion for next release

* Improved releaser script

Ran into a couple issues during the last release

* Upversion post 3.5.1 release

* Ensure tracker files never remain after tests or during prod builds (#506)

Two levels of trying to make sure we never ship with tracker code again.
Also removed now-redundant test for checking if tracker id present in git dir

* Marked optimizer as deprecated

* Added note on deprecation in optimize_scene.py

* Remove deprecated conf.py references

This commit removes the deprecated conf.py references that were meant to
be removed in MCprep 3.5.1, but weren't due to Blender 4.0's release

* Removed make_annotations and renamed compat funcs

* Fixed "Find Missing" due to animated image location and added test. (#520)

This operator was already high in terms of test coverage, but the one
branch not covered (and, the todo listed) was for animated textures. of
course, that was what went wrong. Now it tests this branch too.

* Fix direct socket name reference error in optimizer. (#519)

* Prevent collection-effect obj particle src from being cleaned up. (#518)

Before this change, if a collection effect included a particle system
where the source of the particles was an object (as opposed to an entire
collection), then on save and reload of that file, the particle object
would have been cleaned up, as it was not linked to the scene and for
some reason, since forever, blender does not count usage of an object
as a particle emitter as an object use reference when it comes to its
own internal cleanup.

Works now, tested by saving and reloading a file after spawning in a
version of the firework tutorial where I didn't use a collection as the
firework source.

* Made notice regarding owning a legal copy of Minecraft more visible (#522)

More explicit notice about having valid MC license

---------

Co-authored-by: TheDuckCow & StandingPad Animations

* Change Skin Swap's behavior for more nuanced swapping (#498)

* Changed behavior of skin swap to be more nuanced

Skin swap originally would change all image textures on all materials of
the selected object, regardless of whether the user wanted to or not.
This is not ideal for rigs that need multiple seperate textures.

To expand the use case of skin swap, skin swap will now use a more
nuanced method of swapping skins based on node name, which is an already
established precedent from the MCprep Cycles Optimizer

* Fixed detection of node name in set_cycles_texture

* Fixed error with node name checks

* Changed name check to ignore duplicates

* Rename and set swap_all_imgs default for texture changing function

Adding positional args to fix ordered references and set, while making
custom nuanced swapping still possible.

---------

Co-authored-by: Patrick W. Crawford <theduckcow@live.com>

* Upversion for MCprep v3.5.2

* Update Bug-Report.yml (#525)

* Updated mcprep_data json mapping for latest MC 1.20.4

* Readme updates for new settings and deprecation note

* Updating grass canonical mapping and improved desaturation debug prints

With this, we now have correct prep material saturation again for this
grass object, and all tests passing again.

* Added clarification to README

* Fixed incorrect keyword argument

* Added swap texture pack test

* Upversion and disable porcelain check

* Updated test run script and refreshed data file for MC 1.20.5

* chore: Merging Milestone 3.6.0 to Dev

* Upgrade to BpyBuild 0.3.0

* Updated reference command in CONTRIBUTING.md

* Added action-scripts folder with BpyBuild actions

This is so we can better organize our scripts

* Added Ignore Filters action as default

This was something that would have been useful in #505, and now we have
it :D

* Updated ignore_filters.py to print deleted files

This is a small change that'll make it easier to understand what's going
on at runtime with regards to the ignore filter

* Updated build command in run_tests.py

* Added print statement to dev.py

* Updated bpy-build.yaml

* Added .blend filter for dev builds

* Switched to BpyBuild ignore filters

* Revert "Switched to BpyBuild ignore filters"

This reverts commit 7469142.

* Updated config option in BpyBuild

* Added warning to ignore_filters.py

* Added error object for better user experience

* Switched to BaseException for MCprepError

* Removed redundant variable

* Refactored colorspace setup to be more flexible

In the past, we'd set this to a hard coded value. However, that proved
to be annoying to users using non-standard OCIO configs like ACES or
early versions of AgX. MCprep already fixes MTL files for ACES
compatibility, so we're expanding this to prep materials.

In `mcprep_data.json`, there will now be a section called
"non_color_options", which is a list of different options for Non-Color
Data/Generic Data. If a user is using a non-standard setup, they can
simply add the correct option in the JSON file and prep materials will
function properly.

The matching goes in order from first to last, and MCprep will use the
first value matched at runtime.

* First commit for i18n

* Added .venv to gitignore

* Fixed missing ) and added additional string

* Moved import statement for inspect

* Reduced nesting in function

* refactor(comment): Refactored comment in util.py

* Added some metadata to English PO file

* build(config): Removed ignore filter

* Update .gitignore

Co-authored-by: Patrick W. Crawford <theduckcow@live.com>

* chore: removed MCprep_resources from gitignore

* feat: Added zn_CN to Languages

* refactor(language): moved en to en_US

* feat: make MCprep follow the user's Blender locale

* refactor(languages): moved to zn_HANS

* fix: made translations change on the fly

* fix: Added fallback for non-existent translations

* fix: Fixed fallback

* Added some details to MCprep POT

* docs: Added documentation on translating MCprep

* docs(i18n): Removed redundant line

* Updated POT file

* chore(i18n): Added new strings to zn_HANS

* Recompiled zh_HANS

* fix(annotations): Added missing annotation to _

* docs(i18n): Added developer documentation

* Added polib to dependencies

* build: Added dictionary generator for i18n

This allows us to use bpy.app.translations without requiring translators
to have Python experience. While Blender allegedly has this ability, I
haven't been able to find it, so as far as I'm aware, it doesn't exist.

* Added use of bpy.app.translations

* fix(i18n): Fixed detection of translations.py

* Added autobuilding of MO files to default action

* docs(i18n): Updated i18n docs for maintainers

* build(i18n): Added comments to mo compile script

* docs(i18n): Added BpyBuild to i18n docs

* chore(git): Added MO files to .gitignore

* refactor(removal): Remove MCprep optimizer

The MCprep optimizer was deprecated in the MCprep 3.5 series, and slated
for removal in MCprep 3.6. This is due to the following reasons:
- Outdated design (made prior to Blender 3.0 when Cycles X was still in
  development, and has yet to adapt to modern Cycles)
- Extreme bugs that are hard to fix, such as 80+ light bounces
- Unnecessary as there's no special optimizations for Minecraft scenes,
  and on the contrary may actually be a bad thing as it uses settings
  like clamping indirect with horrible options

* Added polib directly in the source tree

* build: Added separate translate action

* build: added if-name-main to all actions

* docs(i18n): Updated developer docs

* Readded flake8 to requirements.txt

* docs(i18n): Removed negative tone from docs

* Revert "Added polib directly in the source tree"

This reverts commit 8288d5d.

* Updated POT file

* Updated contributing guide readme

* docs: Updated BlenderChanges.md

* build: Add script to generate POT file + Migration to BpyBuild 0.4 (#548)

* build: Add script to generate POT file

* build: Moved build_pot to a separate action

* Fixed metadata and line reference generation

* Updated POT file

* build: Future proofed script for BpyBuild actions

* Revert "build: Future proofed script for BpyBuild actions"

This reverts commit 2d1533a.

* deps: Updated BpyBuild to 0.4

* build: Updated build_pot to use pre_build

* i18n: Updated POT file

* build: Moved build-pot to translate action

* build: Fixed error causing wrong POT comments

* Added translate to release script

* Further update the release script to track changes and safer ver check

* build: Add metadata to POT file, added 4.1 and 4.2

---------

Co-authored-by: Patrick W. Crawford <theduckcow@live.com>

---------

Co-authored-by: Patrick W. Crawford <theduckcow@live.com>

---------

Co-authored-by: Patrick W. Crawford <theduckcow@live.com>
Co-authored-by: Cathal Feeney <c.feeney333@gmail.com>
Co-authored-by: 0znightlord0@gmail.com <0znightlord0@gmail.com>
Co-authored-by: Trung Phạm <61040487+zNightlord@users.noreply.github.com>
Co-authored-by: Patrick W. Crawford <moo-ack@theduckcow.com>
StandingPadAnimations added a commit that referenced this pull request Jul 7, 2024
* Changed behavior of skin swap to be more nuanced

Skin swap originally would change all image textures on all materials of
the selected object, regardless of whether the user wanted to or not.
This is not ideal for rigs that need multiple seperate textures.

To expand the use case of skin swap, skin swap will now use a more
nuanced method of swapping skins based on node name, which is an already
established precedent from the MCprep Cycles Optimizer

* Fixed detection of node name in set_cycles_texture

* Fixed error with node name checks

* Changed name check to ignore duplicates

* Rename and set swap_all_imgs default for texture changing function

Adding positional args to fix ordered references and set, while making
custom nuanced swapping still possible.

---------

Co-authored-by: Patrick W. Crawford <theduckcow@live.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants