Add as_slice and as_mut_slice for Packed*Array #137

indubitablement2 · 2023-03-04T17:44:16Z

Piggyback off of ptr/ ptr_mut.

godot-core/src/builtin/packed_array.rs

Bromeon

Thanks a lot for this 👍

I think slices should be safe, since packed arrays are unique (not ref-counted). Would need to spend some deeper thoughts on it though, e.g. if the CoW backing implementation has any impact.

Could you maybe add one test case for each method, in itest/rust/src/packed_array_test.rs? It's enough to choose one concrete specialization, e.g. for i32, no need for genericity.

bors try

godot-core/src/builtin/packed_array.rs

bors · 2023-03-05T00:08:20Z

try

Build succeeded:

full-ci

indubitablement2 · 2023-03-05T13:13:49Z

Added some tests and safety comments.

lilizoey · 2023-03-05T13:19:58Z

You say Packed*Array is not aliased because it is COW. but they are in fact pass by reference at least within gdscript, and it seems the cow-ness of them is mostly not visible to a gdscript user at least. i am unsure if this is the case with gdextension as well.

see: godotengine/godot#36492 (comment)

does this cause any problems?

indubitablement2 · 2023-03-05T13:46:58Z

You say Packed*Array is not aliased because it is COW. but they are in fact pass by reference at least within gdscript, and it seems the cow-ness of them is mostly not visible to a gdscript user at least. i am unsure if this is the case with gdextension as well.

see: godotengine/godot#36492 (comment)

does this cause any problems?

Good catch! I only tested within rust. I'm not sure what happen when shared between rust and godot. I will make a test.

Bromeon · 2023-03-05T13:54:07Z

itest/rust/src/packed_array_test.rs

+        "arrays should share the same buffer"
+    );
+
+    // array2 should become a copy of array1 since rc > 1.


The "RC > 1" is somewhat irrelevant, now that Godot 4 is released.

I would rather mention that as_mut_slice triggers a copy-on-write.

…that would look like cow

indubitablement2 · 2023-03-05T16:49:18Z

Alright, added tests for when arrays are shared between rust and godot. Looks like packed array are always cow except when sharing between godot and godot. You can even make array behave like godot 3 used to by passing it to and from rust.

lilizoey · 2023-03-05T16:54:08Z

Alright, added tests for when arrays are shared between rust and godot. Looks like packed array are always cow except when sharing between godot and godot. You can even make array behave like godot 3 used to by passing it to and from rust.

Ah ok, that should probably be properly documented. It is a bit unintuitive that these are passed by value in rust but passed by reference in gdscript.

Bromeon · 2023-03-05T17:04:42Z

Regarding merge conflicts, I made #[itest] self-registering, so you can throw out all the ceremony to call each test individually. Should be easy to rebase 🙂

indubitablement2 · 2023-03-05T17:19:41Z

Alright, added tests for when arrays are shared between rust and godot. Looks like packed array are always cow except when sharing between godot and godot. You can even make array behave like godot 3 used to by passing it to and from rust.

Ah ok, that should probably be properly documented. It is a bit unintuitive that these are passed by value in rust but passed by reference in gdscript.

I agree, but this is on the godot side. As far as gdextension is concerned, it's all cow.

This is what I've gathered:

_	_	Behavior
godot	godot	ref
godot	rust	cow
godot	cpp	copy (eg: Image.create_from_data())
rust	cpp	cow

Bromeon

Thanks for the updates!

bors try

Bromeon · 2023-03-05T22:12:49Z

itest/rust/src/packed_array_test.rs

+#[godot_api]
+impl PackedArrayTest {
+    #[func]
+    fn set_array(&mut self, array: PackedByteArray) {
+        self.array = array;
+    }
+
+    #[func]
+    fn get_array(&self) -> PackedByteArray {
+        self.array.clone()
+    }
+
+    #[func]
+    fn are_separate_buffer(&self, other: PackedByteArray) -> bool {
+        self.array.as_slice().as_ptr() != other.as_slice().as_ptr()
+    }
+
+    #[func]
+    fn do_mutable_access(&mut self) {
+        self.array.as_mut_slice();
+    }
+}


This type is currently acting as an abstraction that makes the tests quite a bit harder to understand, because one array is encapsulated, while the other is not.

I would remove the setter/getter as well as any state (struct fields), and just provide two functions:

is_buffer_shared(&self, lhs: &PackedArray, rhs: &PackedArray)

basically the opposite of are_separate_buffer, but doesn't use the double negation

trigger_cow(&self, array: &mut PackedArray)

same as do_mutable_access, but expressing the intent better

The receiver &self is only necessary because #[func] expects it, but you don't access the object's state. You also don't need to have a #[base] field or a impl GodotExt.

And if you have this abstraction, use it in Rust! 🙂
(see other comment)

Bromeon · 2023-03-05T22:14:38Z

itest/rust/src/packed_array_test.rs

+        array1.as_slice().as_ptr(),
+        array2.as_slice().as_ptr(),


Here you could use is_buffer_shared() (see other comment).

Same for similar occurrences.

Bromeon · 2023-03-05T22:16:49Z

itest/rust/src/packed_array_test.rs

+    let array1 = PackedByteArray::from(&[1, 2]);
+    let mut array2 = array1.clone();
+    let array3 = array1.to_variant();
+    let array4 = godot::engine::Image::create_from_data(


Could you name those more meaningfully?

array1 -> array array2 -> cloned array3 -> variant array4 -> image_data

bors · 2023-03-05T22:30:54Z

try

Build succeeded:

full-ci

Bromeon · 2023-03-06T15:21:39Z

Why close? 🤔

307: Implement `PackedArray::as_slice()`, `as_slice_mut()` r=Bromeon a=Bromeon Resumes abandoned work of #137. Closes #306. Open points: 1. Test CoW across GDScript/Rust boundaries? 2. We now have `slice`, `as_slice` and `as_mut_slice`, where the first returns another array. I wonder if it would be clearer to name it `sub_array` or something instead of `slice`? bors try Co-authored-by: Jan Haller <bromeon@gmail.com>

Add as_slice and as_mut_slice for Packed*Array

0693feb

Bromeon added feature Adds functionality to the library c: core Core components labels Mar 4, 2023

lilizoey reviewed Mar 4, 2023

View reviewed changes

godot-core/src/builtin/packed_array.rs Show resolved Hide resolved

Bromeon reviewed Mar 4, 2023

View reviewed changes

godot-core/src/builtin/packed_array.rs Show resolved Hide resolved

godot-core/src/builtin/packed_array.rs Show resolved Hide resolved

bors bot added a commit that referenced this pull request Mar 4, 2023

Try #137:

11f95c8

add tests

c4e3e92

Add safety comments

5913a24

Remove tabs in safety comments to be consistent with others

10f522a

Bromeon reviewed Mar 5, 2023

View reviewed changes

indubitablement2 added 3 commits March 5, 2023 09:32

Add test when array is stored in a variable and an Image

ceca3a1

Add tests for shared array with godot

0050507

use set insteaf of push_back as push_back could trigger resizing and …

a1a9d71

…that would look like cow

Sentences start with a capital letter

a2820d5

indubitablement2 added 3 commits March 5, 2023 12:28

Final test for shared array between var and object

6f809e6

Merge remote-tracking branch 'upstream/master'

5d0f0b9

Fix typos and inconsistencies

430f9f0

Bromeon reviewed Mar 5, 2023

View reviewed changes

bors bot added a commit that referenced this pull request Mar 5, 2023

Try #137:

d3ef289

indubitablement2 closed this Mar 6, 2023

This was referenced Jun 12, 2023

Implement PackedArray::as_slice #306

Closed

Implement PackedArray::as_slice(), as_slice_mut() #307

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add as_slice and as_mut_slice for Packed*Array #137

Add as_slice and as_mut_slice for Packed*Array #137

indubitablement2 commented Mar 4, 2023 •

edited

Loading

Bromeon left a comment

bors bot commented Mar 5, 2023

indubitablement2 commented Mar 5, 2023 •

edited

Loading

lilizoey commented Mar 5, 2023

indubitablement2 commented Mar 5, 2023

Bromeon Mar 5, 2023

indubitablement2 commented Mar 5, 2023

lilizoey commented Mar 5, 2023

Bromeon commented Mar 5, 2023

indubitablement2 commented Mar 5, 2023

Bromeon left a comment

Bromeon Mar 5, 2023

Bromeon Mar 5, 2023

Bromeon Mar 5, 2023

bors bot commented Mar 5, 2023

Bromeon commented Mar 6, 2023

Add as_slice and as_mut_slice for Packed*Array #137

Add as_slice and as_mut_slice for Packed*Array #137

Conversation

indubitablement2 commented Mar 4, 2023 • edited Loading

Bromeon left a comment

Choose a reason for hiding this comment

bors bot commented Mar 5, 2023

try

indubitablement2 commented Mar 5, 2023 • edited Loading

lilizoey commented Mar 5, 2023

indubitablement2 commented Mar 5, 2023

Bromeon Mar 5, 2023

Choose a reason for hiding this comment

indubitablement2 commented Mar 5, 2023

lilizoey commented Mar 5, 2023

Bromeon commented Mar 5, 2023

indubitablement2 commented Mar 5, 2023

Bromeon left a comment

Choose a reason for hiding this comment

Bromeon Mar 5, 2023

Choose a reason for hiding this comment

Bromeon Mar 5, 2023

Choose a reason for hiding this comment

Bromeon Mar 5, 2023

Choose a reason for hiding this comment

bors bot commented Mar 5, 2023

try

Bromeon commented Mar 6, 2023

indubitablement2 commented Mar 4, 2023 •

edited

Loading

indubitablement2 commented Mar 5, 2023 •

edited

Loading