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

WIP - Other rig properties, mob type, color variant #572

Draft
wants to merge 7 commits into
base: dev
Choose a base branch
from

Conversation

zNightlord
Copy link
Contributor

@zNightlord zNightlord commented May 10, 2024

WIP of #440, second part of #561
(This is not ready for review, discuss further on how this should be implemented)

As in #440 this add a system for rig have MCPREP_RIGTYPE non "Custom" under the rig armature object to use a different skin swap tab.
It would be an improvement spawn_with_skin since it getting directly from the resource pack path.

ui_mob_skin.mp4

The UI properties is store under a class and go through store at WindowManager context.
Question:
How should it have access to the rig materials?

  1. Have a for loop iterate though the objects find matching material type property MCPREP_MatType (for example "Profession" "Level" "Biome" for villager)
  2. Have many custom properties MCPREP_MatVillagerProfession MCPREP_MatVillagerLevel MCPrep_MatVillagerBiome string name under the armature object and materials.get( name) to get

and use skin swap function to set the texture inside.

"Invert" mob type.
How or should have an "invert" or zombified type as a checkbox?
Allay -> Vex; Piglin -> Zombified Piglin; Hoglin -> Zombified Hoglin; Zombie Villager;

UI it would be something like Mine imator with the variant but rather upon adding the mob you do it after with an operator button to switch variant or color MCPREP_HAS_COLOR.

image

@zNightlord zNightlord changed the title WIP - WIP - Other rig properties, mob type, color variant May 10, 2024
Copy link
Collaborator

@StandingPadAnimations StandingPadAnimations left a comment

Choose a reason for hiding this comment

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

I know it's a draft at the moment, but I've done a basic review; looks mostly good. There are some areas where I think things could be done differently and more safe from typos and whatnot (such as using enums over raw strings). There's also some use of os.path, which isn't a blocker, but I'd rather pathlib be used since it has better abstractions with filesystems.

There are some i18n things to remember as well, although I do get that the i18n stuff is also extremely new as well. env._ should be used for anything user-facing, except self.report (since that's important for error messages).

I've noticed a class called VariationProp in spawn_util.py. Looks fine from first glance (minus the aforementioned i18n stuff), but it's quite large? I would suggest moving the things that don't depend on the class itself (enum variants) and make them independent functions that we can reuse later. I'm still on the fence on whether we should move all the variation related stuff to a new file, but I'll let @TheDuckCow comment on that.

mob_type = obj.get("MCPREP_mob_type", "Custom")

if mob_type == "Custom":
self.report({'ERROR'}, "You are not allowed to use on Custom rig")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Slight nitpick on spelling and grammar.

Suggested change
self.report({'ERROR'}, "You are not allowed to use on Custom rig")
self.report({'ERROR'}, "You are not allowed to use on a custom rig")

Comment on lines +765 to +784
name = mob_type.lower().replace(" ", "_")
if mob_type == "Villager":
self.doVillager(mats, texture_paths)
elif mob_type == "Zombie":
check_entity_texture(context)
if self.zombie_variation in ["ZOMBIE", "HUSK"]:
# Using Player model so convert player skin to 1.8 format
# loadSkinFile()
pass
else:
# Assign textures materials for drown
pass
elif mob_type == "Skeleton":
pass

elif mob_type in ("Allay", "Vex"):
if mob_type == "Allay":
name += "/" + name
else:
name = "illager"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this would be a good area to use an Enum. Perhaps something like MobType, where each value is rqual to a string. From there, it you could use MobType(whatever_string) to instantiate it.

pass
return {'FINISHED'}

def doVillager(self, materials: List[Material], texture_paths: List[str]):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick about Python convention, use snake_case for functions.

Suggested change
def doVillager(self, materials: List[Material], texture_paths: List[str]):
def do_villager(self, materials: List[Material], texture_paths: List[str]):

Copy link
Member

Choose a reason for hiding this comment

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

Agreed with above comment, but going further - is there some way that such a function like this could be generalized (and not specify villager in the name)?

Imagine that we somehow overhauled all our rigs, or provided different rigs for 3.0+ (for asset browser) vs 2.8/2.9. I would rather we avoid a situation where we have to change MCprep code just because rigs changed.

So, I think these specialized functions can still exist, I would just rather we try to make them generally useable and if anything rely on property assignments we check for to make them useable (I'm happy to change the rigs to match as needed, remember we want to save them in 2.80 specifically to be forwards compatible)

@@ -335,7 +344,29 @@ def download_user(self, context: Context, username: str) -> Optional[Path]:
self.track_param = "username"
return saveloc

def check_entity_texture(texture_path: str) -> Optional[Path]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd add a docstring here so we can better understand this function.

Comment on lines +351 to +361
active_pack = os.path.join(
active_pack, "assets", "minecraft", "textures", "entity")

base_pack = bpy.path.abspath(addon_prefs.custom_texturepack_path)
base_pack = os.path.join(
base_pack, "assets", "minecraft", "textures", "entity")

# if not os.path.isdir(active_pack):
# env.log(f"No models found for active path {active_pack}")
# return None
base_has_textures = os.path.isdir(base_pack)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not really the biggest fan of os.path for filepath operations, it's all string manipulation (vs pathlib.Path which is much nicer in abstraction). Not a blocker, but I'd advise using pathlib instead of os.path for new code since it's much better, and works with os.path functions anyway if needed.

@@ -1044,6 +1053,12 @@ def draw(self, context):
# datapass = scn_props.mob_list[mob_ind].mcmob_type
tx = f"Spawn {name} with {skinname}"
row.operator("mcprep.spawn_with_skin", text=tx)
b_row.label(text="Resource pack")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same story about i18n

Suggested change
b_row.label(text="Resource pack")
b_row.label(text=env._("Resource pack"))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the code was originally written months ago. I hold this for awhile and stuck there so. So noted.

Comment on lines +1989 to +1993
name="Modes",
description="Skinswap modes",
items=(
('PLAYER', "Player", ""),
('MOB', "Mob/Entity", "")
Copy link
Collaborator

Choose a reason for hiding this comment

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

These also need to use i18n functions

Suggested change
name="Modes",
description="Skinswap modes",
items=(
('PLAYER', "Player", ""),
('MOB', "Mob/Entity", "")
name=env._("Modes"),
description=env._("Skinswap modes"),
items=(
('PLAYER', env._("Player"), ""),
('MOB', env._("Mob/Entity"), "")

@@ -41,11 +45,298 @@
COLL_ICON = 'OUTLINER_COLLECTION' if util.bv30() else 'COLLECTION_NEW'


class VariationProp:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I won't go through this entire class, but don't forget to use env._ for any user facing strings.

Comment on lines +295 to +321
elif mob_type == "Zombie":
layout.prop(self, "zombie_variation")
elif mob_type == "Skeleton":
layout.prop(self, "skeleton_variation")
elif mob_type == "Axolotl":
layout.prop(self, "axolotl_variation")
elif mob_type == "Rabbit":
layout.prop(self, "rabbit_variation")
elif mob_type == "Frog":
layout.prop(self, "frog_variation")
elif mob_type == "Parrot":
layout.prop(self, "parrot_variation")
elif mob_type == "Llama":
layout.prop(self, "llama_variation")
elif mob_type == "Horse":
layout.prop(self, "horse_variation")
elif mob_type == "Cat" or mob_type == "Ocelot":
layout.prop(self, "cat_variation")
elif mob_type == "Dog" or mob_type == "Wolf":
layout.prop(self, "dog_variation")

counter_variant = ["Villager", "Piglin", "Hoglin", "Allay", "Vex"]
if mob_type in counter_variant:
text = "Is Vex" if mob_type == "Allay" else "Is Zombified"
layout.prop(self, "is_zombiefied", text=text)

has_variant = counter_variant + ["Zombie", "Skeleton", "Llama", "Axolotl", "Rabbit", "Llama", "Parrot", "Frog", "Horse", "Cat", "Ocelot"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Like I said in an earlier comment, a MobType enum would be a better option IMO for all of these if statements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like I said in an earlier comment, a MobType enum would be a better option IMO for all of these if statements.

I could reduce those if elif down with {x}_variantion self hasattr() check in the first place and leave the other complex variation there. Not sure it going confusing, readable as this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd use hasattr, it's much simpler in the end, and developers can read the Python docs if needed

Comment on lines +332 to +336
def getmob_type(rig: bpy.types.Object):
""" Get mob type from rig
args
obj: Armature Object
"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd also add some documentation on what this function returns (don't forget the return value type as well), and what values it returns in what conditions

Suggested change
def getmob_type(rig: bpy.types.Object):
""" Get mob type from rig
args
obj: Armature Object
"""
def getmob_type(rig: bpy.types.Object) -> bool:
""" Get mob type from rig
args
rig: Armature Object
Returns:
True if (insert condition), False otherwise
"""

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.

Just leaving a few comments. Biggest point is seeing if we can shift away from hard coding rig names/str in MCprep code, and try to do so via the json file or props on the rigs themselves.

I'm also getting a syntax error on this as-is (at least in blender 4.1) if we could take a look at that :)

 File "/Users/patrickcrawford/Library/Application Support/Blender/4.1/scripts/addons/MCprep_addon/materials/skin.py", line 796
    			if mat.get("MCPREP_VILLAGER_BIOME"):
    			                                    ^
IndentationError: unindent does not match any outer indentation level

Comment on lines +40 to +44
swap_all_imgs_desc = (
"Swap textures in all image nodes that exist on the selected \n"
"material; if off, will instead seek to only replace the images of \n"
"nodes (not image blocks) named MCPREP_SKIN_SWAP"
)
Copy link
Member

Choose a reason for hiding this comment

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

Though I agree with the call to use env._ here, I do think we should do it only once on the final constructed multi line string block. We have no way of knowing the number of lines or breaks for other locales.

Comment on lines 428 to -398


Copy link
Member

Choose a reason for hiding this comment

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

Style nit: Good to keep 2 spaces between top-level entities

self.doVillager(mats, texture_paths)
elif mob_type == "Zombie":
check_entity_texture(context)
if self.zombie_variation in ["ZOMBIE", "HUSK"]:
Copy link
Member

Choose a reason for hiding this comment

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

I think you've mentioned this before and I tend to agree: better to not hard code rig-specific data in our code files, but rather (if anywhere) into our mcprep json data file, or better yet onto properties or metadata of the rigs themselves.

Though if in this case, is it just about the player skin 1.8 format aspect ratio? If so, I actually have some code somewhere in MCprep that does this check in order to convert to 1.8 formats on the live, with the username download function.

pass
return {'FINISHED'}

def doVillager(self, materials: List[Material], texture_paths: List[str]):
Copy link
Member

Choose a reason for hiding this comment

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

Agreed with above comment, but going further - is there some way that such a function like this could be generalized (and not specify villager in the name)?

Imagine that we somehow overhauled all our rigs, or provided different rigs for 3.0+ (for asset browser) vs 2.8/2.9. I would rather we avoid a situation where we have to change MCprep code just because rigs changed.

So, I think these specialized functions can still exist, I would just rather we try to make them generally useable and if anything rely on property assignments we check for to make them useable (I'm happy to change the rigs to match as needed, remember we want to save them in 2.80 specifically to be forwards compatible)

@@ -1969,6 +1984,25 @@ class McprepProps(bpy.types.PropertyGroup):
effects_list_index: bpy.props.IntProperty(default=0)


class MCprepWindowManager(spawn_util.VariationProp, bpy.types.PropertyGroup):
Copy link
Member

Choose a reason for hiding this comment

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

I might also need some additional context, where and how is this manager window being used? Is this mean to be just open session property settings used to drive operator functions?

If so, might warrant some more discussion on how I'm thinking things will evolve, since we're moving towards things like resource pack layering

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, since this is more of an UI properties that need only one, single to operate and doesn't need to save to the context scene, blend file so WindowManager would be the best place for it.

@zNightlord
Copy link
Contributor Author

zNightlord commented May 13, 2024

Some code during the merging is mess up at some spot and not tested so I want to see how it should be structured from this draft.

How should it have access to the rig materials?

  1. Have a for loop iterate though the objects find matching material type property MCPREP_MatType (for example "Profession" "Level" "Biome" for villager)
  2. Have many custom properties MCPREP_MatVillagerProfession MCPREP_MatVillagerLevel MCPrep_MatVillagerBiome string name under the armature object and materials.get( name) to get
    and use skin swap function to set the texture inside.

"Invert" mob type.
How or should have an "invert" or zombified type as a checkbox?
Allay -> Vex; Piglin -> Zombified Piglin; Hoglin -> Zombified Hoglin; Zombie Villager;

There are 2 questions.

The first one it is because there is a function in MCPrep that goes over select armature to get the materials. That is why I'm not sure of using it or not while we could add a properties under armature that have material name to get it quickly.

The second one is just to reduce the usage of only 2 variant switch type mobs like Squid only have a default and Glow variant, allay and vex, slime and magma is also in that.

@StandingPadAnimations
Copy link
Collaborator

StandingPadAnimations commented May 31, 2024

Some code during the merging is mess up at some spot and not tested so I want to see how it should be structured from this draft.

How should it have access to the rig materials?

  1. Have a for loop iterate though the objects find matching material type property MCPREP_MatType (for example "Profession" "Level" "Biome" for villager)
  2. Have many custom properties MCPREP_MatVillagerProfession MCPREP_MatVillagerLevel MCPrep_MatVillagerBiome string name under the armature object and materials.get( name) to get
    and use skin swap function to set the texture inside.

"Invert" mob type.
How or should have an "invert" or zombified type as a checkbox?
Allay -> Vex; Piglin -> Zombified Piglin; Hoglin -> Zombified Hoglin; Zombie Villager;

There are 2 questions.

The first one it is because there is a function in MCPrep that goes over select armature to get the materials. That is why I'm not sure of using it or not while we could add a properties under armature that have material name to get it quickly.

The second one is just to reduce the usage of only 2 variant switch type mobs like Squid only have a default and Glow variant, allay and vex, slime and magma is also in that.

  1. I think it's better to go over the materials on the selected armature. We should try to avoid adding properties to objects unless we're constantly using them (ex. needing to access something for the UI, which updates very often)
    • Assuming we're dealing with armatures, we could simply iterate through all child objects of the armature and get their materials. For the purposes of generalization, this function could be called something like get_child_materials
  2. For mobs with just 2 variants, a single enum/thing to represent the 2 states might be an option, and might also make things more extensible for custom mobs. However, I'm not sure if this would make things more or less readable (I guess we'd have to try to find out)

Comment on lines +220 to +224
def squid_items(self, context: Context) -> List[Tuple[str, str, str]]:
items = [
('NORMAL', "Squid", ""),
('GLOW', "Glow", "")
]
Copy link
Collaborator

@StandingPadAnimations StandingPadAnimations May 31, 2024

Choose a reason for hiding this comment

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

For example, with this, perhaps we could change NORMAL and GLOW to BASE_MOB/NORMAL and ALTERNATE_MOB/ALTERNATE, then use some properties like MCPREP_MOB_BASE_MOB/MCPREP_MOD_NORMAL and MCPREP_MOB_ALTERNATE_MOB/MCPREP_MOB_ALTERNATE to define what these states should be called in the UI.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We could also expand this to all variations, and add a number after ALTERNATE_MOB/ALTERNATE (starting with 0 or 1), to make this idea more generalized.

Tagging @TheDuckCow for some thoughts on this approach

@StandingPadAnimations StandingPadAnimations linked an issue Jul 8, 2024 that may be closed by this pull request
1 task
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.

Using custom properties for assets
3 participants