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

Enchancement: to_raw_bytes(Variant) ->PackedByteArray: in GlobalScope #80500

Closed
wants to merge 9 commits into from

Conversation

pkpro
Copy link
Contributor

@pkpro pkpro commented Aug 10, 2023

What

 to_raw_bytes(Variant) -> PackedByteArray

A new function in a GlobalScope to convert a Variant into PackedByteArray which does not include an encoding information (no type information is encoded, no possibility to de-serialize from result)

Why

There is no simple way to prepare a byte stream for push constants or for SSBO updates of the compute shaders in GDScript. It is inconvenient, slow and error prone.

Here is a simple function that convert limited range of Variants (Array, int, float, bool, Vector2, Vector3,Vector4,Vector2i,Vector3i,Vector4i) into byte stream and returns it as PackedByteArray.

Nope, this function does not control the alignment if vec2,vec3 types, this is still responsibility of the user.

Here how it looks like:

    # Vectors are aligned to 16 bytes, so we need to pad vectors with some other 4 byte value like float, int or bool
    var array = [
        Vector3(camera_pos.x,camera_pos.y,camera_pos.z),
        float(seed),                                                    # padded for 16 byte alignment of the camera_position above
        Vector3(light_direction.x,light_direction.y,light_direction.z),
        float(scale),                                                   # padded for 16 byte alignment of the light_direction above
        [ 0, 1, true, 33.17, Vector3(0.1,0.2,0.3), false ]              # some other array values, Vector3 is padded for 16 byte alignment
                                                                            # by bool value, and before Vector3 there are 4 four-byte values
    ]
    
    var bytes:PackedByteArray = to_raw_bytes(array)                     # convert to raw bytes
    _rd.buffer_update(_buffer_rid, 0, bytes.size(), bytes)                # update existing SSBO

Bad

One should avoid using the result of this function with bytes_to_vars() function, because of the obvious possibility to not only get a garbage in return or impossibility to convert, but a danger of engine crash.

@AThousandShips
Copy link
Member

AThousandShips commented Aug 11, 2023

This warrants a proposal to discuss the specifics and the support for such a feature, make sure to fill in the form properly

I fail to see the need for this, it can be easily worked around with the existing encode methods, and doing so gives you full freedom to control the alignment

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

Thank you for contributing

Several issues, this needs to be rewritten based on this, and use more appropriate code IMO

You should probably use the encode_ methods, like encode_real, taking into account details

Error to_raw_bytes(const Variant &p_variant, PackedByteArray &out) {
switch (p_variant.get_type()) {
case Variant::INT: {
int32_t v = p_variant;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
int32_t v = p_variant;
int64_t v = p_variant;

int is 64 bits, not 32

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In GLSL uint and int are 32 bit, here goes the semantic conversion first. If value can't fit into 32 bits, so be it. However this must be documented that there are no 64 bit integers support in this method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a documentation note with describing lack of 64 bit integers support for this method.

core/io/marshalls.h Outdated Show resolved Hide resolved
core/io/marshalls.cpp Outdated Show resolved Hide resolved
core/io/marshalls.cpp Outdated Show resolved Hide resolved
core/io/marshalls.cpp Outdated Show resolved Hide resolved
Comment on lines 1832 to 1842
#ifdef REAL_T_IS_DOUBLE
double v = p_variant;
size_t offset = out.size();
out.resize(out.size() + sizeof(v));
memcpy(out.ptrw() + offset, &v, sizeof(v));
#else
float v = p_variant;
size_t offset = out.size();
out.resize(out.size() + sizeof(v));
memcpy(out.ptrw() + offset, &v, sizeof(v));
#endif
Copy link
Member

Choose a reason for hiding this comment

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

This is not correct, float in Variant is always 64 bits, only the values of Vector2/3/4 are 32/64 bits, this needs to be changed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

float is 32 bits and only 64 bits when REAL_T_IS_DOUBLE is defined.. Variant, does not matter as there is no mapping from Variant to GLSL types. Vectors in GLSL are 16 bytes aligned and Vector2 for example will take 2xfloat - 8 bytes and 8 pad bytes. Padding is not the part of the job for this function.

Copy link
Member

@AThousandShips AThousandShips Aug 11, 2023

Choose a reason for hiding this comment

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

Float in GDScript is 64 bits, always

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed use of the "double" completely, added semantic conversion to 32 bit float at a cost of precision.

Vector2i v = p_variant;
for (size_t elements = 0; elements < 2; elements++) {
to_raw_bytes(v[elements], out);
}
Copy link
Member

Choose a reason for hiding this comment

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

This is not correct either, the values of Vector2/3/4i are all 32 bits, whereas the int is always 64 bits, this needs to be handled specifically

Copy link
Contributor Author

Choose a reason for hiding this comment

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

int and uint in GLSL are 32 bit integers, though you are correct about GDScript int's, they are 64 bits and this have to be adressed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is fixed with the commit 3266fe5f9da822baba14b2998cf9823405ebdfc1, by converting doubles and uint64_t of the GDScript into 32 bit floats and integers.
Documentation updated: 3e7b1bf9417c03c547bb12f802e14d24c2617287

@pkpro
Copy link
Contributor Author

pkpro commented Aug 11, 2023

This warrants a proposal to discuss the specifics and the support for such a feature, make sure to fill in the form properly

I fail to see the need for this, it can be easily worked around with the existing encode methods, and doing so gives you full freedom to control the alignment

It is as I wrote: It is not easy, inconvenient, slow and error prone.

I'll fill out the proposal now.

@AThousandShips
Copy link
Member

AThousandShips commented Aug 11, 2023

You didn't mention the encode methods in PackedByteArray which is what I mean here, which allows a lot of flexibility, is easy, and not error prone IMO

Your suggested approach doesn't allow any flexibility on precision or details like that, which is a drawback, it also doesn't allow you to pass PackedInt*Array, PackedColorArray, or PackedFloat*Array, or even Color

Also I don't know if the example you provided is realistic, you will never send arbitrary data to a compute shader, you have to match the data format, so you can always use individual methods to do it, using the methods on the PackedByteArray, you should also know the size beforehand, so resizing the data to fit isn't really an issue either, IMO

@pkpro
Copy link
Contributor Author

pkpro commented Aug 11, 2023

Thank you for contributing

Several issues, this needs to be rewritten based on this, and use more appropriate code IMO

You should probably use the encode_ methods, like encode_real, taking into account details

This is exactly what to_raw_bytes() have to avoid. See the proposal here in details: godotengine/godot-proposals#7478

The problem with bit-wise conversion has no other solution than some trickery and workarounds in GDScript though, if any.

This is non serializable byte stream for the specific use with compute shaders params udpates.

@AThousandShips
Copy link
Member

Making this shader specific is not right, it should be general purpose if it is in @GlobalScope

pkpro and others added 5 commits August 11, 2023 12:55
Co-authored-by: A Thousand Ships <96648715+AThousandShips@users.noreply.github.com>
Co-authored-by: A Thousand Ships <96648715+AThousandShips@users.noreply.github.com>
@AThousandShips AThousandShips self-requested a review August 11, 2023 11:25
@AThousandShips AThousandShips dismissed their stale review August 11, 2023 11:25

Main issues resolved

@AThousandShips AThousandShips removed their request for review August 11, 2023 11:25
…t and float types conversion into 32 bit equivalents
@pkpro
Copy link
Contributor Author

pkpro commented Aug 11, 2023

You didn't mention the encode methods in PackedByteArray which is what I mean here, which allows a lot of flexibility, is easy, and not error prone IMO

I might be missing something here, care to elaborate?

Your suggested approach doesn't allow any flexibility on precision or details like that, which is a drawback, it also doesn't allow you to pass PackedInt*Array, PackedColorArray, or PackedFloat*Array, or even Color

You are correct in this regard. I didn't worked on this part yet as on the Dictionary conversion as well. I might extend it later though. However I'm not sure this proposal and the functionality will benefit someone beside me yet. So I acted early and we can decide if we go one with it or drop it completely.

Also I don't know if the example you provided is realistic, you will never send arbitrary data to a compute shader, you have to match the data format, so you can always use individual methods to do it, using the methods on the PackedByteArray, you should also know the size beforehand, so resizing the data to fit isn't really an issue either, IMO

There are a lot of scenarios to send arbitrary data to compute shaders, starting with any kind of procedural environment and ending with SFX, like rain drops or even SDF rendering.

you should also know the size beforehand, so resizing the data to fit isn't really an issue either, IMO

Yes that is the game developer task and you can't address this with the code in other way but some simplification for the cost of precision for example.

@pkpro
Copy link
Contributor Author

pkpro commented Aug 11, 2023

Making this shader specific is not right, it should be general purpose if it is in @GlobalScope

But there are already general purpose functions for serialization. The feature which is missing is specifically the workflow with GLSL shaders.

@AThousandShips
Copy link
Member

Let's take this to the proposal, this is for implementation specific things, and I commented before the proposal was made

@pkpro
Copy link
Contributor Author

pkpro commented Aug 11, 2023

Let's take this to the proposal, this is for implementation specific things, and I commented before the proposal was made

Here is the proposal, let's wait on proposal decision: godotengine/godot-proposals#7478

@dalexeev
Copy link
Member

I didn't read the discussion, sorry if the questions are repeated.

  1. Why is there no inverse function? Note that it will require a Variant type parameter to decode.
  2. For consistency with other methods, the names should be a_to_b and b_to_a.
  3. If these methods are not going to be used often, I think it makes sense to put them in the Marshalls class instead of @GlobalScope.

Existing methods:

Return type Signature
PackedByteArray base64_to_raw(base64_str: String)
String base64_to_utf8(base64_str: String)
Variant base64_to_variant(base64_str: String, allow_objects: bool = false)
String raw_to_base64(array: PackedByteArray)
String utf8_to_base64(utf8_str: String)
String variant_to_base64(variant: Variant, full_objects: bool = false)

Logical continuation:

Return type Signature
PackedByteArray variant_to_raw(variant: Variant, full_objects: bool = false)
Variant raw_to_variant(array: PackedByteArray, allow_objects: bool = false)

What could be the methods:

Return type Signature
PackedByteArray variant_to_rawdata(variant: Variant, full_objects: bool = false)
Variant rawdata_to_variant(array: PackedByteArray, type: Variant.Type, allow_objects: bool = false)

Note that binary serialization may encode additional data (see flags). For example, we can add information about typed arrays, see #78219.

@pkpro
Copy link
Contributor Author

pkpro commented Aug 11, 2023

I didn't read the discussion, sorry if the questions are repeated.

1. Why is there no inverse function? Note that it will require a `Variant` type parameter to decode.

That is the point it is not a serialization method, you can't deserealize from the result. No any metadata available in the raw byte stream.

2. For consistency with other methods, the names should be `a_to_b` and `b_to_a`.

It is impossible IMHO. Prove me wrong please.

3. If these methods are not going to be used often, I think it makes sense to put them in the `Marshalls` class instead of `@GlobalScope`.

Existing methods:
Return type Signature
PackedByteArray base64_to_raw(base64_str: String)
String base64_to_utf8(base64_str: String)
Variant base64_to_variant(base64_str: String, allow_objects: bool = false)
String raw_to_base64(array: PackedByteArray)
String utf8_to_base64(utf8_str: String)
String variant_to_base64(variant: Variant, full_objects: bool = false)

Logical continuation:
Return type Signature
PackedByteArray variant_to_raw(variant: Variant, full_objects: bool = false)
Variant raw_to_variant(array: PackedByteArray, allow_objects: bool = false)

What could be the methods:
Return type Signature
PackedByteArray variant_to_rawdata(variant: Variant, full_objects: bool = false)
Variant rawdata_to_variant(array: PackedByteArray, type: Variant.Type, allow_objects: bool = false)

Note that binary serialization may encode additional data (see flags). For example, we can add information about typed arrays, see #78219.

Serialization is not the scope of this function. The purpose of this function is one way conversion of GDScript Variant (limited subset of it) into uniforms of GLSL compute shaders keeping the bit-wise and semantic conversion at bay.

@dalexeev
Copy link
Member

Serialization is not the scope of this function. The purpose of this function is one way conversion of GDScript Variant (limited subset of it) into uniforms of GLSL compute shaders keeping the bit-wise and semantic conversion at bay.

If this is such a specific task, then it should be a RenderingDevice method or some RD* class, not a global function.

@pkpro
Copy link
Contributor Author

pkpro commented Aug 11, 2023

Serialization is not the scope of this function. The purpose of this function is one way conversion of GDScript Variant (limited subset of it) into uniforms of GLSL compute shaders keeping the bit-wise and semantic conversion at bay.

If this is such a specific task, then it should be a RenderingDevice method or some RD* class, not a global function.

Probably. I can't decide it, I'm new to Ggodot engine. It has been mentioned in proposal. From my point of view, it will be implemented there were it should be and you will tell me where, though there is no decision on this enhancement yet.

@pkpro pkpro closed this Aug 16, 2023
@pkpro
Copy link
Contributor Author

pkpro commented Aug 16, 2023

It is definitely a wrong place for such function. It should not be in the RenderingDevice either. Closing this PR. I'll create new PR when I find a better place for this functionality if ever.

@dalexeev dalexeev removed this from the 4.x milestone Aug 16, 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.

Convert an array into byte stream for SSBO and push_constant uniform fill out.
3 participants