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

Jump to bottom

Make Object text-based serialization more readable and VCS-friendly #9774

Open
robert-wallis opened this issue May 19, 2024 · 6 comments · May be fixed by godotengine/godot#92102
Open

Make Object text-based serialization more readable and VCS-friendly #9774

robert-wallis opened this issue May 19, 2024 · 6 comments · May be fixed by godotengine/godot#92102
Labels
topic:core usability

Comments

@robert-wallis
Copy link

robert-wallis commented May 19, 2024 •
edited

Describe the project you are working on

Upgrading the engine from 4.2 to 4.3 in my project.

Describe the problem or limitation you are having in your project

When you git diff after upgrading the engine, and an InputEventKey has a new property, it's very difficult to see what actually changed:
git diff InputEventKey Object is one line with many properties

Describe the feature / enhancement and how it helps to overcome the problem or limitation

This is because Objects are not written the same way that Dictionary objects are.
Dictionaries have newlines \n after each comma. core/variant/variant_parser.cpp lines 2054-2066

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

By adding newline "\n" characters after commas "," in the variant writer. Now when you run git diff it shows exactly what property changed. In the case of 4.2 to 4.3 it was a new property "location":0
Screenshot 2024-05-18 at 6 36 20 PM

This was almost impossible to see when everything in Object was serialized in one line as shown above.

If this enhancement will not be used often, can it be worked around with a few lines of script?

This simple PR has the changes godotengine/godot#92102

Is there a reason why this should be core and not an add-on in the asset library?

The project.godot file is serialized with Object variants for key events.
@robert-wallis robert-wallis linked a pull request May 19, 2024 that will close this issue
Open
@robert-wallis
Copy link
Author

Old ui_accept Example

project.godot

ui_accept={
"deadzone": 0.5,
"events": [Object(InputEventKey,"resource_local_to_scene":false,"resource_name":"","device":0,"window_id":0,"alt_pressed":false,"shift_pressed":false,"ctrl_pressed":false,"meta_pressed":false,"pressed":false,"keycode":4194309,"physical_keycode":0,"key_label":0,"unicode":0,"echo":false,"script":null)
, Object(InputEventKey,"resource_local_to_scene":false,"resource_name":"","device":0,"window_id":0,"alt_pressed":false,"shift_pressed":false,"ctrl_pressed":false,"meta_pressed":false,"pressed":false,"keycode":4194310,"physical_keycode":0,"key_label":0,"unicode":0,"echo":false,"script":null)
, Object(InputEventKey,"resource_local_to_scene":false,"resource_name":"","device":0,"window_id":0,"alt_pressed":false,"shift_pressed":false,"ctrl_pressed":false,"meta_pressed":false,"pressed":false,"keycode":32,"physical_keycode":0,"key_label":0,"unicode":32,"echo":false,"script":null)
, Object(InputEventJoypadButton,"resource_local_to_scene":false,"resource_name":"","device":-1,"button_index":0,"pressure":0.0,"pressed":false,"script":null)
]
}

New ui_accept Example

project.godot

ui_accept={
"deadzone": 0.5,
"events": [Object(
InputEventKey,
"resource_local_to_scene":false,
"resource_name":"",
"device":0,
"window_id":0,
"alt_pressed":false,
"shift_pressed":false,
"ctrl_pressed":false,
"meta_pressed":false,
"pressed":false,
"keycode":4194309,
"physical_keycode":0,
"key_label":0,
"unicode":0,
"location":0,
"echo":false,
"script":null
), Object(
InputEventKey,
"resource_local_to_scene":false,
"resource_name":"",
"device":0,
"window_id":0,
"alt_pressed":false,
"shift_pressed":false,
"ctrl_pressed":false,
"meta_pressed":false,
"pressed":false,
"keycode":4194310,
"physical_keycode":0,
"key_label":0,
"unicode":0,
"location":0,
"echo":false,
"script":null
), Object(
InputEventKey,
"resource_local_to_scene":false,
"resource_name":"",
"device":0,
"window_id":0,
"alt_pressed":false,
"shift_pressed":false,
"ctrl_pressed":false,
"meta_pressed":false,
"pressed":false,
"keycode":32,
"physical_keycode":0,
"key_label":0,
"unicode":32,
"location":0,
"echo":false,
"script":null
), Object(
InputEventJoypadButton,
"resource_local_to_scene":false,
"resource_name":"",
"device":-1,
"button_index":0,
"pressure":0.0,
"pressed":false,
"script":null
)]
}

@KoBeWi
Copy link
Member

KoBeWi commented May 19, 2024

The extra \n can increase file size considerably. Your new example is ~20% bigger.

@Calinou
Copy link
Member

Calinou commented May 19, 2024 •
edited

The extra \n can increase file size considerably. Your new example is ~20% bigger.

I don't think this is a big deal for text-based resources, as they get converted to binary on export (by default). This includes project.godot which becomes project.binary.

@Gnumaru
Copy link

Gnumaru commented May 21, 2024 •
edited

Besides text-based resources being converted to binary on export by default, the main reasons for using text based resources during development instead of binary resources is having version control friendly files and human readable and editable files. IMHO the increased file size of having that extra \n is a very low price for all the benefits it brings. In the past I have created helper functions that wrap var_to_str and str_to_var only for the sake of putting and removing the newlines in order to trivially save and load non resource data to disk and keep it human readable and editable.

@Calinou Calinou changed the title git diff on Object variants is harder to understand than Dictionary variants Make Object text-based serialization more readable and VCS-friendly May 21, 2024
@robert-wallis
Copy link
Author

I made this repo to show why newlines are important for VCS: https://github.com/robert-wallis/godot-object-lf-git

There are two main branches: no-lf and yes-lf. no-lf was made with Godot 4.2.2, and yes-lf is with the patched 4.3 branch that adds \n characters.

Here's the scenario:

  1. An Input Event has been added to close_tab, it is bound to Ctrl+X
  2. One branch fixes this binding from Ctrl+X to Ctrl+W because W is more common.
  3. Another branch fixes the windows only binding of Ctrl to Command/Ctrl auto, so it works on Mac or Windows

When newlines are missing, this causes a merge conflict: robert-wallis/godot-object-lf-git#2
no-lf

When newlines are added, the merge happens successfully: robert-wallis/godot-object-lf-git#4
yes-lf

Even github cannot solve this merge conflict easily robert-wallis/godot-object-lf-git#4 because that's just the way git diff works. ∎ 😋

@robert-wallis
Copy link
Author

If you're trying to decide how to resolve the above merge conflict, and you take either the top or bottom line, you accidently cause a bug regression.

  • Take top: erases command_or_control_autoremap
  • Take bottom: erases keycode from X to W

Both commits need to be merged, not take one or the other.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
topic:core usability
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Write Object variants with '\n' between properties robert-wallis/godot
5 participants
@robert-wallis @Calinou @KoBeWi @Gnumaru @dalexeev