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

Allow export plugins to override export option values #88291

Merged

Conversation

dsnopek
Copy link
Contributor

@dsnopek dsnopek commented Feb 13, 2024

This PR allows EditorExportPlugins to override export options, setting the given options to specific values. Overridden options will be hidden from the UI.

The goal is to allow making addons that customize export presets for specific target platforms/devices, which makes the UI easier to manage (since options that don't apply will be hidden), while not having to hardcode any of this platform-specific information into Godot itself.

Here's an example plugin in GDScript which simplifies the Android export preset when targeting the Meta Quest:

class MetaQuestTemplatePlugin extends EditorExportPlugin:
	func _get_name() -> String:
		return "MetaQuestTemplatePlugin"

	func _supports_platform(p_platform) -> bool:
		if p_platform is EditorExportPlatformAndroid:
			return true
		return false

	func _get_export_option_overrides(p_platform) -> Dictionary:
		if get_option("template/configure_for_meta_quest"):
			# Set override options for Meta Quest.
			return {
				'gradle_build/use_gradle_build': true,
				'gradle_build/export_format': 0,
				'gradle_build/min_sdk': '',
				'gradle_build/target_sdk': '32',
				'architectures/armeabi-v7a': false,
				'architectures/arm64-v8a': true,
				'architectures/x86': false,
				'architectures/x86_64': false,
				'xr_features/xr_mode': 1,
				'xr_features/enable_khronos_plugin': false,
				'xr_features/enable_lynx_plugin': false,
				'xr_features/enable_meta_plugin': true,
				'xr_features/enable_pico_plugin': false,
			}
		return {}

	func _get_export_options(p_platform) -> Array[Dictionary]:
		var ret: Array[Dictionary]
		ret.push_back({
			option = {
				type = TYPE_BOOL,
				name = "template/configure_for_meta_quest",
			},
			default_value = false,
			update_visibility = true,
		})
		return ret

In this example, a new "Configure for Meta Quest" export option is added. If this option is enabled, then a number of other export options will be overridden to specific values and hidden from the UI. This re-uses the existing update_visibility property, to allow the changes to take effect immediately in the UI. (The overrides are normally only updated when switching to the preset in the UI, or just before exporting, to allow the export preset UI to remain speedy.)

As an alternative to having an export option visible in the UI to enable this "template", I could also imagine an addon that provides a wizard to create the export preset with a hidden option set.

Even though this example is in GDScript, this would work from GDExtension as well.

@dsnopek dsnopek added this to the 4.x milestone Feb 13, 2024
@dsnopek dsnopek requested review from m4gr3d and a team February 13, 2024 17:28
@dsnopek dsnopek requested a review from a team as a code owner February 13, 2024 17:28
@Mickeon Mickeon self-requested a review February 13, 2024 18:15
@Mickeon
Copy link
Contributor

Mickeon commented Feb 13, 2024

Could use a similar example you provided in this PR in the documentation in a [codeblock] (albeit shorter of course).

Copy link
Contributor

@m4gr3d m4gr3d left a comment

Choose a reason for hiding this comment

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

Looks great! Simple, but gets the job done!

My only concern is with regard to the mechanism to enable the overrides; at the moment it's up to the plugin which means the plugin can provide an option as you demonstrate in your example, or it can automatically apply the overrides without allowing the user to control the feature.

I wonder if instead we should make it a core part of the export preset config similar to the runnable toggle. So we'd add a toggle to allow for options override, and check the value of the toggle before retrieving the options overrides from the plugin.
Thoughts?

editor/export/editor_export_plugin.h Outdated Show resolved Hide resolved
@m4gr3d m4gr3d modified the milestones: 4.x, 4.3 Feb 13, 2024
@m4gr3d
Copy link
Contributor

m4gr3d commented Feb 13, 2024

@dsnopek How do we resolve plugin overrides conflicts?

For example, the example Meta plugin uses:

return {
				'gradle_build/use_gradle_build': true,
				'gradle_build/export_format': 0,
				'gradle_build/min_sdk': '',
				'gradle_build/target_sdk': '32',
				'architectures/armeabi-v7a': false,
				'architectures/arm64-v8a': true,
				'architectures/x86': false,
				'architectures/x86_64': false,
				'xr_features/xr_mode': 1,
				'xr_features/enable_khronos_plugin': false,
				'xr_features/enable_lynx_plugin': false,
				'xr_features/enable_meta_plugin': true,
				'xr_features/enable_pico_plugin': false,
			}

While for the vendors plugin, we may want to use this capability as well but only return:

return {
				'gradle_build/use_gradle_build': true,
				'gradle_build/min_sdk': '',
				'gradle_build/target_sdk': '32',
				'architectures/arm64-v8a': true,
				'architectures/x86': false,
				'xr_features/xr_mode': 1,
			}

@m4gr3d
Copy link
Contributor

m4gr3d commented Feb 13, 2024

I wonder if instead we should make it a core part of the export preset config similar to the runnable toggle. So we'd add a toggle to allow for options override, and check the value of the toggle before retrieving the options overrides from the plugin.
Thoughts?

An alternative that aligns with your current proposal would be automatically add a template/configure_for_<plugin_name> option for any editor export plugin that declares that it provides options overrides (declared via a bool method to be overridden).
The overrides would only be applied if that that automatic option is enabled by the user.

@dsnopek
Copy link
Contributor Author

dsnopek commented Feb 13, 2024

@m4gr3d Thanks for the review!

My only concern is with regard to the mechanism to enable the overrides; at the moment it's up to the plugin which means the plugin can provide an option as you demonstrate in your example, or it can automatically apply the overrides without allowing the user to control the feature.

Personally, I think this is fine. Export plugins already have a ton of power, and they are already responsible for including some way to turn their behaviors on/off. So, I don't think we need to add special safe guards for how this new bit of functionality is used.

Also, I could imagine an addon that allows developers to enable some options across the board. For example, I know some people who always use "Embed PCK", so you could make an addon that just forces that option to be on. :-) Or, maybe the addon could add some editor settings to override all export presets?

How do we resolve plugin overrides conflicts?

That's an interesting question!

In the current implementation, export plugin will override the overrides added by earlier export plugins, but we don't have any control over the order that they run in.

For the specific example you gave, it should be fine, because one is just a subset of the other: they don't have any values that differ, one just has fewer values.

We could certainly add some sort of export plugin priority, but i don't know if it makes sense to add that kind of complexity before we know if it's really needed.

An alternative that aligns with your current proposal would be automatically add a template/configure_for_<plugin_name> option for any editor export plugin that declares that it provides options overrides (declared via a bool method to be overridden).

I'm not crazy about this alternative, because it limits how addons could potentially use this functionality.

And, like I said above, we're already trusting EditorExportPlugin's quite a bit, and don't require them to add export options to do anything else they're able to do - they're already expected to add their own options or have some other mechanism for deciding if they're going to do their thing or not.

@dsnopek
Copy link
Contributor Author

dsnopek commented Feb 13, 2024

And, like I said above, we're already trusting EditorExportPlugin's quite a bit, and don't require them to add export options to do anything else they're able to do - they're already expected to add their own options or have some other mechanism for deciding if they're going to do their thing or not.

To add some examples regarding this point...

Even before this PR, any EditorExportPlugin can implement, for example, _customize_resource() and change all resources that are being exported in some way. There is no requirement that it adds an export option to enable/disable this behavior - it probably will add some mechanism for that, but it's not required to. (As an example of how this can be used, the DedicatedServerExportPlugin uses _customize_resource() to replace all art assets with placeholders to make headless builds smaller.)

There's also _customize_scene() that an EditorExportPlugin could use to add/remove Node's to every scene that's being exported. It could be used in a bunch of silly or unfriendly ways, for example, adding an AudioStreamPlayer that causes every scene to play an annoying sound. :-)

With all that power already, I don't know that we need to try to prevent abuse of overriding export options, which I'd personally even argue is a more modest power than the powers it already has.

@m4gr3d
Copy link
Contributor

m4gr3d commented Feb 13, 2024

Personally, I think this is fine. Export plugins already have a ton of power, and they are already responsible for including some way to turn their behaviors on/off. So, I don't think we need to add special safe guards for how this new bit of functionality is used.

I usually have the opposite opinion.. guidelines that are not enforced are usually overlooked by developers creating a poor user experience for end users, so I usually lean on enforcing the guideline.
A toggle for example would allow users to create specific export presets each with a different override enabled. It would also assist in conflicts situations as the user can explicitly disable the plugin that conflicts.

It's true that export plugins have a lot of power already, though they still need to be enabled through the Project Settings -> Plugins section (though gdextension plugins are notoriously exempt from that requirement).
Maybe we can also use that setting as fallback, though I'd be curious to see what others think.

@dsnopek
Copy link
Contributor Author

dsnopek commented Feb 13, 2024

It's true that export plugins have a lot of power already, though they still need to be enabled through the Project Settings -> Plugins section (though gdextension plugins are notoriously exempt from that requirement).

FYI, adding a way to enable/disable GDExtensions in project settings is planned - it just hasn't been implemented yet :-)

@dsnopek dsnopek force-pushed the export-plugin-option-overrides branch from 3d32ba6 to 39557ef Compare February 14, 2024 16:10
@dsnopek
Copy link
Contributor Author

dsnopek commented Feb 14, 2024

@m4gr3d: Thanks for the review! My latest push is updated for all your comments :-)

@Mickeon:

Could use a similar example you provided in this PR in the documentation in a [codeblock] (albeit shorter of course).

Sure! In my latest push, I've added a [codeblock] with a shorter example.

@dsnopek dsnopek force-pushed the export-plugin-option-overrides branch from 39557ef to 8b8ae9c Compare February 14, 2024 16:14
Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

New dynamic feature with a reasonable example!?!?
Satisfactory

doc/classes/EditorExportPlugin.xml Outdated Show resolved Hide resolved
Copy link
Contributor

@m4gr3d m4gr3d left a comment

Choose a reason for hiding this comment

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

Looks great!

I think we can go ahead and merge this. My comments on the ability to enable/disable the added capability are part of a larger conversation about how much user control we give over plugins capabilities and should not be a blocker for this PR.

doc/classes/EditorExportPlugin.xml Outdated Show resolved Hide resolved
@dsnopek dsnopek force-pushed the export-plugin-option-overrides branch from 8b8ae9c to ac88acd Compare February 14, 2024 16:51
@akien-mga akien-mga merged commit 828cf95 into godotengine:master Feb 15, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@dsnopek dsnopek deleted the export-plugin-option-overrides branch July 22, 2024 15:31
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.

4 participants