Guards: Modify how variables are being exported #398
Conversation
JuanFdS
force-pushed
the
modify-how-variables-are-being-exported-in-guards
branch
3 times, most recently
from
27bf802 to
32788fe
Compare
| if not property_dict.is_empty(): | ||
| property_dict = property_dict.duplicate() | ||
| property_dict["name"] = "%s__%s" % [child.name.to_snake_case(), property] | ||
| property_dict["name"] = "%s__%s" % [child.name, property] |
There was a problem hiding this comment.
Does this break any properties currently saved in the game? I think yes:
scenes/game_elements/props/buildings/house/house_1.tscn:11:house_sprite__texture = ExtResource("2_qufpk")
scenes/game_elements/props/buildings/house/house_2.tscn:11:house_sprite__texture = ExtResource("2_1mgcp")
scenes/game_elements/props/decoration/bush/bush.tscn:8:bush_sprite__texture = ExtResource("1_p46yk")
scenes/game_elements/props/decoration/spool/spool.tscn:11:spool_sprite__texture = ExtResource("2_dh24t")
...
There was a problem hiding this comment.
You are right. Opening those scenes again corrected it. Then, for the scenes that included these decorations, I went over them and corrected the property manually in 5bd21a6
| func _get_property_list() -> Array[Dictionary]: | ||
| var properties: Array[Dictionary] = [] | ||
| properties.append(PropertyUtils.group("Appearance")) | ||
| properties.append_array( | ||
| PropertyUtils.expose_children_property(self, &"sprite_frames", &"AnimatedSprite2D") | ||
| ) | ||
| return properties | ||
|
|
||
|
|
||
| func _get(property_name: StringName) -> Variant: | ||
| return PropertyUtils.get_child_property(self, property_name) | ||
|
|
||
|
|
||
| func _set(property_name: StringName, value: Variant) -> bool: | ||
| return PropertyUtils.set_child_property(self, property_name, value) |
There was a problem hiding this comment.
Using PropertyUtils we lose adding a documentation comment to the exported property. For example in Player SpriteFrames, I have documented which animations and how many frames each animation should have.
Reading all this, I'm not so sure how much is PropertyUtils saving us, compared to losing the documentation comment. It also makes things more complex by having to traverse all the tree, in my opinion.
There was a problem hiding this comment.
Oh, I tried encoding that requirement as a warning, so it's harder to miss. You are right though about the documentation (!), I think there's no way to add documentation to properties added through _get_property_list, which is a shame
There was a problem hiding this comment.
Yeah, I think that this project being for educational purposes, we should put the effort on our (developer) side when exporting each of these properties, and keep each property documented. Is also not apparent to me how much does PropertyUtils really save to us, developers. I'd get rid of PropertyUtils and move Decoration and Teleporter back to normal exporting.
There was a problem hiding this comment.
Mmm, okay, I think this is a trade off between making it easier to mod almost any texture in the game vs code simplicity. Both are things that are desirable for the learners side.
With the PropertyUtils approach, we can write that once and any animated sprite owned by this scene will be exposed, which might be nice if we have scenes with more than one. Also, it'd be the same exact boilerplate we could have in other scenes (I actually just copied these 3 functions to the throwing_enemy.tscn and that was it), even when the AnimatedSprite2D nodes might be called different or be in different parts of the scene hierarchy. So, I think it has some value. We could be even go higher level and have something like:
func _get_property_list() -> Array[Dictionary]:
return PropertyUtils.expose_children_sprites(self)that automatically exposes the texture property from any Sprite and the sprite_frames property from any AnimatedSprite2D. It'd be even more magic (pejorative) but it would make it really simple to make a node's textures configurable (just have these 3 functions and @tool).
On the other hand, it's not obvious at all how this works and requires deeper knowledge of godot, so we might not want the learners to deal with that just so they have automatic exports for the spritesheets. The alternative would be to repeat the boilerplate for each node that we want to re-export properties and write the necessary glue code:
threadbare/scenes/game_elements/characters/enemies/guard/components/guard.gd
Lines 131 to 132 in b4fc8f3
I'm fine either way, what do you think should be the priority in this case? @manuq
| for required_animation in ["alerted", "idle", "walk"]: | ||
| if sprite_frames and not sprite_frames.has_animation(required_animation): | ||
| warnings.push_back( | ||
| ( | ||
| "AnimatedSprite2D__sprite_frames is missing the following animation: %s" | ||
| % required_animation | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Nice, I should add a warning to the Player SpriteFrames as well.
JuanFdS
force-pushed
the
modify-how-variables-are-being-exported-in-guards
branch
2 times, most recently
from
950279b to
b68af68
Compare
Converting to snake_case and then to PascalCase doesn't always produces the original result. For example: AnimatedSprite2D gets turned into animated_sprite_2d and then into AnimatedSprite2d (last character is d instead of D). Removed that logic to make it work for those cases, the inspector still shows it in a readable way.
JuanFdS
force-pushed
the
modify-how-variables-are-being-exported-in-guards
branch
from
b68af68 to
e38a265
Compare
There are 2 changes: - Use group instead of categories for properties. This is in order to keep it consistent with other scenes (throwing_enemy.tscn, projectile.tscn, sokoban's directional_input.tscn) where we also used groups. - Use PropertyUtils.expose_children_properties for the AnimatedSprite2D texture, so we just reuse that pattern everywhere we need to do that.
|
Closed since we are moving out of exporting variables automagically with PropertyUtils for the sake of simplicity |
While working in #260, I realised:
PropertyUtils.expose_children_propertiesin Allow configuring Guards' spritesheets in the inspector #363 (so answering Allow configuring Guards' spritesheets in the inspector #363 (review), I actually forgot about that when I made that PR 😅 ).PropertyUtils.expose_children_propertieshad a bug that made it not work for some properties names.This PR addresses those 3 things and adds a helper to PropertyUtils to create groups.