Greater VCS friendliness

[RandomShaper]

Make AnimationPlayer serialize its blend times always sorted so their order is predictable in the .tscn file.
Serialize dictionaries adding newlines between key-value pairs.
Serialize group lists also with newlines in between.
Serialize string properties escaping only " and \ (needed for a good diff experience with built-in scripts).

This PR is back-compat; won't break loading of existing files.

[akien-mga]

Can you provide a before/after example? :)

[reduz]

Wait before merging this, I want to give it a check

On Oct 31, 2016 16:56, "Rémi Verschelde" notifications@github.com wrote:

Can you provide a before/after example? :)

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#7002 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AF-Z23YuVCjhPkV6LyKTrMZJmpuxjF3yks5q5kf0gaJpZM4Klael
.

[akien-mga]

@reduz Definitely, I'm not touching VariantParser personally :p

[RandomShaper]

Before:

[gd_scene load_steps=7 format=1]

[sub_resource type="CanvasItemShader" id=1]

_code = { "fragment":"// Useless variables\nfloat a;\nfloat b;\n// Some output\nCOLOR = vec4(0, 0, 0, 1);\n", "fragment_ofs":0, "light":"", "light_ofs":0, "vertex":"", "vertex_ofs":0 }

[sub_resource type="CanvasItemMaterial" id=2]

shader/shader = SubResource( 1 )
shader/shading_mode = 0

[sub_resource type="GDScript" id=3]

script/source = "extends Node2D\n\nfunc _ready():\n\tpass\n"

[sub_resource type="Animation" id=4]

length = 1.0
loop = false
step = 0.1

[sub_resource type="Animation" id=5]

length = 1.0
loop = false
step = 0.1

[sub_resource type="Animation" id=6]

length = 1.0
loop = false
step = 0.1

[node name="Node2D" type="Node2D" groups=[ "three", "one", "two" ]]

material/material = SubResource( 2 )
script/script = SubResource( 3 )

[node name="AnimationPlayer" type="AnimationPlayer" parent="."]

playback/process_mode = 1
playback/default_blend_time = 0.0
root/root = NodePath("..")
anims/a = SubResource( 4 )
anims/b = SubResource( 5 )
anims/c = SubResource( 6 )
playback/active = true
playback/speed = 1.0
blend_times = [ "b", "a", 0.3, "c", "b", 0.2, "a", "b", 0.4, "a", "c", 0.2 ]
autoplay = ""

After (comments added in uppercase):

[gd_scene load_steps=7 format=1]

[sub_resource type="CanvasItemShader" id=1]

# MULTILINE DICTIONARY AND CODE STRING
_code = {
"fragment": "// Useless variables
float a;
float b;
// Some output
COLOR = vec4(0, 0, 0, 1);
",
"fragment_ofs": 0,
"light": "",
"light_ofs": 0,
"vertex": "",
"vertex_ofs": 0
}

[sub_resource type="CanvasItemMaterial" id=2]

shader/shader = SubResource( 1 )
shader/shading_mode = 0

[sub_resource type="GDScript" id=3]

# MULTILINE CODE STRING
script/source = "extends Node2D

func _ready():
    pass
"

[sub_resource type="Animation" id=4]

length = 1.0
loop = false
step = 0.1

[sub_resource type="Animation" id=5]

length = 1.0
loop = false
step = 0.1

[sub_resource type="Animation" id=6]

length = 1.0
loop = false
step = 0.1

# MULTILINE ARRAY OF GROUPS
[node name="Node2D" type="Node2D" groups=[
"three",
"one",
"two",
]]

material/material = SubResource( 2 )
script/script = SubResource( 3 )

[node name="AnimationPlayer" type="AnimationPlayer" parent="."]

playback/process_mode = 1
playback/default_blend_time = 0.0
root/root = NodePath("..")
anims/a = SubResource( 4 )
anims/b = SubResource( 5 )
anims/c = SubResource( 6 )
playback/active = true
playback/speed = 1.0
# SORTED ANIMATION PAIRS
blend_times = [ "a", "b", 0.4, "a", "c", 0.2, "b", "a", 0.3, "c", "b", 0.2 ]
autoplay = ""

[akien-mga]

Wait before merging this, I want to give it a check

@reduz Ping :)

I've tested the patch locally and it seems to work fine.

[reduz]

If changing the order makes it more VCS friendly, just change the order, no need to add a condition

[akien-mga]

I think it's not just changing the order here, but actually disabling various escapes when in VCS friendly mode.

[bojidar-bg]

I think the vcs-friendly mode would be better in its own function.

[RandomShaper]

Akien is right. Now, about splitting the function into the two use cases, I did it this way with a default value for the argument to keep the API as untouched as possible, but I see no reason why it couldn't be done if you prefer.

[akien-mga]

I think a new method would be good indeed, as the current change would mean exposing a boolean argument to the GDScript API, where it is not particularly meaningful as par of a c_escape() function IINM.

Maybe add a tscn_escape method (or a better name if you think of one), which potentially wouldn't need to be exposed to GDScript.

[RandomShaper]

I've decided to name it c_escape_multiline(). The rationale is that it performs C-style string escaping as well, but keeping the multiline structure of the source data (and any other format control characters like tab and those exotic vertical tabs, backspace, etc.).

That name also makes it clear its intent where it's used.

What do you think?

[akien-mga]

Sounds good :)