Skip to content

Commit

Permalink
Add stack management for icall arguments. (#364)
Browse files Browse the repository at this point in the history
* Add stack management for icall arguments.
* Fallback to new array when stack full.
* Use Godot max argument constant instead of custom one.
  • Loading branch information
CedNaru authored Oct 27, 2022
1 parent b00c797 commit 0e80dc1
Show file tree
Hide file tree
Showing 6 changed files with 87 additions and 18 deletions.
4 changes: 2 additions & 2 deletions docs/src/doc/user-guide/versioning.md
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
The module uses semantic versioning for its own versions but adds a suffix for the supported godot version:

Full version: `0.5.0-3.5.1`
Full version: `0.5.1-3.5.1`

Module Version: `0.5.0`
Module Version: `0.5.1`

Supported Godot Version: `3.5.1`
26 changes: 26 additions & 0 deletions harness/tests/src/main/kotlin/godot/tests/Invocation.kt
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,28 @@ class Invocation : Spatial() {
@RegisterSignal
val signalTwoParam by signal<String, Invocation>("str", "inv")

@RegisterSignal
val signalWithMultipleTargets by signal<Vector2>("vector2")

//To store values emitted by signals
@RegisterProperty
var array: VariantArray<Vector2> = VariantArray()

@RegisterFunction
fun targetFunctionOne(vector2: Vector2) {
array.append(vector2)
//call GodotAPI to insert different parameters in the stack.
this.setMeta("Random", "Value")
val size = array.size
if(size < 8)
//Call signal inside another signal
signalWithMultipleTargets.emit(Vector2(1, size))
}
@RegisterFunction
fun targetFunctionTwo(vector2: Vector2) {
array.append(vector2)
}

@RegisterFunction
fun intValue(value: Int) = value

Expand Down Expand Up @@ -347,13 +369,17 @@ class Invocation : Spatial() {
signalOneParam.connect(invocation, invocation::hookOneParam)
signalTwoParam.connect(invocation, invocation::hookTwoParam)

signalWithMultipleTargets.connect(this, ::targetFunctionOne)
signalWithMultipleTargets.connect(this, ::targetFunctionTwo)

(getNodeOrNull(path) as Button?)?.signalPressed?.connect(
invocation,
invocation::hookNoParam
)
signalNoParam.emit()
signalOneParam.emit(false)
signalTwoParam.emit("My Awesome param !", this)
signalWithMultipleTargets.emit(Vector2(0,0))

println("NavMesh instance id before re-assign: ${resourceTest.getInstanceId()}")
resourceTest = NavigationMesh()
Expand Down
23 changes: 23 additions & 0 deletions harness/tests/test/unit/test_signals.gd
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,26 @@ func test_signal_connection_code():
yield(get_tree().create_timer(1), "timeout")
assert_eq(invocation_script.get_node("CanvasLayer/Button").is_connected("pressed", invocation_script.invocation, "hook_no_param"), true, "signal \"pressed\" of button should be connected to \"invocation_script.invocation::hook_no_param\"")
invocation_script.free()

func test_signal_emitted_witt_multiple_targets():
var invocation_script = godot_tests_Invocation.new()
get_tree().root.add_child(invocation_script)
yield(get_tree().create_timer(1), "timeout")
assert_eq(invocation_script.array.size(), 16)
assert_eq(invocation_script.array[0], Vector2(0,0))
assert_eq(invocation_script.array[1], Vector2(1,1))
assert_eq(invocation_script.array[2], Vector2(1,2))
assert_eq(invocation_script.array[3], Vector2(1,3))
assert_eq(invocation_script.array[4], Vector2(1,4))
assert_eq(invocation_script.array[5], Vector2(1,5))
assert_eq(invocation_script.array[6], Vector2(1,6))
assert_eq(invocation_script.array[7], Vector2(1,7))
assert_eq(invocation_script.array[8], Vector2(1,7))
assert_eq(invocation_script.array[9], Vector2(1,6))
assert_eq(invocation_script.array[10], Vector2(1,5))
assert_eq(invocation_script.array[11], Vector2(1,4))
assert_eq(invocation_script.array[12], Vector2(1,3))
assert_eq(invocation_script.array[13], Vector2(1,2))
assert_eq(invocation_script.array[14], Vector2(1,1))
assert_eq(invocation_script.array[15], Vector2(0,0))
invocation_script.free()
2 changes: 1 addition & 1 deletion kt/buildSrc/src/main/kotlin/godotKotlinJvmVersion.kt
Original file line number Diff line number Diff line change
@@ -1 +1 @@
const val godotKotlinJvmVersion = "0.5.0"
const val godotKotlinJvmVersion = "0.5.1"
48 changes: 35 additions & 13 deletions src/transfer_context.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,11 @@

JNI_INIT_STATICS_FOR_CLASS(TransferContext)

thread_local static Variant variant_args[MAX_ARGS_SIZE]; // NOLINT(cert-err58-cpp)
thread_local static const Variant* variant_args_ptr[MAX_ARGS_SIZE];
thread_local static bool icall_args_init = false;
const int MAX_STACK_SIZE = VARIANT_ARG_MAX * 8;

thread_local static Variant variant_args[MAX_STACK_SIZE]; // NOLINT(cert-err58-cpp)
thread_local static const Variant* variant_args_ptr[MAX_STACK_SIZE];
thread_local static int stack_offset = -1;

TransferContext::TransferContext(jni::JObject p_wrapped, jni::JObject p_class_loader)
: JavaInstanceWrapper("godot.core.TransferContext", p_wrapped, p_class_loader) {
Expand Down Expand Up @@ -110,11 +112,11 @@ void TransferContext::icall(
jlong j_ptr,
jint p_method_index,
jint expectedReturnType) {
if (unlikely(!icall_args_init)) {
for (int i = 0; i < MAX_ARGS_SIZE; i++) {
if (unlikely(stack_offset == -1)) {
for (int i = 0; i < MAX_STACK_SIZE; i++) {
variant_args_ptr[i] = &variant_args[i];
}
icall_args_init = true;
stack_offset = 0;
}

TransferContext* transfer_context{GDKotlin::get_instance().transfer_context};
Expand All @@ -124,12 +126,10 @@ void TransferContext::icall(
uint32_t args_size{read_args_size(env, buffer)};

#ifdef DEBUG_ENABLED
JVM_CRASH_COND_MSG(args_size > MAX_ARGS_SIZE,
vformat("Cannot have more than %s arguments for method call.", MAX_ARGS_SIZE));
JVM_CRASH_COND_MSG(args_size > VARIANT_ARG_MAX,
vformat("Cannot have more than %s arguments for method call.", VARIANT_ARG_MAX));
#endif

read_args_to_array(buffer, variant_args, args_size);

auto* ptr{reinterpret_cast<Object*>(static_cast<uintptr_t>(j_ptr))};

int method_index{static_cast<int>(p_method_index)};
Expand All @@ -140,14 +140,36 @@ void TransferContext::icall(
#endif

Variant::CallError r_error{Variant::CallError::CALL_OK};
const Variant& ret_value{methodBind->call(ptr, variant_args_ptr, args_size, r_error)};

if(unlikely(stack_offset + args_size > MAX_STACK_SIZE)){
Variant args[VARIANT_ARG_MAX];
read_args_to_array(buffer, args, args_size);

const Variant* args_ptr[VARIANT_ARG_MAX];
for (int i = 0; i < args_size; i++) {
args_ptr[i] = &args[i];
}

const Variant& ret_value{methodBind->call(ptr, args_ptr, args_size, r_error)};
write_return_value(buffer, ret_value);
} else {
Variant* args{variant_args + stack_offset};
read_args_to_array(buffer, args, args_size);

const Variant** args_ptr{variant_args_ptr + stack_offset};

stack_offset += args_size;
const Variant& ret_value{methodBind->call(ptr, args_ptr, args_size, r_error)};
stack_offset -= args_size;

write_return_value(buffer, ret_value);
}


#ifdef DEBUG_ENABLED
JVM_CRASH_COND_MSG(r_error.error != Variant::CallError::CALL_OK,
vformat("Call to method with id %s failed.", method_index));
#endif

write_return_value(buffer, ret_value);
}

void TransferContext::invoke_constructor(JNIEnv* p_raw_env, jobject p_instance, jint p_class_index) {
Expand Down
2 changes: 0 additions & 2 deletions src/transfer_context.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@
#include "java_instance_wrapper.h"
#include "shared_buffer.h"

#define MAX_ARGS_SIZE 16

class TransferContext : public JavaInstanceWrapper<TransferContext> {
public:

Expand Down

0 comments on commit 0e80dc1

Please sign in to comment.