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

zNightlord · 2024-05-10T07:38:00Z

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?

Have a for loop iterate though the objects find matching material type property MCPREP_MatType (for example "Profession" "Level" "Biome" for villager)
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.

StandingPadAnimations

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.

StandingPadAnimations · 2024-05-11T03:06:37Z

MCprep_addon/materials/skin.py

+		mob_type = obj.get("MCPREP_mob_type", "Custom")
+
+		if mob_type == "Custom":
+			self.report({'ERROR'}, "You are not allowed to use on Custom rig")


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")

StandingPadAnimations · 2024-05-11T03:08:14Z

MCprep_addon/materials/skin.py

+		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"


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.

StandingPadAnimations · 2024-05-11T03:08:56Z

MCprep_addon/materials/skin.py

+			pass
+		return {'FINISHED'}
+
+	def doVillager(self, materials: List[Material], texture_paths: List[str]):


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]):

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)

StandingPadAnimations · 2024-05-11T03:10:17Z

MCprep_addon/materials/skin.py

@@ -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]:


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

StandingPadAnimations · 2024-05-11T03:12:01Z

MCprep_addon/materials/skin.py

+	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)


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.

StandingPadAnimations · 2024-05-11T03:22:29Z

MCprep_addon/mcprep_ui.py

@@ -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")


Same story about i18n

Suggested change

b_row.label(text="Resource pack")

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

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

StandingPadAnimations · 2024-05-11T03:24:12Z

MCprep_addon/mcprep_ui.py

+		name="Modes",
+		description="Skinswap modes",
+		items=(
+			('PLAYER', "Player", ""),
+			('MOB', "Mob/Entity", "")


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"), "")

StandingPadAnimations · 2024-05-11T03:25:30Z

MCprep_addon/spawner/spawn_util.py

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


+class VariationProp:


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

StandingPadAnimations · 2024-05-11T03:27:44Z

MCprep_addon/spawner/spawn_util.py

+		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"]


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

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.

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

StandingPadAnimations · 2024-05-11T03:30:07Z

MCprep_addon/spawner/spawn_util.py

+def getmob_type(rig: bpy.types.Object):
+	""" Get mob type from rig
+	args
+		obj: Armature Object
+	"""


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

"""

TheDuckCow

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

TheDuckCow · 2024-05-13T04:04:50Z

MCprep_addon/materials/skin.py

+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"
+)


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.

TheDuckCow · 2024-05-13T04:05:43Z

MCprep_addon/materials/skin.py

-

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

TheDuckCow · 2024-05-13T04:24:18Z

MCprep_addon/materials/skin.py

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


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.

TheDuckCow · 2024-05-13T04:27:37Z

MCprep_addon/materials/skin.py

+			pass
+		return {'FINISHED'}
+
+	def doVillager(self, materials: List[Material], texture_paths: List[str]):


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)

TheDuckCow · 2024-05-13T04:32:58Z

MCprep_addon/mcprep_ui.py

@@ -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):


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

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 · 2024-05-13T14:13:49Z

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?

Have a for loop iterate though the objects find matching material type property MCPREP_MatType (for example "Profession" "Level" "Biome" for villager)

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 · 2024-05-31T06:09:10Z

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?

Have a for loop iterate though the objects find matching material type property MCPREP_MatType (for example "Profession" "Level" "Biome" for villager)

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.

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
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)

StandingPadAnimations · 2024-05-31T06:15:45Z

MCprep_addon/spawner/spawn_util.py

+	def squid_items(self, context: Context) -> List[Tuple[str, str, str]]:
+		items = [
+			('NORMAL', "Squid", ""),
+			('GLOW', "Glow", "")
+		]


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.

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

zNightlord added 7 commits July 30, 2023 14:27

Layout things

7d35b1f

Add back the enum

9ff1e4e

unf

ea6956c

some extra code

8f500da

swap mob variant

e3f3cfe

change

75d9801

init wip-prop-other-attributes

a66bae9

zNightlord changed the title ~~WIP -~~ WIP - Other rig properties, mob type, color variant May 10, 2024

StandingPadAnimations requested changes May 11, 2024

View reviewed changes

TheDuckCow reviewed May 13, 2024

View reviewed changes

StandingPadAnimations reviewed May 31, 2024

View reviewed changes

StandingPadAnimations linked an issue Jul 8, 2024 that may be closed by this pull request

Using custom properties for assets #440

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

zNightlord commented May 10, 2024 •

edited

Loading

StandingPadAnimations left a comment

StandingPadAnimations May 11, 2024

StandingPadAnimations May 11, 2024

StandingPadAnimations May 11, 2024

TheDuckCow May 13, 2024

StandingPadAnimations May 11, 2024

StandingPadAnimations May 11, 2024

StandingPadAnimations May 11, 2024

zNightlord May 13, 2024

StandingPadAnimations May 11, 2024

StandingPadAnimations May 11, 2024

StandingPadAnimations May 11, 2024

zNightlord May 13, 2024

StandingPadAnimations May 14, 2024

StandingPadAnimations May 11, 2024

TheDuckCow left a comment

TheDuckCow May 13, 2024

TheDuckCow May 13, 2024

TheDuckCow May 13, 2024

TheDuckCow May 13, 2024

TheDuckCow May 13, 2024

zNightlord May 13, 2024

zNightlord commented May 13, 2024 •

edited

Loading

StandingPadAnimations commented May 31, 2024 •

edited

Loading

StandingPadAnimations May 31, 2024 •

edited

Loading

StandingPadAnimations May 31, 2024

	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")

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

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

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


		class VariationProp:

		@@ -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):

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

Are you sure you want to change the base?

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

Conversation

zNightlord commented May 10, 2024 • edited Loading

StandingPadAnimations left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheDuckCow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zNightlord commented May 13, 2024 • edited Loading

StandingPadAnimations commented May 31, 2024 • edited Loading

StandingPadAnimations May 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zNightlord commented May 10, 2024 •

edited

Loading

zNightlord commented May 13, 2024 •

edited

Loading

StandingPadAnimations commented May 31, 2024 •

edited

Loading

StandingPadAnimations May 31, 2024 •

edited

Loading