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

GDScript: Fix typed arrays #69248

Merged
merged 1 commit into from
Jan 31, 2023
Merged

Conversation

vonagam
Copy link
Contributor

@vonagam vonagam commented Nov 27, 2022

This is complete rework how typed arrays do function. In terms of interactions now typed arrays act like packed arrays do.

No more implicit conversions between arrays on assigns.

var ints: Array[int] = [1]
var floats: Array[float] = ints # will not work
# same as you cannot do this:
var ints := PackedInt64Array()
var floats: PackedFloat64Array = ints

Array (without a parameter) for typed arrays is what Variant is for other types. It is possible to assign typed array to assignable (variable/parameter/constant) with type of basic array and it is possible to do the opposite, but later fails if types do not match.

var ints: Array[int] = [1]
var basic: Array = ints
ints = basic

var floats: Array[float] = [1.0]
basic = floats
ints = basic # runtime error because assigned is Array[float] and not Array[int]

basic = [2]
ints = basic # runtime error because assigned is Array and not Array[int]

Explicit inference no longer tries to guess what element type an array has, it will be typed as basic one.

var infer := [1, 2, 3] # type is Array, not Array[int]

Typed arrays in constants now work properly.

const ints: Array[int] = [1] # value now is really Array[int]

There is no implicit conversions between arrays, but there is for array literals. In all examples above literal on right side is a usual array literal, but it gets converted during analysis to be of proper type and fails if it is impossible to do. It applies to variable/parameters/constants initializers, assignments, returns, arguments and casts:

var ints: Array[int] = [1] # init
ints = [2] # assign
var floats := [3.0] as Array[float] # cast
var make_bools = func() -> Array[bool]: return [true] # return
func pass_strings(strings: Array[String]): pass # this is a member function, lambdas are untyped
pass_strings(['ok']) # arg 

typed_assign now does only one thing - replaces contents of an array with contents from another with conversions if needed. It is also renamed to simple assign.

var ints: Array[int] = [1]
var floats: Array[float] = []
floats.assign(ints)

Fixed resize not using resize_zeroed and not initializing new elements in typed arrays. This was leading to memory corruptions and crashes.

In VariantInterals::initialize add missing initailization for Projection and Color (alpha needs to be set at 1).

Added serialization/deserialization of typed arrays. Though it does not handle custom classes at the moment.

Fixed undo/redo for packed arrays, for some reason they were excluded from that before.

Changed editors for arrays/dictionaries a little - no need to do recursively duplicate them on each rerender, do it only when value is about to be modified.

Fixed default values for arrays/dictionaries not showing in editor.

Fixed GDScriptInstance::set not converting values when setters involved.

Related issues:
Fixes #53847 (about parameters).
Fixes #54643 (about parameters).
Fixes #55916 (about constants).
Fixes #56389 (about documentation).
Fixes #58285 (about editor).
Fixes #58636 (about parameters).
Fixes #62753 (about editor).
Fixes #63502 (about default values).
Fixes #63570 (about resize).
Fixes #67653 (about editor).
Fixes #67889 (about literals).
Fixes #68978 (about constants).
Fixes #69198 (about constants).
Fixes #69839 (about assigns).
Fixes #70021 (about constants).
Fixes #70235 (about resize).
Fixes #71607 (about editor).
Fixes #71653 (about parameters).
Fixes #71756 (about enums).
Fixes #71815 (about editor).
Fixes #72349 (about parameters).

@vonagam vonagam requested a review from a team as a code owner November 27, 2022 10:24
@Chaosus Chaosus added this to the 4.0 milestone Nov 30, 2022
@vonagam

This comment was marked as outdated.

@vonagam

This comment was marked as outdated.

@vonagam vonagam requested a review from a team as a code owner December 4, 2022 10:58
@vonagam
Copy link
Contributor Author

vonagam commented Dec 4, 2022

Fixed another thing about arrays (not connected to literals or constants) - typed assigns. Right now if element types are somewhat compatible a destination array just references a source array losing its own type in a process.

class A: pass
class B extends A: pass

func _ready():
  var b: Array[ B ] = [ B.new() ]
  var a: Array[ A ] = b # before: a references b, becomes Array[ B ], after: a copies b contents
  a.push_back( A.new() )
  a.push_back( B.new() )
  print( b.size() ) # before: 2, now: 1
  print( a.size() ) # before: 2, now: 3
  check( a ) # before: error, now: works

func check( a: Array[ A ] ) -> void: print( 'check' )

To make it correct array a should copy contents of array b and maintain its own type info.

By the way, initial commit fixes #69198 and #67889.

@Chaosus
Copy link
Member

Chaosus commented Dec 4, 2022

You need to squash the commits as required by our pipeline (https://docs.godotengine.org/en/latest/community/contributing/pr_workflow.html#modifying-a-pull-request).

@vonagam
Copy link
Contributor Author

vonagam commented Dec 4, 2022

Oh, I was doing that initially, but then it grew and I assume became harder to review in a single commit. Also I assume that third commit might be dropped (about flatness/deepness of constants).

@vonagam

This comment was marked as outdated.

@vonagam

This comment was marked as outdated.

@vonagam
Copy link
Contributor Author

vonagam commented Dec 4, 2022

After I made is_type_compatible to use exact equality for element types when no implicit conversion is allowed I've encountered bugs when a class type was not equal to the same class type. Turns out in is_type_compatible compassion was done by equating fqcns (fully-qualified class name) while == operator only checked equality of pointers to ClassNode, but it seems like the same class may have multiple ClassNode instances pointing to it (hence, I assume, why is_type_compatible used fqcn). So I added comparison of fqcn in == operator for classes types. Right now fqcn is a String, but is there anything preventing it from being NodePath?

@vonagam
Copy link
Contributor Author

vonagam commented Dec 5, 2022

Fixed another major thing about array assigns.

var first: Array[ A ] = []
var second: Array[ A ] = first
var value: Variant = A.new()
second = [ value ]
print( first.size() ) # before: 1, after: 0
print( second.size() ) # 1

Currently Array::_assign changes contents of an array it points to if there is a type mismatch instead of creating new array and properly copying contents. Here second starts referencing the same array as first as they have the same type, but during the assignment of the array literal which is Array[Variant] instead of trying to creating new Array[A] from what is given it simply replaces an underlying vector of the array it already has.

@vonagam

This comment was marked as outdated.

@vonagam vonagam force-pushed the fixing-typed-arrays branch 2 times, most recently from f949908 to d58b7f8 Compare December 12, 2022 09:34
@vonagam vonagam changed the title Fix typing of array literals with expected parameterised type Fix typed arrays literals, assigns, default params, constants Dec 12, 2022
@vonagam vonagam force-pushed the fixing-typed-arrays branch 2 times, most recently from 6c5ca93 to ad50cc1 Compare December 13, 2022 00:34
@vonagam

This comment was marked as outdated.

@vonagam
Copy link
Contributor Author

vonagam commented Jan 30, 2023

copy and clone are generic names for such methods in some languages (clone does what duplicate does in gdscript). C# is a little more relevant than other languages because it is a second officially supported language. For array method name assign might work, but for dictionaries (or other types) it might become confusing - merge and assign (where there is Object.assign in js which what merge does). copy has a required argument with an array to copy so it will be clear that it is not what duplicate does.

@vonagam vonagam requested a review from a team as a code owner January 30, 2023 20:34
@kleonc
Copy link
Member

kleonc commented Jan 30, 2023

Note that C#'s Array.Copy is a static method which takes in both source and destination arrays (and it's capable of copying only a specific range of elements, possibly starting at specific destination index). A non-static version is Array.CopyTo which copies the whole array to the specified destination (array + start index). But what's important: these methods actually do perform copying.

In case of "our method" it might end up just assigning the internal array which is copy-on-write and hence actual copying can happen later. So copy (or copy_from which I think would be a better name) would be a little misleading. On the other hand assign does make sense to me, that's pretty much what this method does.

@vonagam
Copy link
Contributor Author

vonagam commented Jan 30, 2023

In case of "our method" it might end up just assigning the internal array which is copy-on-write and hence actual copying can happen later. So copy (or copy_from which I think would be a better name) would be a little misleading.

I don't how it is misleading. It changes contents to match those of source array. That's it. copy-on-write is just an internal optimization/implementation, has nothing to do with behavior.

I see that Python has copy() on List. So I guess it is no go. I will rename it to assign.

@vonagam vonagam force-pushed the fixing-typed-arrays branch 2 times, most recently from 62e16fc to 245c7cc Compare January 31, 2023 09:11
@vonagam
Copy link
Contributor Author

vonagam commented Jan 31, 2023

I added such code to SceneState::instantiate in PackedScene:

if (value.get_type() == Variant::ARRAY) {
  Array set_array = value;
  bool is_get_valid = false;
  Variant get_value = node->get(snames[nprops[j].name], &is_get_valid);
  if (is_get_valid && get_value.get_type() == Variant::ARRAY) {
    Array get_array = get_value;
    if (!set_array.is_same_typed(get_array)) {
      value = Array(set_array, get_array.get_typed_builtin(), get_array.get_typed_class_name(), get_array.get_typed_script());
    }
  }
}

So if an array being set but it is possible to get a default one and the element types are different then try to convert the incoming array to type of currently present one.

Not super ideal - proper is to use get_property_list and parse the hint to get the type (to handle setter-only exports, but much more code though), not only packed scenes use parser/writer.

@vonagam
Copy link
Contributor Author

vonagam commented Jan 31, 2023

Added the same logic to ResourceLoaderText::load().

@akien-mga akien-mga merged commit 99a44f8 into godotengine:master Jan 31, 2023
@akien-mga
Copy link
Member

Thanks!

@anvilfolk
Copy link
Contributor

Congratulations on this huge PR! I know it was a loooooooooot of work with a looooooooot of subtleties, but so very important! Very very excited this got in! :) :) :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment