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

Project converter: fix convertion for project.godot file #66251

Conversation

gotnospirit
Copy link
Contributor

This PR is intended to fixes #66125.

While testing this issue, I noticed that adding { "display/window/size/width", "display/window/size/viewport_width" }, to the project_settings_renames array, do not have any effect.

After a closer look, I think this is because project.godot is an INI file

[display]

window/size/width=1280

In this PR, I decided to parse the existing project settings renames into a new structure (so substitutions for ".gd" and ".tscn" files are not affected).

Apart from solving the initial issue, this PR also supports remove keys or move a key to another/new section, based on the original rename config format. Because of that, the project.godot file is fully rewritten at the end of the conversion process.

As a dummy example, adding these lines to the existing config

{ "display/window/size/width", "display/window/size/viewport_width" },
{ "display/window/size/height", "toto/window/size/viewport_height" },
{ "application/config/icon", nullptr },
{ "application/config/name", nullptr },
{ "gui/common/drop_mouse_on_gui_input_disabled", "gui_input_disabled" },
{ "config_version", "physics/version" },

with this project.godot file:

; Engine configuration file.
; It's best edited using the editor UI and not directly,
; since the parameters that go here are not all obvious.
;
; Format:
;   [section] ; section goes between []
;   param=value ; assign values to parameters

config_version=4

[application]

config/name="project_converter_3_to_4-bug3"

config/icon="res://icon.png"

[gui]

common/drop_mouse_on_gui_input_disabled=true

[display]

window/size/width=1280
window/size/height=720 ; move this comment as well

[physics]

common/enable_pause_aware_picking=true

[rendering]

environment/defaults/default_environment="res://default_env.tres"

will produce the following validation check

 2 2 project.godot 0
    Checking file took      0.001 ms.
                - Remove section 'application'
                - Remove section 'gui'
                - Create section 'toto'
                - In 'display', rename 'window/size/width' to 'window/size/viewport_width'
                - In 'rendering', rename 'environment/default_environment' to 'environment/defaults/default_environment'
                - In 'display', remove 'window/size/height=720 ; move this comment as well'
                - In root, remove 'config_version=4'
                - In 'toto', insert 'window/size/viewport_height=720 ; move this comment as well'
                - In root, insert 'gui_input_disabled=true'
                - In 'physics', insert 'version=4'

When converting, the file is rewritten according to the actions detected during the analyze stage.

Any comments are welcomed!

@akien-mga akien-mga added this to the 4.0 milestone Sep 22, 2022
@akien-mga akien-mga requested a review from qarmin September 22, 2022 19:25
@gotnospirit
Copy link
Contributor Author

Bug found: If I want to remove a key or move it to another section and its value is multiline, then the resulting file is broken.

@gotnospirit gotnospirit force-pushed the master-project_converter-process_project_settings branch from 64ba5b2 to 9aeb252 Compare September 25, 2022 05:48
@gotnospirit gotnospirit force-pushed the master-project_converter-process_project_settings branch from c07df6e to a04da1c Compare October 11, 2022 06:13
@gotnospirit
Copy link
Contributor Author

I did some rewrite here in order to be able to later handle resource files (.tscn/.tres) a bit like project.godot ones.

It provides a static method ConfigSection::parse_lines to transform the initial Vector<String> into a Vector<ConfigSection *>.
Every ConfigSection has a name, can have attributes, and contains a Vector<ConfigSection::Element *>.

I defined 5 types of elements:

  • Comment (any line starting with ';' char)
  • EmptyLine (any line empty when stripped)
  • KeyValue (key=value pair)
  • EmbeddedScript (for .tscn files)
  • Basic (any other lines)

As I mentioned here, the problem is that sometimes we need some context to be able to rename things.

Now, we would be able to do things like

Vector<ConfigSection *> sections = ConfigSection::parse_lines(lines, " = ");

for (ConfigSection *section : sections) {
  if (section->name == "sub_resource" && section->get_attr("type") == "\"Animation\"") {
    for (ConfigSection::Element *el : section->elements) {
      if (el->get_type() == ConfigSection::KEY_VALUE) {
        ConfigSection::KeyValue *kv = dynamic_cast<ConfigSection::KeyValue *>(el);
        if (kv->key == "loop" && kv->value == "true") {
          kv->key = "loop_mode";
          kv->value = "1";
        }
      }
    }
  }
}

Vector<String> new_lines;
for (ConfigSection *section : sections) {
  new_lines.append_array(section->dump());
}

if we want to rename the loop parameter for any Animation.

Few things to note:

  • because some attributes are not quoted in v3 (resource ids, node instance, ...), attributes with quoted value contains the quotes (see code example for Animation)
  • multi-line key/value elements contains "\n"
    e.g.:
    for
__meta__ = {
"_editor_description_": "this is my description"
} ; meta comment

the KeyValue element stores

key: "__meta__"
value: "{\n"_editor_description_": "this is my description"\n} ; meta comment"

@gotnospirit
Copy link
Contributor Author

If this PR is approved, I will have to make some change to #66150 (should I turn it into a draft?)

@akien-mga
Copy link
Member

I haven't reviewed the code in-depth but I'm not sure why ConfigSection is needed, this seems to be reimplementing the ConfigFile API?

You should be able to open project.godot as a ConfigFile and do changes to any section/key pair.

@gotnospirit
Copy link
Contributor Author

Regarding a project.godot file, you're absolutly right, it reimplements ConfigFile, but it is also made to be able to handle .tscn/.tres files (kv separator is =, sections can have attributes, and can have embedded scripts).

This allows us to have some kind of context when converting .tscn files, because we can traverse the sections, and for example, if it's a sub_resource of type Animation, we can then traverse its elements and rename loop = true into loop_mode = 1, or apply the class_renames array only to type attributes (as in #66256).

@akien-mga
Copy link
Member

.tscn/.tres files are formats written and read by Godot, so we also already have relevant classes to handle them. See scene/resource/resource_format_text.h, and the lower level core/variant/variant_parser.h.

I don't claim that this exactly fits the bill as I haven't reviewed your code in depth, but I'd still be surprised if we needed so much new code to reimplement loading/editing files that Godot loads/edits all the time.

@akien-mga
Copy link
Member

I went for a much simpler approach in #74040. More hardcoding, but for a converter which relies on hacky use of regexes, I think it's good enough.

@akien-mga akien-mga closed this Feb 27, 2023
@aaronfranke aaronfranke removed this from the 4.x milestone Feb 28, 2023
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.

3-to-4 project converter doesn't preserve viewport width and height
4 participants