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] export var array reduced to const bug fixed #38976

Closed

Conversation

ThakeeNathees
Copy link
Contributor

@ThakeeNathees ThakeeNathees commented May 23, 2020

Fix: #20436

## working
export var arr1 = []
export var arr2 = []

export(Array) var arr1 = []
export(Array) var arr2 = []

export(Array) var arr1 := []
export(Array) var arr2 := []

export(Array) var arr1 :Array= []
export(Array) var arr2 :Array= []

export var arr1 :Array= []
export var arr2 :Array= []

export var arr1 :Array= Array()
export var arr2 :Array= Array()

export(Vector2) var arr1 = Vector2i()
export(Vector2) var arr2 = Vector2i()

it's working for every scenario above, (seems like only Array and Dictionary are exportable reference types)

@ThakeeNathees ThakeeNathees force-pushed the export-array-bug-fix branch from a8c3b64 to 077b2f2 Compare May 23, 2020 08:23
@akien-mga akien-mga added bug cherrypick:3.x Considered for cherry-picking into a future 3.x release topic:gdscript labels May 23, 2020
@akien-mga akien-mga added this to the 4.0 milestone May 23, 2020
@ThakeeNathees ThakeeNathees force-pushed the export-array-bug-fix branch from 077b2f2 to 0c43d17 Compare May 23, 2020 08:38
@bojidar-bg
Copy link
Contributor

Doesn't this break default values, e.g.:

export var x = [4]
export var y = 91
export var z = Vector3(7,6,4)

@ThakeeNathees
Copy link
Contributor Author

ThakeeNathees commented May 23, 2020

@bojidar-bg even without reduced to const = true they reduced to const, so they'll work as old,
EDIT
except for export var x = [4] this is an ArrayNode and not const so I check if export type is not const but array or dictionary (only Array and Dictionary are exportable reference types) then the export type will be array or dictionary

export var z1 = Vector3(7,6,4)
export var z2 = Vector3(7,6,4)

func _ready():
	z1.x = 12
	print(z1)
	print(z2)
	pass

--the output--
(12, 6, 4)
(7, 6, 4)

@bojidar-bg
Copy link
Contributor

bojidar-bg commented May 23, 2020

@ThakeeNathees I would still suggest the same thing as before: call _reduce_expression(expr, true) in order to reduce the node to a constant only for the default value (but not to keep the reduced node around in the tree).

That way there would be no regression for export var x = [5, 4] or maybe even expressions like export var z = CONST + sin(6).

@ThakeeNathees
Copy link
Contributor Author

ThakeeNathees commented May 23, 2020

@bojidar-bg there is no regression for export var x = [5, 4] or export var z = CONST + sin(6) even In that case how about this fix (a simplified version)

@@ -4926,6 +4926,12 @@ void GDScriptParser::_parse_class(ClassNode *p_class) {

	if (autoexport && !member.data_type.has_type) {
		if (subexpr->type != Node::TYPE_CONSTANT) {
			_set_error("Type-less export needs a constant expression assigned to infer type.");
			return;
		}

		ConstantNode *cn = static_cast<ConstantNode *>(subexpr);
		if (cn->value.get_type() == Variant::NIL) {
			_set_error("Can't accept a null constant expression for inferring export type.");
			return;
		}

		if (!_reduce_export_var_type(cn->value, member.line)) {
			return;
		}

		member._export.type = cn->value.get_type();
		member._export.usage |= PROPERTY_USAGE_SCRIPT_VARIABLE;
		if (cn->value.get_type() == Variant::OBJECT) {
			Object *obj = cn->value;
			Resource *res = Object::cast_to<Resource>(obj);
			if (res == nullptr) {
				_set_error("The exported constant isn't a type or resource.");
				return;
			}
			member._export.hint = PROPERTY_HINT_RESOURCE_TYPE;
			member._export.hint_string = res->get_class();
		}
+
+		if (cn->value.get_type() == Variant::ARRAY) {
+			subexpr = constant_node_to_array_node(cn);
+		} else if (cn->value.get_type() == Variant::DICTIONARY) {
+			subexpr = constant_node_to_dict_node(cn);
+		}
	}
#ifdef TOOLS_ENABLED

@bojidar-bg
Copy link
Contributor

there is no regression for export var x = [5, 4]

I'm at a loss. How does it work if default value is set only if the node is a ConstantNode:

member.default_value = cn->value;

But subexpr is an ArrayNode at this point:

Node *subexpr = _parse_and_reduce_expression(p_class, false, false);


In that case how about this fix (a simplified version)

Looks patchy to me. Why would we need to generate the ArrayNode again, when it already exists before calling _reduce_expression?
Are there any problems with my idea of forcibly reducing to constant only when trying to get the default value?

@ThakeeNathees
Copy link
Contributor Author

ThakeeNathees commented May 24, 2020

@bojidar-bg

  1. if autoexport || member._export.type != Variant::NIL -> subexpr must be constant node, if not error will set. in my pr I'm not trying to reduce it to const but if it's not constant node error will set unless if it's an array node or dictionary node -> so except for array and dictionary it'll be the same.

  2. Are there any problems with my idea of forcibly reducing to constant only when trying to get the default value?

  • if array node cn_arr = _reduce_expression(arraynode, p_reduce_const=true); if cn_arr is not constant node set error else member.default_value = cn_arr => value; (TODO:)
  • else if dictionary node same (TODO:)
  • else if constant node nothing new
  • else set error
  1. (TODO:) if array/dict node after reduced to constant node I'll check if all elements are exportable using _reduce_export_var_type()(Fix: export var type reduce() implemented #36927) (unless it'll case regression : Dictionary of References export breaks compilation #36899)

I'm at a loss. How does it work if default value is set only if the node is a ConstantNode

even with out default value it's working may be default value is NIL variant not sure, but now we can set it a default value from my point 2.

@bojidar-bg
Copy link
Contributor

  1. Default values for exported variables are a feature which should continue working if possible. This includes default values for arrays/dictionaries.

  2. if array node cn_arr = _reduce_expression(arraynode, p_reduce_const=true); if cn_arr is not constant node set error else member.default_value = cn_arr => value; [...]

    • Note that _reduce_expression returns the node itself when called on a ConstantNode. So, there is no need to check for ArrayNode/DictionaryNode/ConstantNode -- it already happens.
    • If the result of _reduce_expression(.., true) is not a ConstantNode, you are correct that it shouldn't be used as a default value.
  3. I don't think that using _reduce_expression that way would cause a regression, but it is a nice thing to keep in mind and check.

@andrew121410
Copy link

Any information on this?

@ThakeeNathees
Copy link
Contributor Author

@andrew121410 thanks for pointing out. marking this as WIP for now. (#38976 (comment) TODOs), I'll complete this ASAP (in a couple of days) and It'll ready for reivew.

@ThakeeNathees ThakeeNathees marked this pull request as draft June 9, 2020 07:14
@ThakeeNathees ThakeeNathees changed the title export var array reduced to const bug fixed [WIP] export var array reduced to const bug fixed Jun 9, 2020
@JestemStefan
Copy link
Contributor

@ThakeeNathees Hi! I saw your PR that fixed this issue for 4.0. Are you still working on backport for 3.x or you dropped it and someone can salvage your code?

@akien-mga
Copy link
Member

Superseded by #41983.

@akien-mga akien-mga closed this Feb 11, 2023
@akien-mga akien-mga added archived and removed cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Feb 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Export arrays in singleton end up shared if initialized to same value (fixed in master)
5 participants