godot icon indicating copy to clipboard operation
godot copied to clipboard

Add PackedByteArray conversion to PackedVector2Array, PackedVector3Array, PackedVector4Array and PackedColorArray

Open OsakiTsukiko opened this issue 2 years ago • 12 comments
trafficstars

adds to_vector2_array, to_vector3_array, and to_color_array methods on PackedByteArray to convert to the corresponding types, as prior you could only convert from said type to PackedByteArray (with to_byte_array)

  • Bugsquad edit, closes: https://github.com/godotengine/godot-proposals/issues/8139

OsakiTsukiko avatar Apr 15 '23 00:04 OsakiTsukiko

I haven't done one for PackedStringArray as I think the to_byte_array method on PackedStringArray is broken (I'll look some more into it tomorrow)

OsakiTsukiko avatar Apr 15 '23 00:04 OsakiTsukiko

Note that for Vector2/3/4 the underlaying data type is not always a float, it's real_t which can be a 32-bit float or 64-bit double depending on Godot build settings. But it might be best to just use full class size. So instead of using sizeof(float) * 2 for Vector2, just use sizeof(Vector2) directly. You'll also have to update the documentation for this.

PackedStringArray also only stores the String objects themselves, which are basically just pointers to the actual text data. It doesn't actually store tightly packed text data or anything like that. So there is probably no point in adding that converter.

Also remember to squish your commits into a single commit, see PR Workflow.

bitsawer avatar Apr 15 '23 04:04 bitsawer

Note that for Vector2/3/4 the underlaying data type is not always a float, it's real_t which can be a 32-bit float or 64-bit double depending on Godot build settings. But it might be best to just use full class size. So instead of using sizeof(float) * 2 for Vector2, just use sizeof(Vector2) directly. You'll also have to update the documentation for this.

Oh, I wasn't sure if I could use sizeof on custom data types

PackedStringArray also only stores the String objects themselves, which are basically just pointers to the actual text data. It doesn't actually store tightly packed text data or anything like that. So there is probably no point in adding that converter.

Hmm, yea, I thought so. Is there any point to the to_byte_array method on the String? I do not see much use for a byte array of references, and it's quite unintuitive, especially for beginners (It doesn't really specify anywhere in the docs that it only stores references, but that it packs data tightly (in-built docs), and even worse on the online docs: Returns a PackedByteArray with each string encoded as bytes.)

Also remember to squish your commits into a single commit, see PR Workflow.

Oh, Sure!

OsakiTsukiko avatar Apr 15 '23 08:04 OsakiTsukiko

Duplicate/alternative: #65811.

kleonc avatar Apr 28 '23 20:04 kleonc

why isnt this approved already? friend is running this PR on his own custom compiled version with no problem, this is essential for compute shaders, and compute shaders are a very big thing

ghost avatar Aug 30 '23 22:08 ghost

why isnt this approved already? friend is running this PR on his own custom compiled version with no problem, this is essential for compute shaders, and compute shaders are a very big thing

There is a related proposal, but it's not very focused on this particular feature. Not much consensus has been reached, and we don't know whether this PR or https://github.com/godotengine/godot/pull/65811 is better yet.

Calinou avatar Aug 30 '23 22:08 Calinou

Please squash your commits into one, see here

AThousandShips avatar May 19 '24 18:05 AThousandShips

ok, I hope I did it right 🥲

OsakiTsukiko avatar May 19 '24 19:05 OsakiTsukiko

Looks great! Just one last detail, could you please update the title of the commit to match the PR to make it a bit clearer without low level details of the code, you do this with:

git commit --amend
git push --force

Thank you and this looks great!

AThousandShips avatar May 20 '24 07:05 AThousandShips

Will it be merged? Since it is essential for working with compute shaders.

nvl-ez avatar Jan 22 '25 19:01 nvl-ez

It will hopefully be merged early in 4.5, we're in feature freeze now

AThousandShips avatar Jan 22 '25 19:01 AThousandShips

It will hopefully be merged early in 4.5, we're in feature freeze now

Looking forward to this merge for 4.5!

tjpalmer avatar Apr 12 '25 15:04 tjpalmer

Thanks! And congrats for your first merged Godot contribution :tada:

akien-mga avatar Jun 12 '25 20:06 akien-mga

Thanks! And congrats for your first merged Godot contribution :tada:

Thank you! Tho it's not my first 😅

OsakiTsukiko avatar Jun 12 '25 20:06 OsakiTsukiko

First with this GitHub identity / username at least according to GitHub and the git log :D

akien-mga avatar Jun 12 '25 21:06 akien-mga

First with this GitHub identity / username at least according to GitHub and the git log :D

I thought that was weird, so I looked a bit into it, and it doesn't show in the log because it's not on the master branch. (Tho tbf that was just a small fix, so this is my first actual contribution. I was just interested as to how you knew this was my first commit).

OsakiTsukiko avatar Jun 12 '25 21:06 OsakiTsukiko