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

Implement textual ext/subresource IDs. #50676

Merged
merged 1 commit into from
Jul 22, 2021

Conversation

reduz
Copy link
Member

@reduz reduz commented Jul 21, 2021

Implements godotengine/godot-proposals#471

  • Friendlier with version control.
  • Generates pseudo unique IDs, to minimize conflicts when merging, but still user readable (so, not UUID).
  • Eventually will also allow to have more precisely named sub-resources in imported files.
  • This will allow better reloading on changes (including resources already loaded) as well as better keeping track of changes on the DCC.
  • Keeps backward compatibility with the old formats.
  • Binary and text format version incremented to mark breakage in forward
    compatibility.

How it looks:

[ext_resource path="res://icon2.png" type="Texture2D" id="1_ylvbm"]
[sub_resource type="SpriteFrames" id="SpriteFrames-a9ryz"]
...
frames = SubResource( "SpriteFrames-a9ryz" )
icon = ExtResource( "1_ylvbm" )

BEFORE MERGING: TEST WELL!! A BUG HERE WILL BREAK SCENE FILES, RESULTING IN DATA LOSS.

This can be backported to 3.x.

Bugsquad edit: This closes godotengine/godot-proposals#471 and closes #22226.

@reduz reduz requested a review from a team as a code owner July 21, 2021 00:46
@Calinou
Copy link
Member

Calinou commented Jul 21, 2021

cc @Wavesonics, who was implementing a feature like this at some point.

@Zylann
Copy link
Contributor

Zylann commented Jul 21, 2021

Super-minor: could use _ instead of - so double-click will select the whole ID in most text editors?

@reduz
Copy link
Member Author

reduz commented Jul 21, 2021

@Zylann aknowledged

@reduz reduz changed the title Implement textual subresource IDs. Implement textual ext/subresource IDs. Jul 21, 2021
@reduz reduz force-pushed the textual-resource-ids branch 3 times, most recently from 74a1d30 to c76e600 Compare July 21, 2021 02:44
@Wavesonics
Copy link
Contributor

This is great! I wounder if we could apply something similar to the Resource IDs, that would solve the other (lesser) half of the issue.

@reduz
Copy link
Member Author

reduz commented Jul 21, 2021

@Wavesonics It applies to the resource IDs already

@akien-mga
Copy link
Member

akien-mga commented Jul 21, 2021

Updated to use natural sorting of IDs (so 11_* > 2_*).

@akien-mga
Copy link
Member

Example of how this looks like on a 3.x project (with #50693) when removing a Node that used an ext_resource:

diff --git a/menu/main_menu.tscn b/menu/main_menu.tscn
index 06d3c9a..89131b7 100644
--- a/menu/main_menu.tscn
+++ b/menu/main_menu.tscn
@@ -1,4 +1,4 @@
-[gd_scene load_steps=29 format=2]
+[gd_scene load_steps=28 format=2]
 
 [ext_resource path="res://menu/main_menu.gd" type="Script" id="1"]
 [ext_resource path="res://art/intro_sky_bg.png" type="Texture" id="2"]
@@ -9,7 +9,6 @@
 [ext_resource path="res://art/intro_logo.png" type="Texture" id="7"]
 [ext_resource path="res://art/intro_taptofly.png" type="Texture" id="8"]
 [ext_resource path="res://art/alpaca.png" type="Texture" id="9_kqlfj"]
-[ext_resource path="res://art/alpacocha_small.png" type="Texture" id="10_j67j6"]
 [ext_resource path="res://art/alpaquitas.png" type="Texture" id="11_j68oo"]
 [ext_resource path="res://art/arbol.png" type="Texture" id="12_nyfon"]
 [ext_resource path="res://new_noisetexture.res" type="Texture" id="13_jxnjk"]
@@ -409,9 +408,6 @@ margin_bottom = 40.0
 [node name="Sprite" type="Sprite" parent="Control"]
 texture = ExtResource( "9_kqlfj" )
 
-[node name="Sprite" type="Sprite" parent="."]
-texture = ExtResource( "10_j67j6" )
-
 [node name="Sprite2" type="Sprite" parent="."]
 texture = ExtResource( "11_j68oo" )
 

So no more reordering of all indices like shown in #22226.

@starry-abyss
Copy link
Contributor

Will this allow (in future) to auto-correct references to assets in scripts, when the assets are renamed/moved?

@reduz
Copy link
Member Author

reduz commented Jul 21, 2021

@starry-abyss This is internal for scenes, I think what you are looking for is UUID support.

@Error7Studios
Copy link

related? godotengine/godot-proposals#2365

@ignaloidas
Copy link

On initial inspection everything works fine besides the default initial default_env.tres getting changed after creating a scene.

diff --git a/default_env.tres b/default_env.tres
index ddf6bb7..636a639 100644
--- a/default_env.tres
+++ b/default_env.tres
@@ -1,7 +1,7 @@
 [gd_resource type="Environment" load_steps=2 format=2]
 
-[sub_resource type="Sky" id=1]
+[sub_resource type="Sky" id="1"]
 
 [resource]
 background_mode = 2
-sky = SubResource( 1 )
+sky = SubResource( "1" )

I'm gonna go through some older code where we had some issues with merging and check whether the issues are gone a bit later.

core/io/resource.cpp Outdated Show resolved Hide resolved
core/io/resource.h Outdated Show resolved Hide resolved
core/io/resource_format_binary.cpp Outdated Show resolved Hide resolved
* Friendlier with version control.
* Generates pseudo unique IDs, to minimize conflicts when merging, but still
  user readable (so, not UUID).
* Eventually will also allow to have more precisely named sub-resources in
  imported files.
* This will allow better reloading on changes (including resources already
  loaded) as well as better keeping track of changes on the DCC.
* Keeps backward compatibility with the old formats.
* Binary and text format version incremented to mark breakage in forward
  compatibility.
@akien-mga
Copy link
Member

Fixed issues pointed out by reviewers, and bumped the FORMAT_VERSION for both binary and text formats to mark the forward compatibility breakage.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good to me. Let's get it merged and further tested in master.

@reduz
Copy link
Member Author

reduz commented Jul 22, 2021

Changes look good!

@akien-mga akien-mga merged commit c2c82de into godotengine:master Jul 22, 2021
@akien-mga
Copy link
Member

Thanks!

@Wavesonics
Copy link
Contributor

Man this is a really big deal! Another step toward being able to use Godot in larger teams/projects!

@knightofiam
Copy link

Can this be backported into 3.4? 😁

@Calinou
Copy link
Member

Calinou commented Aug 19, 2021

Can this be backported into 3.4? 😁

See #50693. If you want to help get it merged, test it locally on your project and report the results back on that PR 🙂

See #50693 (comment) for Windows builds of the above PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants