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

Add fill method to Arrays and PackedArrays #46476

Merged
merged 1 commit into from
Apr 28, 2021

Conversation

DarknessCatt
Copy link
Contributor

@DarknessCatt DarknessCatt commented Feb 27, 2021

Closes godotengine/godot-proposals#2181
Based on the discussion on #46173

Adds a fill(value) method for Arrays and PackedArrays that allows filling the entire array with a single value.

Example usage:

var arr = Array()
arr.resize(4)
arr.fill(3.14)
print(arr) # [3.14, 3.14, 3.14, 3.14]

I would also like to thank @anberino. He ran all tests for me (since my gpu doesn't support vulkan) and this PR wouldnt be possible without his help.

@Xrayez
Copy link
Contributor

Xrayez commented Feb 27, 2021

I'm not sure about the extra logic for duplicating container types. Things like Array are passed by reference and is documented behavior, so I'd expect fill() to allocate needed elements to hold the same Array reference. Perhaps this can be made configurable. But I find it difficult to find a use case for filling an array elements with arrays/dictionaries, I think in most cases the method would be used by structures which are passed by value rather than reference.

@DarknessCatt
Copy link
Contributor Author

While I agree it's a bit counter intuitive to duplicate, I though it would be a little weird to have an array where every position is the same object.

However, I can make it optional or simply remove the duplication, If requested.

core/variant/array.cpp Outdated Show resolved Hide resolved
@DarknessCatt
Copy link
Contributor Author

Due to the reasons pointed by @Xrayez, I've removed the duplication on container types.

This should make the method more consistent and avoid future bugs related to duplicate(...).

@Xrayez
Copy link
Contributor

Xrayez commented Feb 28, 2021

I suggest refactoring the code this way:

// vector.h

template <class T>
void Vector<T>::fill(T p_elem) {
	T *p = ptrw();
	for (int i = 0; i < size(); i++) {
		p[i] = p_elem;
	}
}
// array.cpp

void Array::fill(const Variant &p_value) {
	ERR_FAIL_COND(!_p->typed.validate(p_value, "fill"));
	_p->array.fill(p_value);
}
  1. Eliminates code duplication.
  2. Improves performance. set() uses a write() proxy on each element access. Using ptrw() allows to modify the vector's contents directly. I've got x3 speed up on my machine on large arrays this way (tested on PackedByteArray).
  3. Consistent with other Array method implementations.

@DarknessCatt
Copy link
Contributor Author

Thanks for the feedback, @Xrayez.

I've updated the code and the documentation with your chances. I've also added the documentation for PackedVector2Array and PackedVector3Array, since I forgot to add them in the previous commits.

<argument index="0" name="value" type="Variant">
</argument>
<description>
Inserts a new element at every position of the array.
Copy link
Member

@akien-mga akien-mga Mar 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like this was adapted from the insert description, that doesn't sound clear to me. I'd suggest instead:

Suggested change
Inserts a new element at every position of the array.
Initializes all elements in the array with the given value. This can typically be used together with [method resize] to create an array with a given size and initialized elements:
[codeblocks]
[gdscript]
var array = []
array.resize(10)
array.fill(0) # Initialize the 10 elements to 0.
[/gdscript]
[csharp]
var array = new Godot.Collections.Array{};
array.Resize(10);
array.Fill(0); // Initialize the 10 elements to 0.
[/csharp]
[/codeblocks]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code example might not need to be added to all Packed*Array equivalent descriptions though, might be a bit redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, it was adapted from insert. I will update the descriptions ASAP, but I might change the word "initialize" to something else. I don't know if it's just me, but it gives me the ideia of something that you do when creating the array, but you can call array.fill() whenever (you could use it to "reset" an array or something).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it could be "Sets all elements in the array to the given value." or "Assigns the given value to all elements in the array."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the documentation. Thanks for the help and suggestions!

<argument index="0" name="value" type="Color">
</argument>
<description>
Assigns the given value to all elements in the array. This can typically be used together with [method resize] to create an array with a given size and initialized elements
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just missing a dot at the end of the sentence for all Packed*Array descriptions, otherwise it's all good.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, my fault for copy pasting and not double checking. Thanks for the heads up, it's should be fixed now.

@akien-mga
Copy link
Member

Thanks!

@Zireael07
Copy link
Contributor

Can this get backported to 3.x?

@Calinou
Copy link
Member

Calinou commented Mar 25, 2022

Can this get backported to 3.x?

Yes, but this PR needs to be manually redone for 3.x.

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.

Ability to init an array of specified size, pre-filled with a specified init value
5 participants