Skip to content

Commit

Permalink
Fix some arguments being passed as references in virtual functions
Browse files Browse the repository at this point in the history
These can't be used safely in extensions, replaced with `Dictionary`
returns
  • Loading branch information
AThousandShips committed Nov 1, 2024
1 parent c6c464c commit a216584
Show file tree
Hide file tree
Showing 10 changed files with 89 additions and 51 deletions.
6 changes: 2 additions & 4 deletions doc/classes/EditorImportPlugin.xml
Original file line number Diff line number Diff line change
Expand Up @@ -223,14 +223,12 @@
</description>
</method>
<method name="_import" qualifiers="virtual const">
<return type="int" enum="Error" />
<return type="Dictionary" />
<param index="0" name="source_file" type="String" />
<param index="1" name="save_path" type="String" />
<param index="2" name="options" type="Dictionary" />
<param index="3" name="platform_variants" type="String[]" />
<param index="4" name="gen_files" type="String[]" />
<description>
Imports [param source_file] into [param save_path] with the import [param options] specified. The [param platform_variants] and [param gen_files] arrays will be modified by this function.
Imports [param source_file] into [param save_path] with the import [param options] specified.
This method must be overridden to do the actual importing work. See this class' description for an example of overriding this method.
</description>
</method>
Expand Down
8 changes: 2 additions & 6 deletions doc/classes/EditorResourcePreviewGenerator.xml
Original file line number Diff line number Diff line change
Expand Up @@ -17,27 +17,23 @@
</description>
</method>
<method name="_generate" qualifiers="virtual const">
<return type="Texture2D" />
<return type="Dictionary" />
<param index="0" name="resource" type="Resource" />
<param index="1" name="size" type="Vector2i" />
<param index="2" name="metadata" type="Dictionary" />
<description>
Generate a preview from a given resource with the specified size. This must always be implemented.
Returning [code]null[/code] is an OK way to fail and let another generator take care.
Care must be taken because this function is always called from a thread (not the main thread).
[param metadata] dictionary can be modified to store file-specific metadata that can be used in [method EditorResourceTooltipPlugin._make_tooltip_for_path] (like image size, sample length etc.).
</description>
</method>
<method name="_generate_from_path" qualifiers="virtual const">
<return type="Texture2D" />
<return type="Dictionary" />
<param index="0" name="path" type="String" />
<param index="1" name="size" type="Vector2i" />
<param index="2" name="metadata" type="Dictionary" />
<description>
Generate a preview directly from a path with the specified size. Implementing this is optional, as default code will load and call [method _generate].
Returning [code]null[/code] is an OK way to fail and let another generator take care.
Care must be taken because this function is always called from a thread (not the main thread).
[param metadata] dictionary can be modified to store file-specific metadata that can be used in [method EditorResourceTooltipPlugin._make_tooltip_for_path] (like image size, sample length etc.).
</description>
</method>
<method name="_generate_small_preview_automatically" qualifiers="virtual const">
Expand Down
4 changes: 1 addition & 3 deletions doc/classes/EditorTranslationParserPlugin.xml
Original file line number Diff line number Diff line change
Expand Up @@ -107,10 +107,8 @@
</description>
</method>
<method name="_parse_file" qualifiers="virtual">
<return type="void" />
<return type="Dictionary" />
<param index="0" name="path" type="String" />
<param index="1" name="msgids" type="String[]" />
<param index="2" name="msgids_context_plural" type="Array[]" />
<description>
Override this method to define a custom parsing logic to extract the translatable strings.
</description>
Expand Down
28 changes: 20 additions & 8 deletions editor/editor_resource_preview.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -53,17 +53,29 @@ bool EditorResourcePreviewGenerator::handles(const String &p_type) const {
}

Ref<Texture2D> EditorResourcePreviewGenerator::generate(const Ref<Resource> &p_from, const Size2 &p_size, Dictionary &p_metadata) const {
Ref<Texture2D> preview;
if (GDVIRTUAL_CALL(_generate, p_from, p_size, p_metadata, preview)) {
return preview;
Dictionary ret;
if (GDVIRTUAL_CALL(_generate, p_from, p_size, ret)) {
if (!ret.has("result")) {
return Ref<Texture2D>();
}
if (ret.has("metadata")) {
p_metadata = ret["metadata"];
}
return ret["result"];
}
ERR_FAIL_V_MSG(Ref<Texture2D>(), "EditorResourcePreviewGenerator::_generate needs to be overridden.");
}

Ref<Texture2D> EditorResourcePreviewGenerator::generate_from_path(const String &p_path, const Size2 &p_size, Dictionary &p_metadata) const {
Ref<Texture2D> preview;
if (GDVIRTUAL_CALL(_generate_from_path, p_path, p_size, p_metadata, preview)) {
return preview;
Dictionary ret;
if (GDVIRTUAL_CALL(_generate_from_path, p_path, p_size, ret)) {
if (!ret.has("result")) {
return Ref<Texture2D>();
}
if (ret.has("metadata")) {
p_metadata = ret["metadata"];
}
return ret["result"];
}

Ref<Resource> res = ResourceLoader::load(p_path);
Expand All @@ -87,8 +99,8 @@ bool EditorResourcePreviewGenerator::can_generate_small_preview() const {

void EditorResourcePreviewGenerator::_bind_methods() {
GDVIRTUAL_BIND(_handles, "type");
GDVIRTUAL_BIND(_generate, "resource", "size", "metadata");
GDVIRTUAL_BIND(_generate_from_path, "path", "size", "metadata");
GDVIRTUAL_BIND(_generate, "resource", "size");
GDVIRTUAL_BIND(_generate_from_path, "path", "size");
GDVIRTUAL_BIND(_generate_small_preview_automatically);
GDVIRTUAL_BIND(_can_generate_small_preview);
}
Expand Down
4 changes: 2 additions & 2 deletions editor/editor_resource_preview.h
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,8 @@ class EditorResourcePreviewGenerator : public RefCounted {
static void _bind_methods();

GDVIRTUAL1RC(bool, _handles, String)
GDVIRTUAL3RC(Ref<Texture2D>, _generate, Ref<Resource>, Vector2i, Dictionary)
GDVIRTUAL3RC(Ref<Texture2D>, _generate_from_path, String, Vector2i, Dictionary)
GDVIRTUAL2RC(Dictionary, _generate, Ref<Resource>, Vector2i)
GDVIRTUAL2RC(Dictionary, _generate_from_path, String, Vector2i)
GDVIRTUAL0RC(bool, _generate_small_preview_automatically)
GDVIRTUAL0RC(bool, _can_generate_small_preview)

Expand Down
37 changes: 20 additions & 17 deletions editor/editor_translation_parser.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -38,25 +38,28 @@
EditorTranslationParser *EditorTranslationParser::singleton = nullptr;

Error EditorTranslationParserPlugin::parse_file(const String &p_path, Vector<String> *r_ids, Vector<Vector<String>> *r_ids_ctx_plural) {
TypedArray<String> ids;
TypedArray<Array> ids_ctx_plural;

if (GDVIRTUAL_CALL(_parse_file, p_path, ids, ids_ctx_plural)) {
// Add user's extracted translatable messages.
for (int i = 0; i < ids.size(); i++) {
r_ids->append(ids[i]);
Dictionary ret;

if (GDVIRTUAL_CALL(_parse_file, p_path, ret)) {
// TODO: Errors for invalid return?
if (ret.has("ids")) {
TypedArray<String> ids = ret["ids"];
for (const String id : ids) {
r_ids->push_back(id);
}
}

// Add user's collected translatable messages with context or plurals.
for (int i = 0; i < ids_ctx_plural.size(); i++) {
Array arr = ids_ctx_plural[i];
ERR_FAIL_COND_V_MSG(arr.size() != 3, ERR_INVALID_DATA, "Array entries written into `msgids_context_plural` in `parse_file()` method should have the form [\"message\", \"context\", \"plural message\"]");
if (ret.has("ids_ctx_plural")) {
TypedArray<Array> ids_ctx_plural = ret["ids_ctx_plural"];
for (const Array arr : ids_ctx_plural) {
ERR_FAIL_COND_V_MSG(arr.size() != 3, ERR_INVALID_DATA, "Array entries written into `msgids_context_plural` in `parse_file()` method should have the form [\"message\", \"context\", \"plural message\"]");

Vector<String> id_ctx_plural;
id_ctx_plural.push_back(arr[0]);
id_ctx_plural.push_back(arr[1]);
id_ctx_plural.push_back(arr[2]);
r_ids_ctx_plural->append(id_ctx_plural);
Vector<String> id_ctx_plural;
id_ctx_plural.push_back(arr[0]);
id_ctx_plural.push_back(arr[1]);
id_ctx_plural.push_back(arr[2]);
r_ids_ctx_plural->push_back(id_ctx_plural);
}
}
return OK;
} else {
Expand All @@ -77,7 +80,7 @@ void EditorTranslationParserPlugin::get_recognized_extensions(List<String> *r_ex
}

void EditorTranslationParserPlugin::_bind_methods() {
GDVIRTUAL_BIND(_parse_file, "path", "msgids", "msgids_context_plural");
GDVIRTUAL_BIND(_parse_file, "path");
GDVIRTUAL_BIND(_get_recognized_extensions);
}

Expand Down
2 changes: 1 addition & 1 deletion editor/editor_translation_parser.h
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ class EditorTranslationParserPlugin : public RefCounted {
protected:
static void _bind_methods();

GDVIRTUAL3(_parse_file, String, TypedArray<String>, TypedArray<Array>)
GDVIRTUAL1R(Dictionary, _parse_file, String)
GDVIRTUAL0RC(Vector<String>, _get_recognized_extensions)

public:
Expand Down
35 changes: 26 additions & 9 deletions editor/import/editor_import_plugin.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -165,23 +165,40 @@ bool EditorImportPlugin::get_option_visibility(const String &p_path, const Strin

Error EditorImportPlugin::import(const String &p_source_file, const String &p_save_path, const HashMap<StringName, Variant> &p_options, List<String> *r_platform_variants, List<String> *r_gen_files, Variant *r_metadata) {
Dictionary options;
TypedArray<String> platform_variants, gen_files;

HashMap<StringName, Variant>::ConstIterator E = p_options.begin();
while (E) {
options[E->key] = E->value;
++E;
}

Error err = OK;
if (GDVIRTUAL_CALL(_import, p_source_file, p_save_path, options, platform_variants, gen_files, err)) {
for (int i = 0; i < platform_variants.size(); i++) {
r_platform_variants->push_back(platform_variants[i]);
Dictionary ret;
if (GDVIRTUAL_CALL(_import, p_source_file, p_save_path, options, ret)) {
if (!ret.has("result")) {
return ERR_UNAVAILABLE;
}
for (int i = 0; i < gen_files.size(); i++) {
r_gen_files->push_back(gen_files[i]);

if (r_platform_variants != nullptr && ret.has("platform_variants")) {
TypedArray<String> platform_variants = ret["platform_variants"];
for (const String platform_variant : platform_variants) {
r_platform_variants->push_back(platform_variant);
}
}

if (r_gen_files != nullptr && ret.has("gen_files")) {
TypedArray<String> gen_files = ret["gen_files"];
for (const String gen_file : gen_files) {
r_gen_files->push_back(gen_file);
}
}
return err;

if (r_metadata != nullptr && ret.has("metadata")) {
*r_metadata = ret["metadata"];
}

Error result = Error(int(ret["result"]));

return result;
}
ERR_FAIL_V_MSG(ERR_METHOD_NOT_FOUND, "Unimplemented _import in add-on.");
}
Expand Down Expand Up @@ -221,7 +238,7 @@ void EditorImportPlugin::_bind_methods() {
GDVIRTUAL_BIND(_get_priority)
GDVIRTUAL_BIND(_get_import_order)
GDVIRTUAL_BIND(_get_option_visibility, "path", "option_name", "options")
GDVIRTUAL_BIND(_import, "source_file", "save_path", "options", "platform_variants", "gen_files");
GDVIRTUAL_BIND(_import, "source_file", "save_path", "options");
GDVIRTUAL_BIND(_can_import_threaded);
ClassDB::bind_method(D_METHOD("append_import_external_resource", "path", "custom_options", "custom_importer", "generator_parameters"), &EditorImportPlugin::_append_import_external_resource, DEFVAL(Dictionary()), DEFVAL(String()), DEFVAL(Variant()));
}
2 changes: 1 addition & 1 deletion editor/import/editor_import_plugin.h
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ class EditorImportPlugin : public ResourceImporter {
GDVIRTUAL0RC(float, _get_priority)
GDVIRTUAL0RC(int, _get_import_order)
GDVIRTUAL3RC(bool, _get_option_visibility, String, StringName, Dictionary)
GDVIRTUAL5RC(Error, _import, String, String, Dictionary, TypedArray<String>, TypedArray<String>)
GDVIRTUAL3RC(Dictionary, _import, String, String, Dictionary)
GDVIRTUAL0RC(bool, _can_import_threaded)

Error _append_import_external_resource(const String &p_file, const Dictionary &p_custom_options = Dictionary(), const String &p_custom_importer = String(), Variant p_generator_parameters = Variant());
Expand Down
14 changes: 14 additions & 0 deletions misc/extension_api_validation/4.3-stable.expected
Original file line number Diff line number Diff line change
Expand Up @@ -108,3 +108,17 @@ GH-97257
Validate extension JSON: Error: Field 'classes/EditorFeatureProfile/enums/Feature/values/FEATURE_MAX': value changed value in new API, from 8.0 to 9.

New entry to the `EditorFeatureProfile.Feature` enum added. Those need to go before `FEATURE_MAX`, which will always cause a compatibility break.


GH-92175
--------
Validate extension JSON: Error: Field 'classes/EditorImportPlugin/methods/_import/arguments': size changed value in new API, from 5 to 3.
Validate extension JSON: Error: Field 'classes/EditorImportPlugin/methods/_import/return_value': type changed value in new API, from "enum::Error" to "Dictionary".
Validate extension JSON: Error: Field 'classes/EditorResourcePreviewGenerator/methods/_generate/arguments': size changed value in new API, from 3 to 2.
Validate extension JSON: Error: Field 'classes/EditorResourcePreviewGenerator/methods/_generate/return_value': type changed value in new API, from "Texture2D" to "Dictionary".
Validate extension JSON: Error: Field 'classes/EditorResourcePreviewGenerator/methods/_generate_from_path/arguments': size changed value in new API, from 3 to 2.
Validate extension JSON: Error: Field 'classes/EditorResourcePreviewGenerator/methods/_generate_from_path/return_value': type changed value in new API, from "Texture2D" to "Dictionary".
Validate extension JSON: Error: Field 'classes/EditorTranslationParserPlugin/methods/_parse_file/arguments': size changed value in new API, from 3 to 1.
Validate extension JSON: JSON file: Field was added in a way that breaks compatibility 'classes/EditorTranslationParserPlugin/methods/_parse_file': return_value

Return values were passed in constant reference arguments. Changed to using a return Dictionary.

0 comments on commit a216584

Please sign in to comment.