From 4d0da7401412edaba475de33d42a73ba8ca82cd3 Mon Sep 17 00:00:00 2001 From: Fredia Huya-Kouadio Date: Mon, 22 Jul 2024 17:51:45 -0700 Subject: [PATCH] Fix the cleanup logic for the Android render thread On Android the exit logic goes through `Godot#onDestroy()` who attempts to cleanup the engine using the following code: ``` runOnRenderThread { GodotLib.ondestroy() forceQuit() } ``` The issue however is that by the time we ran this code, the render thread has already been paused (but not yet destroyed), and thus `GodotLib.ondestroy()` and `forceQuit()` which are scheduled on the render thread are not executed. To address this, we instead explicitly request the render thread to exit and block until it does. As part of it exit logic, the render thread has been updated to properly destroy and clean the native instance of the Godot engine, resolving the issue. --- .../org/godotengine/editor/GodotEditor.kt | 9 +++- .../lib/src/org/godotengine/godot/Godot.kt | 50 +++++++++++++++---- .../org/godotengine/godot/GodotActivity.kt | 6 +-- .../org/godotengine/godot/GodotFragment.java | 11 ++-- .../godotengine/godot/GodotGLRenderView.java | 10 ++-- .../godotengine/godot/GodotRenderView.java | 5 ++ .../godot/GodotVulkanRenderView.java | 9 +++- .../godotengine/godot/gl/GLSurfaceView.java | 20 ++++++++ .../godotengine/godot/gl/GodotRenderer.java | 10 ++++ .../godotengine/godot/vulkan/VkRenderer.kt | 15 ++++-- .../godotengine/godot/vulkan/VkSurfaceView.kt | 8 ++- .../org/godotengine/godot/vulkan/VkThread.kt | 6 ++- platform/android/java_godot_lib_jni.cpp | 1 + platform/android/java_godot_wrapper.cpp | 11 ++++ platform/android/java_godot_wrapper.h | 2 + 15 files changed, 136 insertions(+), 37 deletions(-) diff --git a/platform/android/java/editor/src/main/java/org/godotengine/editor/GodotEditor.kt b/platform/android/java/editor/src/main/java/org/godotengine/editor/GodotEditor.kt index 5515347bd61..7c11d696090 100644 --- a/platform/android/java/editor/src/main/java/org/godotengine/editor/GodotEditor.kt +++ b/platform/android/java/editor/src/main/java/org/godotengine/editor/GodotEditor.kt @@ -203,7 +203,14 @@ open class GodotEditor : GodotActivity() { } if (editorWindowInfo.windowClassName == javaClass.name) { Log.d(TAG, "Restarting ${editorWindowInfo.windowClassName} with parameters ${args.contentToString()}") - ProcessPhoenix.triggerRebirth(this, newInstance) + val godot = godot + if (godot != null) { + godot.destroyAndKillProcess { + ProcessPhoenix.triggerRebirth(this, newInstance) + } + } else { + ProcessPhoenix.triggerRebirth(this, newInstance) + } } else { Log.d(TAG, "Starting ${editorWindowInfo.windowClassName} with parameters ${args.contentToString()}") newInstance.putExtra(EXTRA_NEW_LAUNCH, true) diff --git a/platform/android/java/lib/src/org/godotengine/godot/Godot.kt b/platform/android/java/lib/src/org/godotengine/godot/Godot.kt index 7e2a44ab390..fa39ccb546f 100644 --- a/platform/android/java/lib/src/org/godotengine/godot/Godot.kt +++ b/platform/android/java/lib/src/org/godotengine/godot/Godot.kt @@ -73,6 +73,7 @@ import java.io.InputStream import java.lang.Exception import java.security.MessageDigest import java.util.* +import java.util.concurrent.atomic.AtomicReference /** * Core component used to interface with the native layer of the engine. @@ -127,6 +128,11 @@ class Godot(private val context: Context) : SensorEventListener { val netUtils = GodotNetUtils(context) private val commandLineFileParser = CommandLineFileParser() + /** + * Task to run when the engine terminates. + */ + private val runOnTerminate = AtomicReference() + /** * Tracks whether [onCreate] was completed successfully. */ @@ -577,10 +583,7 @@ class Godot(private val context: Context) : SensorEventListener { plugin.onMainDestroy() } - runOnRenderThread { - GodotLib.ondestroy() - forceQuit() - } + renderView?.onActivityDestroyed() } /** @@ -663,6 +666,15 @@ class Godot(private val context: Context) : SensorEventListener { primaryHost?.onGodotMainLoopStarted() } + /** + * Invoked on the render thread when the engine is about to terminate. + */ + @Keep + private fun onGodotTerminating() { + Log.v(TAG, "OnGodotTerminating") + runOnTerminate.get()?.run() + } + private fun restart() { primaryHost?.onGodotRestartRequested(this) } @@ -798,8 +810,28 @@ class Godot(private val context: Context) : SensorEventListener { mClipboard.setPrimaryClip(clip) } - fun forceQuit() { - forceQuit(0) + /** + * Destroys the Godot Engine and kill the process it's running in. + */ + @JvmOverloads + fun destroyAndKillProcess(destroyRunnable: Runnable? = null) { + val host = primaryHost + val activity = host?.activity + if (host == null || activity == null) { + // Run the destroyRunnable right away as we are about to force quit. + destroyRunnable?.run() + + // Fallback to force quit + forceQuit(0) + return + } + + // Store the destroyRunnable so it can be run when the engine is terminating + runOnTerminate.set(destroyRunnable) + + runOnUiThread { + onDestroy(host) + } } @Keep @@ -814,11 +846,7 @@ class Godot(private val context: Context) : SensorEventListener { } ?: return false } - fun onBackPressed(host: GodotHost) { - if (host != primaryHost) { - return - } - + fun onBackPressed() { var shouldQuit = true for (plugin in pluginRegistry.allPlugins) { if (plugin.onMainBackPressed()) { diff --git a/platform/android/java/lib/src/org/godotengine/godot/GodotActivity.kt b/platform/android/java/lib/src/org/godotengine/godot/GodotActivity.kt index 4c5e857b7ac..913e3d04c5d 100644 --- a/platform/android/java/lib/src/org/godotengine/godot/GodotActivity.kt +++ b/platform/android/java/lib/src/org/godotengine/godot/GodotActivity.kt @@ -85,12 +85,8 @@ abstract class GodotActivity : FragmentActivity(), GodotHost { protected open fun getGodotAppLayout() = R.layout.godot_app_layout override fun onDestroy() { - Log.v(TAG, "Destroying Godot app...") + Log.v(TAG, "Destroying GodotActivity $this...") super.onDestroy() - - godotFragment?.let { - terminateGodotInstance(it.godot) - } } override fun onGodotForceQuit(instance: Godot) { diff --git a/platform/android/java/lib/src/org/godotengine/godot/GodotFragment.java b/platform/android/java/lib/src/org/godotengine/godot/GodotFragment.java index 1612ddd0b3b..fdda7665947 100644 --- a/platform/android/java/lib/src/org/godotengine/godot/GodotFragment.java +++ b/platform/android/java/lib/src/org/godotengine/godot/GodotFragment.java @@ -187,7 +187,12 @@ public class GodotFragment extends Fragment implements IDownloaderClient, GodotH final Activity activity = getActivity(); mCurrentIntent = activity.getIntent(); - godot = new Godot(requireContext()); + if (parentHost != null) { + godot = parentHost.getGodot(); + } + if (godot == null) { + godot = new Godot(requireContext()); + } performEngineInitialization(); BenchmarkUtils.endBenchmarkMeasure("Startup", "GodotFragment::onCreate"); } @@ -209,7 +214,7 @@ public class GodotFragment extends Fragment implements IDownloaderClient, GodotH final String errorMessage = TextUtils.isEmpty(e.getMessage()) ? getString(R.string.error_engine_setup_message) : e.getMessage(); - godot.alert(errorMessage, getString(R.string.text_error_title), godot::forceQuit); + godot.alert(errorMessage, getString(R.string.text_error_title), godot::destroyAndKillProcess); } catch (IllegalArgumentException ignored) { final Activity activity = getActivity(); Intent notifierIntent = new Intent(activity, activity.getClass()); @@ -325,7 +330,7 @@ public class GodotFragment extends Fragment implements IDownloaderClient, GodotH } public void onBackPressed() { - godot.onBackPressed(this); + godot.onBackPressed(); } /** diff --git a/platform/android/java/lib/src/org/godotengine/godot/GodotGLRenderView.java b/platform/android/java/lib/src/org/godotengine/godot/GodotGLRenderView.java index 81043ce782b..7fbdb34047b 100644 --- a/platform/android/java/lib/src/org/godotengine/godot/GodotGLRenderView.java +++ b/platform/android/java/lib/src/org/godotengine/godot/GodotGLRenderView.java @@ -42,7 +42,6 @@ import org.godotengine.godot.xr.regular.RegularContextFactory; import org.godotengine.godot.xr.regular.RegularFallbackConfigChooser; import android.annotation.SuppressLint; -import android.content.Context; import android.content.res.AssetManager; import android.graphics.Bitmap; import android.graphics.BitmapFactory; @@ -77,7 +76,7 @@ import java.io.InputStream; * that matches it exactly (with regards to red/green/blue/alpha channels * bit depths). Failure to do so would result in an EGL_BAD_MATCH error. */ -public class GodotGLRenderView extends GLSurfaceView implements GodotRenderView { +class GodotGLRenderView extends GLSurfaceView implements GodotRenderView { private final GodotHost host; private final Godot godot; private final GodotInputHandler inputHandler; @@ -140,9 +139,14 @@ public class GodotGLRenderView extends GLSurfaceView implements GodotRenderView resumeGLThread(); } + @Override + public void onActivityDestroyed() { + requestRenderThreadExitAndWait(); + } + @Override public void onBackPressed() { - godot.onBackPressed(host); + godot.onBackPressed(); } @Override diff --git a/platform/android/java/lib/src/org/godotengine/godot/GodotRenderView.java b/platform/android/java/lib/src/org/godotengine/godot/GodotRenderView.java index 5b2f9f57c7a..19ec0fd1a4a 100644 --- a/platform/android/java/lib/src/org/godotengine/godot/GodotRenderView.java +++ b/platform/android/java/lib/src/org/godotengine/godot/GodotRenderView.java @@ -44,6 +44,9 @@ public interface GodotRenderView { */ void startRenderer(); + /** + * Queues a runnable to be run on the rendering thread. + */ void queueOnRenderThread(Runnable event); void onActivityPaused(); @@ -54,6 +57,8 @@ public interface GodotRenderView { void onActivityStarted(); + void onActivityDestroyed(); + void onBackPressed(); GodotInputHandler getInputHandler(); diff --git a/platform/android/java/lib/src/org/godotengine/godot/GodotVulkanRenderView.java b/platform/android/java/lib/src/org/godotengine/godot/GodotVulkanRenderView.java index a1ee9bd6b4d..f4411ddf2c3 100644 --- a/platform/android/java/lib/src/org/godotengine/godot/GodotVulkanRenderView.java +++ b/platform/android/java/lib/src/org/godotengine/godot/GodotVulkanRenderView.java @@ -50,7 +50,7 @@ import androidx.annotation.Keep; import java.io.InputStream; -public class GodotVulkanRenderView extends VkSurfaceView implements GodotRenderView { +class GodotVulkanRenderView extends VkSurfaceView implements GodotRenderView { private final GodotHost host; private final Godot godot; private final GodotInputHandler mInputHandler; @@ -118,9 +118,14 @@ public class GodotVulkanRenderView extends VkSurfaceView implements GodotRenderV }); } + @Override + public void onActivityDestroyed() { + requestRenderThreadExitAndWait(); + } + @Override public void onBackPressed() { - godot.onBackPressed(host); + godot.onBackPressed(); } @Override diff --git a/platform/android/java/lib/src/org/godotengine/godot/gl/GLSurfaceView.java b/platform/android/java/lib/src/org/godotengine/godot/gl/GLSurfaceView.java index c9421a32573..6a4e9da699e 100644 --- a/platform/android/java/lib/src/org/godotengine/godot/gl/GLSurfaceView.java +++ b/platform/android/java/lib/src/org/godotengine/godot/gl/GLSurfaceView.java @@ -595,6 +595,15 @@ public class GLSurfaceView extends SurfaceView implements SurfaceHolder.Callback protected final void resumeGLThread() { mGLThread.onResume(); } + + /** + * Requests the render thread to exit and block until it does. + */ + protected final void requestRenderThreadExitAndWait() { + if (mGLThread != null) { + mGLThread.requestExitAndWait(); + } + } // -- GODOT end -- /** @@ -783,6 +792,11 @@ public class GLSurfaceView extends SurfaceView implements SurfaceHolder.Callback * @return true if the buffers should be swapped, false otherwise. */ boolean onDrawFrame(GL10 gl); + + /** + * Invoked when the render thread is in the process of shutting down. + */ + void onRenderThreadExiting(); // -- GODOT end -- } @@ -1621,6 +1635,12 @@ public class GLSurfaceView extends SurfaceView implements SurfaceHolder.Callback * clean-up everything... */ synchronized (sGLThreadManager) { + Log.d("GLThread", "Exiting render thread"); + GLSurfaceView view = mGLSurfaceViewWeakRef.get(); + if (view != null) { + view.mRenderer.onRenderThreadExiting(); + } + stopEglSurfaceLocked(); stopEglContextLocked(); } diff --git a/platform/android/java/lib/src/org/godotengine/godot/gl/GodotRenderer.java b/platform/android/java/lib/src/org/godotengine/godot/gl/GodotRenderer.java index 9d44d8826cb..7e5e262b2de 100644 --- a/platform/android/java/lib/src/org/godotengine/godot/gl/GodotRenderer.java +++ b/platform/android/java/lib/src/org/godotengine/godot/gl/GodotRenderer.java @@ -34,6 +34,8 @@ import org.godotengine.godot.GodotLib; import org.godotengine.godot.plugin.GodotPlugin; import org.godotengine.godot.plugin.GodotPluginRegistry; +import android.util.Log; + import javax.microedition.khronos.egl.EGLConfig; import javax.microedition.khronos.opengles.GL10; @@ -41,6 +43,8 @@ import javax.microedition.khronos.opengles.GL10; * Godot's GL renderer implementation. */ public class GodotRenderer implements GLSurfaceView.Renderer { + private final String TAG = GodotRenderer.class.getSimpleName(); + private final GodotPluginRegistry pluginRegistry; private boolean activityJustResumed = false; @@ -62,6 +66,12 @@ public class GodotRenderer implements GLSurfaceView.Renderer { return swapBuffers; } + @Override + public void onRenderThreadExiting() { + Log.d(TAG, "Destroying Godot Engine"); + GodotLib.ondestroy(); + } + public void onSurfaceChanged(GL10 gl, int width, int height) { GodotLib.resize(null, width, height); for (GodotPlugin plugin : pluginRegistry.getAllPlugins()) { diff --git a/platform/android/java/lib/src/org/godotengine/godot/vulkan/VkRenderer.kt b/platform/android/java/lib/src/org/godotengine/godot/vulkan/VkRenderer.kt index 6f09f51d4c2..a93a7dbe097 100644 --- a/platform/android/java/lib/src/org/godotengine/godot/vulkan/VkRenderer.kt +++ b/platform/android/java/lib/src/org/godotengine/godot/vulkan/VkRenderer.kt @@ -31,11 +31,9 @@ @file:JvmName("VkRenderer") package org.godotengine.godot.vulkan +import android.util.Log import android.view.Surface - -import org.godotengine.godot.Godot import org.godotengine.godot.GodotLib -import org.godotengine.godot.plugin.GodotPlugin import org.godotengine.godot.plugin.GodotPluginRegistry /** @@ -52,6 +50,11 @@ import org.godotengine.godot.plugin.GodotPluginRegistry * @see [VkSurfaceView.startRenderer] */ internal class VkRenderer { + + companion object { + private val TAG = VkRenderer::class.java.simpleName + } + private val pluginRegistry: GodotPluginRegistry = GodotPluginRegistry.getPluginRegistry() /** @@ -101,8 +104,10 @@ internal class VkRenderer { } /** - * Called when the rendering thread is destroyed and used as signal to tear down the Vulkan logic. + * Invoked when the render thread is in the process of shutting down. */ - fun onVkDestroy() { + fun onRenderThreadExiting() { + Log.d(TAG, "Destroying Godot Engine") + GodotLib.ondestroy() } } diff --git a/platform/android/java/lib/src/org/godotengine/godot/vulkan/VkSurfaceView.kt b/platform/android/java/lib/src/org/godotengine/godot/vulkan/VkSurfaceView.kt index 791b4254443..9e30de6a153 100644 --- a/platform/android/java/lib/src/org/godotengine/godot/vulkan/VkSurfaceView.kt +++ b/platform/android/java/lib/src/org/godotengine/godot/vulkan/VkSurfaceView.kt @@ -113,12 +113,10 @@ open internal class VkSurfaceView(context: Context) : SurfaceView(context), Surf } /** - * Tear down the rendering thread. - * - * Must not be called before a [VkRenderer] has been set. + * Requests the render thread to exit and block until it does. */ - fun onDestroy() { - vkThread.blockingExit() + fun requestRenderThreadExitAndWait() { + vkThread.requestExitAndWait() } override fun surfaceChanged(holder: SurfaceHolder, format: Int, width: Int, height: Int) { diff --git a/platform/android/java/lib/src/org/godotengine/godot/vulkan/VkThread.kt b/platform/android/java/lib/src/org/godotengine/godot/vulkan/VkThread.kt index 8c0065b31ea..c7cb97d911e 100644 --- a/platform/android/java/lib/src/org/godotengine/godot/vulkan/VkThread.kt +++ b/platform/android/java/lib/src/org/godotengine/godot/vulkan/VkThread.kt @@ -75,6 +75,9 @@ internal class VkThread(private val vkSurfaceView: VkSurfaceView, private val vk private fun threadExiting() { lock.withLock { + Log.d(TAG, "Exiting render thread") + vkRenderer.onRenderThreadExiting() + exited = true lockCondition.signalAll() } @@ -93,7 +96,7 @@ internal class VkThread(private val vkSurfaceView: VkSurfaceView, private val vk /** * Request the thread to exit and block until it's done. */ - fun blockingExit() { + fun requestExitAndWait() { lock.withLock { shouldExit = true lockCondition.signalAll() @@ -171,7 +174,6 @@ internal class VkThread(private val vkSurfaceView: VkSurfaceView, private val vk while (true) { // Code path for exiting the thread loop. if (shouldExit) { - vkRenderer.onVkDestroy() return } diff --git a/platform/android/java_godot_lib_jni.cpp b/platform/android/java_godot_lib_jni.cpp index 11e897facf1..fec317ecb88 100644 --- a/platform/android/java_godot_lib_jni.cpp +++ b/platform/android/java_godot_lib_jni.cpp @@ -114,6 +114,7 @@ static void _terminate(JNIEnv *env, bool p_restart = false) { NetSocketAndroid::terminate(); if (godot_java) { + godot_java->on_godot_terminating(env); if (!restart_on_cleanup) { if (p_restart) { godot_java->restart(env); diff --git a/platform/android/java_godot_wrapper.cpp b/platform/android/java_godot_wrapper.cpp index 6e7f5ef5a17..70ea4b09c10 100644 --- a/platform/android/java_godot_wrapper.cpp +++ b/platform/android/java_godot_wrapper.cpp @@ -76,6 +76,7 @@ GodotJavaWrapper::GodotJavaWrapper(JNIEnv *p_env, jobject p_activity, jobject p_ _get_input_fallback_mapping = p_env->GetMethodID(godot_class, "getInputFallbackMapping", "()Ljava/lang/String;"); _on_godot_setup_completed = p_env->GetMethodID(godot_class, "onGodotSetupCompleted", "()V"); _on_godot_main_loop_started = p_env->GetMethodID(godot_class, "onGodotMainLoopStarted", "()V"); + _on_godot_terminating = p_env->GetMethodID(godot_class, "onGodotTerminating", "()V"); _create_new_godot_instance = p_env->GetMethodID(godot_class, "createNewGodotInstance", "([Ljava/lang/String;)I"); _get_render_view = p_env->GetMethodID(godot_class, "getRenderView", "()Lorg/godotengine/godot/GodotRenderView;"); _begin_benchmark_measure = p_env->GetMethodID(godot_class, "nativeBeginBenchmarkMeasure", "(Ljava/lang/String;Ljava/lang/String;)V"); @@ -136,6 +137,16 @@ void GodotJavaWrapper::on_godot_main_loop_started(JNIEnv *p_env) { } } +void GodotJavaWrapper::on_godot_terminating(JNIEnv *p_env) { + if (_on_godot_terminating) { + if (p_env == nullptr) { + p_env = get_jni_env(); + } + ERR_FAIL_NULL(p_env); + p_env->CallVoidMethod(godot_instance, _on_godot_terminating); + } +} + void GodotJavaWrapper::restart(JNIEnv *p_env) { if (_restart) { if (p_env == nullptr) { diff --git a/platform/android/java_godot_wrapper.h b/platform/android/java_godot_wrapper.h index e86391d4e3e..358cf3261d7 100644 --- a/platform/android/java_godot_wrapper.h +++ b/platform/android/java_godot_wrapper.h @@ -68,6 +68,7 @@ private: jmethodID _get_input_fallback_mapping = nullptr; jmethodID _on_godot_setup_completed = nullptr; jmethodID _on_godot_main_loop_started = nullptr; + jmethodID _on_godot_terminating = nullptr; jmethodID _create_new_godot_instance = nullptr; jmethodID _get_render_view = nullptr; jmethodID _begin_benchmark_measure = nullptr; @@ -85,6 +86,7 @@ public: void on_godot_setup_completed(JNIEnv *p_env = nullptr); void on_godot_main_loop_started(JNIEnv *p_env = nullptr); + void on_godot_terminating(JNIEnv *p_env = nullptr); void restart(JNIEnv *p_env = nullptr); bool force_quit(JNIEnv *p_env = nullptr, int p_instance_id = 0); void set_keep_screen_on(bool p_enabled);