-
-
Notifications
You must be signed in to change notification settings - Fork 21.9k
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
Project feature warning system #31171
Conversation
3e7f755
to
2c16d18
Compare
2c16d18
to
54eabac
Compare
54eabac
to
ec0efa7
Compare
2610c82
to
683493f
Compare
76c42a0
to
ce26ba2
Compare
ce26ba2
to
ac27514
Compare
ac27514
to
bcdf4c7
Compare
bcdf4c7
to
a77d79a
Compare
a77d79a
to
2a2dc52
Compare
2a2dc52
to
32f30bb
Compare
151b1e7
to
b2805b7
Compare
656a851
to
59414b5
Compare
Yes that is the idea, otherwise it would be a major compatibility breakage between 4.0 and 4.1.
Not against a version check, this part may go through, but needs to be consulted with other contributors. For the rest, I think using feature tags for this is not a good idea, because most of them are either irrelevant or platform dependent. IMO something more specialized is required for this situation. I think this needs to be something else more related to compilation flags (engine version, whether it uses doubles, etc) and not feature tags. I suggest going back to the drawing board with this and create a new proposal. |
All the current feature tags are determined at compile-time, so double precision support could be a feature tag. |
6644764
to
43fe34f
Compare
43fe34f
to
81b2676
Compare
81b2676
to
6bd80e1
Compare
editor/project_manager.cpp
Outdated
if (renderer_type == 0) { | ||
project_features.push_back("Vulkan 1 Clustered"); | ||
} else if (renderer_type == 1) { | ||
project_features.push_back("Vulkan 1 Mobile"); | ||
} else { | ||
WARN_PRINT("Unknown renderer type. Please report this as a bug on GitHub."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be hardcoded (it's no longer accurate either). I mean the numbers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you happen to know where the enum is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it's hard-coded here too (servers/rendering_server.cpp:2827
):
GLOBAL_DEF_RST_BASIC("rendering/vulkan/rendering/back_end", 0);
GLOBAL_DEF_RST_BASIC("rendering/vulkan/rendering/back_end.mobile", 1);
ProjectSettings::get_singleton()->set_custom_property_info("rendering/vulkan/rendering/back_end",
PropertyInfo(Variant::INT,
"rendering/vulkan/rendering/back_end",
PROPERTY_HINT_ENUM, "Forward Clustered (Supports Desktop Only),Forward Mobile (Supports Desktop and Mobile)"));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm right, I guess it's fine for now. We can rethink this once the OpenGL backend is ready and needs to be handled too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved in PR meeting.
We might want to iterate on the actual implementation to see what makes the most sense, but this is a good start to build upon.
6bd80e1
to
f64c2b7
Compare
editor/project_manager.cpp
Outdated
String warning_message = ""; | ||
for (int i = 0; i < unsupported_features.size(); i++) { | ||
String feature = unsupported_features[i]; | ||
if (feature == "Double Precision") { | ||
warning_message += TTR("Warning: This project uses double precision floats, but this version of\nGodot uses single precision floats. Opening this project may cause data loss.\n\n"); | ||
unsupported_features.remove(i); | ||
i--; | ||
} else if (feature == "C#") { | ||
warning_message += TTR("Warning: This project uses C#, but this build of Godot does not have\nthe Mono module. If you proceed you will not be able to use any C# scripts.\n\n"); | ||
unsupported_features.remove(i); | ||
i--; | ||
} else if (feature.substr(0, 3).is_numeric()) { | ||
warning_message += vformat(TTR("Warning: This project was built in Godot %s.\nOpening will upgrade or downgrade the project to Godot %s.\n\n"), Variant(feature), Variant(VERSION_BRANCH)); | ||
unsupported_features.remove(i); | ||
i--; | ||
} | ||
} | ||
if (!unsupported_features.is_empty()) { | ||
String unsupported_features_str = Variant(unsupported_features).operator String().trim_prefix("[").trim_suffix("]"); | ||
warning_message += vformat(TTR("Warning: This project uses the following features not supported by this build of Godot:\n\n%s\n\n"), unsupported_features_str); | ||
} | ||
warning_message += TTR("Open anyway? Project will be modified."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code has been updated to have more specific error messages, but still have a fallback for anything not covered by those messages.
130fbbd
to
f986caf
Compare
You can remove me as co-author, it's not deserved for a style nitpick ;) |
f986caf
to
e81c689
Compare
Thanks! |
(Note: This PR also increments the config version by necessity)
A mostly complete implementation of a feature warning system, implements and closes godotengine/godot-proposals#427, implements and closes godotengine/godot-proposals#237
EDIT: The screenshot is a bit outdated, since now there are more specific error messages available.
What works: Reading project features from
project.godot
, displaying unsupported features, warning when trying to open a project with unsupported features, changing the features to comply with currently supported and required features, and saving project features toproject.godot
.This PR uses the new feature system for the following:
config_version
, so as an example if 4.0 and 4.1 use the same config version then this system will show a warning when opening 4.0 projects in 4.1 and 4.1 projects in 4.0, but if 4.2 uses a higher config version then upgrading to 4.2 would be one-way.