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

Fix JavaScriptBridge.eval() never returning PackedByteArray #81015

Merged

Conversation

OverloadedOrama
Copy link
Contributor

@OverloadedOrama OverloadedOrama commented Aug 26, 2023

JavaScriptBridge.eval() currently returns 20 on array buffers, which used to be the enumerator value of Godot 3.x's TYPE_POOL_BYTE_ARRAY (and now is the value of TYPE_COLOR in 4.x), while it should return 29 which is the enumerator value of TYPE_PACKED_BYTE_ARRAY.

The rest of the enumerator values of the file seem OK, but feel free to let me know if you see anything else that needs to be updated.

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.

LGTM, can't test for web but the update of type should be correct

@OverloadedOrama OverloadedOrama changed the title Fix JavaScriptBridge.eval() never returning PackedByteArray Fix JavaScriptBridge.eval() never returning PackedByteArray Aug 26, 2023
Copy link
Member

@adamscott adamscott left a comment

Choose a reason for hiding this comment

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

Seems fine to me. Without this PR, it returns a Variant::Color type that will fail to map here:

Variant JavaScriptBridge::eval(const String &p_code, bool p_use_global_exec_context) {
union js_eval_ret js_data;
PackedByteArray arr;
VectorWriteProxy<uint8_t> arr_write;
Variant::Type return_type = static_cast<Variant::Type>(godot_js_eval(p_code.utf8().get_data(), p_use_global_exec_context, &js_data, &arr, &arr_write, resize_PackedByteArray_and_open_write));
switch (return_type) {
case Variant::BOOL:
return js_data.b;
case Variant::FLOAT:
return js_data.d;
case Variant::STRING: {
String str = String::utf8(js_data.s);
free(js_data.s); // Must free the string allocated in JS.
return str;
}
case Variant::PACKED_BYTE_ARRAY:
arr_write = VectorWriteProxy<uint8_t>();
return arr;
default:
return Variant();
}
}

@adamscott adamscott requested a review from Faless August 27, 2023 13:40
@adamscott
Copy link
Member

@OverloadedOrama Can you remove this part from your commit message?

The rest of the enumerator values of the file seem OK, but feel free to let me know if you see anything else that needs to be updated.

@OverloadedOrama
Copy link
Contributor Author

Yup, should be removed now.

@AThousandShips
Copy link
Member

AThousandShips commented Aug 27, 2023

I'd remove the reference to the docs as well as that link isn't permanent, alternatively rename it to 4.1 instead of stable

@OverloadedOrama
Copy link
Contributor Author

I renamed it to 4.1.

@akien-mga akien-mga added cherrypick:4.0 cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release labels Aug 28, 2023
It wrongly returned 20 on array buffers, which used to be the enumerator
value of Godot 3.x's type PoolByteArray, and now is the value of type Color,
while it should return 29 which is the enumerator value for PackedByteArray.
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

I reviewed the file and confirmed that the other hardcoded type values are correct. There were a couple outdated comments (REAL -> FLOAT) so I pushed an update to change those too.

Also wrapped the commit message to <= 78 chars and simplified a bit, it was quite verbose for such a small change :)

@akien-mga akien-mga merged commit cfac3e2 into godotengine:master Aug 28, 2023
13 of 14 checks passed
@akien-mga
Copy link
Member

Thanks!

@OverloadedOrama OverloadedOrama deleted the byte-array-drowned-in-a-pool branch August 31, 2023 08:44
@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Sep 21, 2023
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.2.

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.

None yet

5 participants