Add export variables for default build XML

[sinewavey]

I was tired of re-exporting the NRC config, and erasing my default build menu in the process.

Sure, I could just save a copy somewhere and replace it each time (no thanks) or use the desired file as an export variable -- but it's really just a few key/string value combos that can be written in a few arrays.

Here's a rough patch to include this, or at least allow the data to be retained export to export. Also, there was a minor typo in the print statements that was corrected.

As always, I don't expect this to be without some change or feedback.

[RhapsodyInGeek]

It's really hard to track the actual changes when you've (I assume accidentally?) deleted every indentation that doesn't have any code on the same line. Is there any chance you could undo that and avoid it in the future?

[sinewavey]

Oh shoot, that's my Godot editor auto clearing whitespace. Will fix this for the future + this commit as well

Resolved. Personally, I think trimming trailing whitespace is preferable but that's something that would be its own cleanup commit, otherwise, stick to the pattern. Not a pressing issue, just the source of this (it's an option in the editor itself)

[RhapsodyInGeek]

The main reason I don't clean up white space indentations in GDScript is that it makes it easier to track what lines of code belong to which code blocks when they're spaced apart with line breaks when in the Godot's script editor. Makes it easier to parse when you're dealing with extremely long scripts (like func_godot_map.gd). I'm honestly not sure of any good reason to clean up white space indentations to be honest, since there isn't a measurable impact on performance.

That doesn't mean trailing white space spaces shouldn't be cleaned up, but that's also not really a rampant problem as far as I've seen.

[RhapsodyInGeek]

Can we call these default_build_menu_variables and default_build_menu_commands?

Additionally, wouldn't default_build_menu_variables be better as a Dictionary? That way we can use the keys as the variable names and the values as the.... values. It's more intuitive than making a 2 element array for each variable. Basically require it as Dictionary<String><String>.

[sinewavey]

I was split between which one to use anyway. The difference came down to which I found easier to work with in the inspector, and that's just preference. Not a problem, will push a fix shortly

[RhapsodyInGeek]

Looking at how build options and commands are constructed I think it'd make sense to also make these Dictionaries, but they'd be treated as Dictionary<String><Array>, as there can be multiple commands in a build option. Would make them easier to parse, more intuitive.

[RhapsodyInGeek]

I totally forgot we generated the default_build_menu.xml, even though it makes sense that we do, why would NRC autogenerate that for us when it doesn't autogenerate anything except my misery?

[sinewavey]

I totally forgot we generated the default_build_menu.xml, even though it makes sense that we do, why would NRC autogenerate that for us when it doesn't autogenerate anything except my misery?

Lol, yep. Time to fix that other docs page again lol

[RhapsodyInGeek]

Should be default_build_menu_variables, since we don't actually use the term build_xml anywhere and it's too much of a shorthand. Exposed variables should always be fully descriptive and easily associated with other processes and exports.

Can we also get a more expository description on what these variables are for? Not quite a tutorial, but since NRC is so poorly documented it might be helpful to let NRC users know what this is for. We should also make sure people know that the keys and values should be String types.

[sinewavey]

Sure thing. Is the docstring description where you're referring to more info/expectations on data?

[RhapsodyInGeek]

Basically. Just something that says "this is specifically what it's used for, this is the format it needs to be in". Of course we should still try to keep it short as possible, but a bit more explanation on a poorly documented NRC feature would be helpful.

[RhapsodyInGeek]

Same as above. Should be default_build_menu_commands. Should also have a more expository description, to state exactly what it's for, and explain that the keys should be Strings while the values can be either Strings or Arrays of Strings.

[RhapsodyInGeek]

This should be a for loop that loops through the build commands array, since you can have multiple commands in one build option. Even though we haven't found a use for multiple in our own projects, we should still leave the theoretical possibility open since it's easy enough to implement it.

Better still would be an option to support both, since sometimes you only want the one option:

	for key in default_build_menu_commands.keys():
		file.store_string("'\t<build name=" + key + ">\n")
		if default_build_menu_commands[key] is String:
			file.store_string("\t\t<command>" + default_build_commands[key] + "</command>\n"
		elif default_build_menu_commands[key] is Array:
			var commands: Array = default_build_menu_commands[key]
			for command in commands:
				if command is String:
					file.store_string("\t\t<command>" + (command as String) + "</command>\n"
				else:
					push_error("ERROR: A command in build option \"" + key + "\" commands Array is not a String")
		else:
			push_error("ERROR: Build option \"" + key + "\" command is not a String or Array of Strings")

[RhapsodyInGeek]

Should do a safety check to make sure we're using a string here.

		for key in default_build_menu_variables.keys():
			if default_build_menu_variables[key] is String:
				file.store_string('\t<var name="%s">%s</var>\n' % [key, build_xml_variables[key]])
			else:
				push_error("ERROR: Build menu variable \"" + key + "\" is not a String")

[sinewavey]

Sure - %s casts to string I thought but it should still probably be caught anyway.

[RhapsodyInGeek]

AFAIK not all types can be cast to String though. I know there's a special str(value) in GDScript but does %s use that to convert it to String or does it just do the equivalent of value as String?

[sinewavey]

I'm not sure, actually, so that's a great point. Will fix it's caught by the type checks

[RhapsodyInGeek]

@sinewavey just wanted to make sure this was all good to go before merging? I read it over again and it looks good to me, but still wanted to double check with you.

[sinewavey]

Sounds good to me