Refinement/Fixes from using %nodepath and $nodepath changes for a few weeks

[greenfox1505]

Couple of issues arose when working with these changes. These are my fixes for that.

• These changes only applied to Solid Entities, not Point Entities. The functionality is now pulled out of generate_solid_entity_node( and called from both that and generate_point_entity_node(.

• apply_entity_properties( didn't handle the type change between String/StringName and NodePath. This handles that now. It's been a few weeks since I encountered this problem, I'm not totally sure what the issue was, but I am certain it fixed a type issue.

• generate_entity_node( now detects $node_name and treats it like %node_name, but without making it unique for naming nodes without the "entity_" + naming.

[greenfox1505]

#221 (comment)

Further expansion on the particular code changes that made this work.

[RhapsodyInGeek]

Looks like if it gets a type mismatch it displays the error twice. Don't need the third match statement.

But also, what happens if node property is a NodePath but properties[property] is an integer? Or a float? Should check if we're trying to send a string.

					if typeof(node.get(property)) == typeof(properties[property]):
						node.set(property, properties[property])
					elif typeof(properties[property] == TYPE_STRING):
						match typeof(node.get(property)):
							TYPE_STRING, TYPE_STRING_NAME:
								node.set(property, properties[property])
							TYPE_NODE_PATH:
								node.set(property, NodePath(properties[property]))
							_:
								push_error("Entity %s property \'%s\' type mismatch with matching generated node property." % [node.name, property])
					else:
						push_error("Entity %s property \'%s\' type mismatch with matching generated node property." % [node.name, property])

[greenfox1505]

I think I must have mangled that somewhere in my copy/pasting from my project version to the PR version. My project version of this is:

if property in node:
	if typeof(node.get(property)) == typeof(properties[property]):
		node.set(property, properties[property])
	else:
		match typeof(node.get(property)):
			TYPE_STRING,TYPE_STRING_NAME:
				node.set(property,properties[property])
			TYPE_NODE_PATH:
				node.set(property,NodePath(properties[property]))
			_:
				push_error("Entity %s property \'%s\' type mismatch with matching generated node property." % [node.name, property])

[greenfox1505]

Your version handles source/target data type mangling better than mine.

[greenfox1505]

A side effect of this system is that ownership of nodes matters a lot. This isn't really a FuncGodot issue, as long as you're building maps in Editor and not at Runtime, this usually wouldn't cause an issue. But I've run into some unique errors due to my mostly-runtime map building:

I have an Entity called MapEntity. It loads and builds a sub map. This lets me make props in Trenchbroom and then place those props in a separate TrenchBroom level without having to duplicate or use linked groups. It also lets that prop be used in multiple levels. (I have a caching system that turns maps into reusable prefabs so we don't generate multiple copies of the same mesh, etc)

For %node_name syntax to work, a node.owner must be set correctly. Under the hood, % is a StringName->Node* dict lookup against at owner (node.owned_unique_nodes). Ownership is set normally when building maps in editor. But since I'm often building at runtime, that doesn't always connect correctly. Or if you have more than one copy of the same MapEntity with internal references.

[RhapsodyInGeek]

Okay, but I still need that TYPE_STRING_PATH and TYPE_NODE_PATH check fixed before I can merge this.

[greenfox1505]

Okay, but I still need that TYPE_STRING_PATH and TYPE_NODE_PATH check fixed before I can merge this.

Yeah, I was mid-writing the above when you posted that. I'm looking at it now.

[greenfox1505]

I'd like to change that to this, to avoid the text duplication:

if property in node:
	const TYPE_MISMATCH_ERROR = "Entity %s property \'%s\' type mismatch with matching generated node property."
	if typeof(node.get(property)) == typeof(properties[property]):
		node.set(property, properties[property])
	elif typeof(properties[property] == TYPE_STRING):
		match typeof(node.get(property)):
			TYPE_STRING, TYPE_STRING_NAME:
				node.set(property, properties[property])
			TYPE_NODE_PATH:
				node.set(property, NodePath(properties[property]))
			_:
				push_error(TYPE_MISSMATCH_ERROR % [node.name, property])
	else:
		push_error(TYPE_MISSMATCH_ERROR % [node.name, property])

But duplicated error messages in the rest of the repo are not handled this way. I'll push your version if you'd prefer that.

[RhapsodyInGeek]

I'll push your version if you'd prefer that

I would prefer my version in this case. Sometimes it's better to not construct and allocate data if you don't need to.

I'm still not convinced that the $ prefix is the way to go for literal names, to avoid the entity_%s pattern. Admittedly also not convinced % is the best way to go for unique name nodes, but it's an actual Godot convention, and % is not often used for naming conventions.

Using $ as the means for changing the name to the literal property entry feels too hacky and obfuscated while also using a not uncommonly used character. I do think a toggle would be the better way to go, something much more explicit and obvious. I think I brought these concerns and ideas up during our last conversation about this topic.

Right now, I believe the only truly obfuscated parts of the plugin are what many of the Godot Variants translate to when written to the FGD and other class property construction tricks. I'd rather not add to the pile and I'd also prefer to avoid potentially breaking someone's project for an unnecessary convenience. Something to remember is that a change like this affects hundreds of users; any changes that could adversely affect their use needs to be with very good reason or worked around. The % change fits this, the $ does not.

[greenfox1505]

I don't think a const does construct or allocate?

---

I honestly wasn't expecting my previous MR to go through any more, so I just pulled what I was using locally; wasn't really considering broader implications when I wrote it for my own project.

I think % is solid. Godot literally will not let you use that character in a Node name and will replace it with _.

You CAN theoretically put a $ in a name. But, IMHO, that would be extremely bad practice and probably SHOULD be engine-level illegal. I would be extremely surprised if that broke anyone's project. Even with hundreds of users. But that's my opinion and I can understand an argument against it.

@ could also be an option, since that's also something Godot literally will not let you use in a node name, but IMHO, that goes against Godot's natural syntax, which makes going back and forth between tools more confusing.

---

I came up with a few ways we could add this feature, with low complexity that won't break existing projects.

Settings Bool

A plugin settings variable that defaults to "false":

elif ProjectSettings.get_setting("func_godot/dolarsign_as_literal_name_prefix", false) and name_prop.begins_with("$"):
	node_name = name_prop.trim_prefix("$")

Settings String

Plugin settings variable that is a string allowing you to write your own literal string prefix. Could be "$" or "@" or "no_entity_" or "whatever" (longer strings would be acceptable)

var literal_name_prefix = ProjectSettings.get_setting("func_godot/literal_name_prefix", "")
...
elif literal_name_prefix.length() != 0 and name_prop.begins_with(literal_name_prefix):
	node_name = name_prop.trim_prefix(literal_name_prefix)

Per Entity prefix

Add a per-entity setting to func_godot_fgd_enitty_class.gd (IMHO, allowing per-object setting here, while being more flexible, encourages much worse design. But I tend toward more opinionated design and I respect you tend toward maximum compatibility over enforcing standards)

#somewhere around `@export var name_property :=`
@export var use_literal_name := false
@export var literal_name_prefix := "$"

#in entity_assembler.gd
elif entity_def.use_literal_name and name_prop.begins_with(entity_def.literal_name_prefix):
	node_name = name_prop.trim_prefix(entity_def.literal_name_prefix)

---

I would be VERY surprised if anyone is actually $ in a node name expecting that to show up as entity_$.... It goes against Godot convention pretty hard. But any of these suggestions will work without breaking those theoretical configurations.

I can remove those two lines for THIS MR, then make another one to discuss options. Or you can tell me which of option here you like and I can roll that in right now. These are all pretty low complexity, adding very few overall lines (the project settings ones add the most due to overall Godot bureaucracy, there are a lot of repeated lines in func_godot_plugin.gd::_enter_tree() that could be pulled into a function).

[RhapsodyInGeek]

I would be extremely surprised if that broke anyone's project. Even with hundreds of users.

I have been doing this for a long time and yet users continue to surprise me, both in good and bad ways.

I can remove those two lines for THIS MR, then make another one to discuss options.

I think this would be the best option for now.