Skip to content

Commit

Permalink
Fix the cleanup logic for the Android render thread
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
m4gr3d committed Jul 23, 2024
1 parent 18c1c25 commit 3f8dc42
Show file tree
Hide file tree
Showing 11 changed files with 76 additions and 33 deletions.
11 changes: 2 additions & 9 deletions platform/android/java/lib/src/org/godotengine/godot/Godot.kt
Original file line number Diff line number Diff line change
Expand Up @@ -577,10 +577,7 @@ class Godot(private val context: Context) : SensorEventListener {
plugin.onMainDestroy()
}

runOnRenderThread {
GodotLib.ondestroy()
forceQuit()
}
renderView?.onActivityDestroyed()
}

/**
Expand Down Expand Up @@ -814,11 +811,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()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,12 @@ public void onCreate(Bundle icicle) {
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");
}
Expand Down Expand Up @@ -325,7 +330,7 @@ public void onResume() {
}

public void onBackPressed() {
godot.onBackPressed(this);
godot.onBackPressed();
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@
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;
Expand Down Expand Up @@ -77,7 +76,7 @@
* 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;
Expand Down Expand Up @@ -140,9 +139,14 @@ public void onActivityStarted() {
resumeGLThread();
}

@Override
public void onActivityDestroyed() {
requestRenderThreadExitAndWait();
}

@Override
public void onBackPressed() {
godot.onBackPressed(host);
godot.onBackPressed();
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -54,6 +57,8 @@ public interface GodotRenderView {

void onActivityStarted();

void onActivityDestroyed();

void onBackPressed();

GodotInputHandler getInputHandler();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@

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;
Expand Down Expand Up @@ -118,9 +118,14 @@ public void onActivityResumed() {
});
}

@Override
public void onActivityDestroyed() {
requestRenderThreadExitAndWait();
}

@Override
public void onBackPressed() {
godot.onBackPressed(host);
godot.onBackPressed();
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -595,6 +595,15 @@ protected final void pauseGLThread() {
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 --

/**
Expand Down Expand Up @@ -783,6 +792,11 @@ public interface Renderer {
* @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 --
}

Expand Down Expand Up @@ -1621,6 +1635,12 @@ private void guardedRun() throws InterruptedException {
* clean-up everything...
*/
synchronized (sGLThreadManager) {
Log.d("GLThread", "Exiting render thread");
GLSurfaceView view = mGLSurfaceViewWeakRef.get();
if (view != null) {
view.mRenderer.onRenderThreadExiting();
}

stopEglSurfaceLocked();
stopEglContextLocked();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,17 @@
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;

/**
* 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;

Expand All @@ -62,6 +66,12 @@ public boolean onDrawFrame(GL10 gl) {
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()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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

/**
Expand All @@ -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()

/**
Expand Down Expand Up @@ -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()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
Expand All @@ -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()
Expand Down Expand Up @@ -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
}

Expand Down

0 comments on commit 3f8dc42

Please sign in to comment.