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.

Sign up for GitHub

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 create_instance in android GodotApp so non-editor apps can restart #101050

Merged
merged 1 commit into from
Jan 22, 2025
+79 −31
Merged

Fix create_instance in android GodotApp so non-editor apps can restart #101050

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 8 additions & 26 deletions platform/android/java/editor/src/main/java/org/godotengine/editor/BaseGodotEditor.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,6 @@ abstract class BaseGodotEditor : GodotActivity() {

private const val WAIT_FOR_DEBUGGER = false

@JvmStatic
protected val EXTRA_COMMAND_LINE_PARAMS = "command_line_params"
@JvmStatic
protected val EXTRA_PIP_AVAILABLE = "pip_available"
@JvmStatic
Expand Down Expand Up @@ -130,7 +128,6 @@ abstract class BaseGodotEditor : GodotActivity() {
}

private val editorMessageDispatcher = EditorMessageDispatcher(this)
private val commandLineParams = ArrayList<String>()
private val editorLoadingIndicator: View? by lazy { findViewById(R.id.editor_loading_indicator) }

override fun getGodotAppLayout() = R.layout.godot_editor_layout
Expand Down Expand Up @@ -183,10 +180,6 @@ abstract class BaseGodotEditor : GodotActivity() {
// requested on demand based on use cases.
PermissionsUtil.requestManifestPermissions(this, getExcludedPermissions())

val params = intent.getStringArrayExtra(EXTRA_COMMAND_LINE_PARAMS)
Log.d(TAG, "Starting intent $intent with parameters ${params.contentToString()}")
updateCommandLineParams(params?.asList() ?: emptyList())

editorMessageDispatcher.parseStartIntent(packageManager, intent)

if (BuildConfig.BUILD_TYPE == "dev" && WAIT_FOR_DEBUGGER) {
Expand Down Expand Up @@ -219,20 +212,16 @@ abstract class BaseGodotEditor : GodotActivity() {
}

@CallSuper
protected open fun updateCommandLineParams(args: List<String>) {
// Update the list of command line params with the new args
commandLineParams.clear()
if (args.isNotEmpty()) {
commandLineParams.addAll(args)
}
if (BuildConfig.BUILD_TYPE == "dev") {
commandLineParams.add("--benchmark")
protected override fun updateCommandLineParams(args: Array<String>) {
val args = if (BuildConfig.BUILD_TYPE == "dev") {
Copy link
Contributor

Choose a reason for hiding this comment

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

The new variable should have a different name otherwise it shadows the function parameter, and may lead to subtle bugs / mistakes when this method is updated in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO that's a feature, not a bug.. if you have multiple variables around that's when you might choose the wrong variable
https://discuss.kotlinlang.org/t/rust-style-variable-shadowing/16338

if there's only one 'args' then you can't use the 'wrong' one. I want to 'destroy' the old 'args' value

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a fair argument.. I don't have a preference one way or another but since it seems to match the use-case here, it can remain as is. Note though that this would not work in the GodotXRGame use-case since we're updating the arguments several times based on separate constraints.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It makes sense to me to add a bunch of stuff to a list, then call toTypedArray() to pass it on in that case (which is how GodotXRGame.updateCommandLineParams() works in this change)

  • Really this method should be named setCommandLineParams() since it's only ever called once and always sets the entire array/list at once and each level only really calls it once to set it's arguments and then passes them to super().
  • IMO The whole dance here is a bit much. To simplify I would eliminate this method and just make it a protected List<String> and let each level add it's own args. The consumer could call public getCommandLine(): Array<String> to get the result after the end.
    • Paranoia: this may be too simple in some case that doesn't exist yet if arguments needed to be strictly ordered .. but it's a protected member so you'd just have to refactor it then, and currently it doesn't look like they do need to be strictly ordered.
  • If setCommandLineParams() threw when it's too late then that would make sense though.
  • Right now I don't want to do any more refactoring in this change though, already have too much whiplash :D

args + "--benchmark"
} else {
args
}
super.updateCommandLineParams(args);
}

final override fun getCommandLine() = commandLineParams

protected fun retrieveEditorWindowInfo(args: Array<String>): EditorWindowInfo {
protected open fun retrieveEditorWindowInfo(args: Array<String>): EditorWindowInfo {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this change can be reverted

var hasEditor = false
var xrMode = XR_MODE_DEFAULT

Expand Down Expand Up @@ -335,14 +324,7 @@ abstract class BaseGodotEditor : GodotActivity() {
val newInstance = getNewGodotInstanceIntent(editorWindowInfo, args)
if (editorWindowInfo.windowClassName == javaClass.name) {
Log.d(TAG, "Restarting ${editorWindowInfo.windowClassName} with parameters ${args.contentToString()}")
val godot = godot
if (godot != null) {
godot.destroyAndKillProcess {
ProcessPhoenix.triggerRebirth(this, activityOptions?.toBundle(), newInstance)
}
} else {
ProcessPhoenix.triggerRebirth(this, activityOptions?.toBundle(), newInstance)
}
triggerRebirth(activityOptions?.toBundle(), newInstance)
} else {
Log.d(TAG, "Starting ${editorWindowInfo.windowClassName} with parameters ${args.contentToString()}")
newInstance.putExtra(EXTRA_NEW_LAUNCH, true)
Expand Down
4 changes: 2 additions & 2 deletions platform/android/java/editor/src/main/java/org/godotengine/editor/GodotXRGame.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ open class GodotXRGame: GodotGame() {

override fun overrideOrientationRequest() = true

override fun updateCommandLineParams(args: List<String>) {
override fun updateCommandLineParams(args: Array<String>) {
val updatedArgs = ArrayList<String>()
if (!args.contains(XRMode.OPENXR.cmdLineArg)) {
updatedArgs.add(XRMode.OPENXR.cmdLineArg)
Expand All @@ -51,7 +51,7 @@ open class GodotXRGame: GodotGame() {
}
updatedArgs.addAll(args)

super.updateCommandLineParams(updatedArgs)
super.updateCommandLineParams(updatedArgs.toTypedArray())
}

override fun getEditorWindowInfo() = XR_RUN_GAME_INFO
Expand Down
23 changes: 20 additions & 3 deletions platform/android/java/lib/src/org/godotengine/godot/Godot.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -823,9 +823,26 @@ class Godot(private val context: Context) {
* Returns true if `Vulkan` is used for rendering.
*/
private fun usesVulkan(): Boolean {
val renderer = GodotLib.getGlobal("rendering/renderer/rendering_method")
val renderingDevice = GodotLib.getGlobal("rendering/rendering_device/driver")
return ("forward_plus" == renderer || "mobile" == renderer) && "vulkan" == renderingDevice
var rendererSource = "ProjectSettings"
var renderer = GodotLib.getGlobal("rendering/renderer/rendering_method")
var renderingDeviceSource = "ProjectSettings"
var renderingDevice = GodotLib.getGlobal("rendering/rendering_device/driver")
val cmdline = getCommandLine()
var index = cmdline.indexOf("--rendering-method")
if (index > -1 && cmdline.size > index + 1) {
rendererSource = "CommandLine"
renderer = cmdline.get(index + 1)
}
index = cmdline.indexOf("--rendering-driver")
if (index > -1 && cmdline.size > index + 1) {
renderingDeviceSource = "CommandLine"
renderingDevice = cmdline.get(index + 1)
}
jamie-pate marked this conversation as resolved.
Show resolved Hide resolved
val result = ("forward_plus" == renderer || "mobile" == renderer) && "vulkan" == renderingDevice
Log.d(TAG, """usesVulkan(): ${result}
renderingDevice: ${renderingDevice} (${renderingDeviceSource})
renderer: ${renderer} (${rendererSource})""")
return result
}

/**
Expand Down
49 changes: 49 additions & 0 deletions platform/android/java/lib/src/org/godotengine/godot/GodotActivity.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
package org.godotengine.godot

import android.app.Activity
import android.content.ComponentName
import android.content.Intent
import android.content.pm.PackageManager
import android.os.Bundle
Expand All @@ -52,18 +53,32 @@ abstract class GodotActivity : FragmentActivity(), GodotHost {
companion object {
private val TAG = GodotActivity::class.java.simpleName

@JvmStatic
protected val EXTRA_COMMAND_LINE_PARAMS = "command_line_params"

@JvmStatic
protected val EXTRA_NEW_LAUNCH = "new_launch_requested"

// This window must not match those in BaseGodotEditor.RUN_GAME_INFO etc
@JvmStatic
private final val DEFAULT_WINDOW_ID = 664;
}

private val commandLineParams = ArrayList<String>()
/**
* Interaction with the [Godot] object is delegated to the [GodotFragment] class.
*/
protected var godotFragment: GodotFragment? = null
private set

@CallSuper
override fun onCreate(savedInstanceState: Bundle?) {
val params = intent.getStringArrayExtra(EXTRA_COMMAND_LINE_PARAMS)
Log.d(TAG, "Starting intent $intent with parameters ${params.contentToString()}")
updateCommandLineParams(params ?: emptyArray())

super.onCreate(savedInstanceState)

setContentView(getGodotAppLayout())

handleStartIntent(intent, true)
Expand All @@ -79,6 +94,29 @@ abstract class GodotActivity : FragmentActivity(), GodotHost {
}
}

override fun onNewGodotInstanceRequested(args: Array<String>): Int {
Log.d(TAG, "Restarting with parameters ${args.contentToString()}")
val intent = Intent()
.setComponent(ComponentName(this, javaClass.name))
.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK)
.putExtra(EXTRA_COMMAND_LINE_PARAMS, args)
triggerRebirth(null, intent)
// fake 'process' id returned by create_instance() etc
return DEFAULT_WINDOW_ID;
}

protected fun triggerRebirth(bundle: Bundle?, intent: Intent) {
// Launch a new activity
val godot = godot
if (godot != null) {
godot.destroyAndKillProcess {
ProcessPhoenix.triggerRebirth(this, bundle, intent)
}
} else {
ProcessPhoenix.triggerRebirth(this, bundle, intent)
}
}

@LayoutRes
protected open fun getGodotAppLayout() = R.layout.godot_app_layout

Expand Down Expand Up @@ -176,4 +214,15 @@ abstract class GodotActivity : FragmentActivity(), GodotHost {
protected open fun initGodotInstance(): GodotFragment {
return GodotFragment()
}

@CallSuper
protected open fun updateCommandLineParams(args: Array<String>) {
// Update the list of command line params with the new args
commandLineParams.clear()
if (args.isNotEmpty()) {
commandLineParams.addAll(args)
}
}

final override fun getCommandLine() = commandLineParams
}