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

[3.x] Implement textual ext/subresource IDs. #50693

Closed
wants to merge 1 commit into from

Conversation

akien-mga
Copy link
Member

@akien-mga akien-mga commented Jul 21, 2021

  • 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 compatibility with the old formats.

Backport of #50676.


IMPORTANT: Needs extensive testing on safe project backups. If there's a bug in this PR, it can break scene files and lead to data loss. So make copies of your projects (or better, use VCS) when testing this PR. But please do test it on your projects to make sure that it works as expected :)

How to test: See Testing pull requests for instructions on how to download CI builds to test locally.

@akien-mga akien-mga added this to the 3.4 milestone Jul 21, 2021
@akien-mga akien-mga requested a review from a team as a code owner July 21, 2021 11:02
@akien-mga akien-mga changed the title Implement textual ext/subresource IDs. [3.x] Implement textual ext/subresource IDs. Jul 21, 2021
@akien-mga akien-mga force-pushed the 3.x-backport-50676 branch from 0c3325c to 8c9bb2e Compare July 21, 2021 14:34
@steffendietz
Copy link

Just wanted to provide feedback regarding this PR:

  • I built the PR branch from source
  • before every test I git reset --hard my test project to observe changes

Test1

  • Opened the test project -> git status showed no changes
  • Played the project (F5) -> assets/theme.tres, default_env.tres and scenes/PlayingField.tscn (main scene) were updated to the new format

Test2

  • Opened the test project -> git status showed no changes
  • Closed the initially opened main scene (scenes/PlayingField.tscn)
  • Played the project (F5) -> only default_env.tres was updated to the new format

Test3

  • Opened the test project -> git status showed no changes
  • Closed the initially opened main scene (scenes/PlayingField.tscn) and opened another scene (scenes/Pedestal.tscn)
  • Played the project (F5) -> default_env.tres and scenes/Pedestal.tscn were updated to the new format

Observations

The project ran without problems.
The updates to the new format only took place when running the project, not when merely opening the scenes in the editor.

@akien-mga akien-mga force-pushed the 3.x-backport-50676 branch from 8c9bb2e to 4c8e11e Compare July 23, 2021 08:39
@akien-mga
Copy link
Member Author

Updated to match the latest state from #50676.

The updates to the new format only took place when running the project, not when merely opening the scenes in the editor.

That's expected indeed, changes only happen when the scenes or resources are saved. Some are saved automatically when you run the project, which is why you saw them changing.

@Calinou
Copy link
Member

Calinou commented Aug 19, 2021

Here are Windows x86_64 builds of this pull request for testing:

  • Editor: https://0x0.st/-yty.zip (link expires in October 2021)
    • The binary is larger than usual (218 MB). It contains full debugging symbols, which is required to get accurate backtraces if Godot crashes. It's also less optimized than official binaries.

I would recommend testing this PR with an exported project as well. (The project must be exported with an editor build that includes this pull request.)

@akien-mga akien-mga closed this Oct 4, 2021
@akien-mga akien-mga deleted the 3.x-backport-50676 branch October 4, 2021 12:58
@akien-mga akien-mga restored the 3.x-backport-50676 branch October 4, 2021 12:58
@akien-mga
Copy link
Member Author

Closed by mistake.

@akien-mga akien-mga reopened this Oct 4, 2021
@akien-mga akien-mga modified the milestones: 3.4, 3.5 Oct 4, 2021
* 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.

(cherry picked from commit 75755be)
@Calinou
Copy link
Member

Calinou commented Nov 8, 2021

If this is to be merged for 3.5, beware of #54774.

@akien-mga akien-mga force-pushed the 3.x branch 2 times, most recently from 71cb8d3 to c58391c Compare January 6, 2022 22:40
knightofiam added a commit to forerunnergames/coa that referenced this pull request Jun 8, 2022
Redo tree tilemaps & colliders in Godot editor after integrating new
modularized player animation system. This is necessary because of
conflicts in the scene file when merging branches - there is as of yet
no coherent way to deal with internal resource id conflicts.

For more information about scene file merging issues in Godot, see:

godotengine/godot-proposals#1281
godotengine/godot#50693
@theludovyc
Copy link
Contributor

Hello 😊 ! How can I help to close this one ? :)

@Calinou
Copy link
Member

Calinou commented Jun 14, 2022

Hello blush ! How can I help to close this one ? :)

3.5 is too close to release for this pull request to be merged by now.

This PR could still be considered for 3.6, but this is a fairly risky change to undertake. We also can't revert this change once it is made in a released beta/RC build, as the new UID-based format is incompatible with non-UID-aware versions of Godot.

For this PR to be merged in a 3.6 beta, there needs to be strong community demand and people available to test this change on their real world projects (after making backups, of course 🙂).

Also, this PR will need be rebased and have UID fixes that were applied in master cherry-picked before this can be merged. (I can't find the PRs for those fixes right now, but I remember seeing a few.)

@akien-mga akien-mga modified the milestones: 3.5, 3.x Jun 14, 2022
@theludovyc
Copy link
Contributor

This PR could still be considered for 3.6, but this is a fairly risky change to undertake.

Why ?

For this PR to be merged in a 3.6 beta, there needs to be strong community demand and people available to test this change on their real world projects (after making backups, of course 🙂).

Got it ;) !

@Calinou
Copy link
Member

Calinou commented Jun 15, 2022

Why ?

This PR changes how scene files are saved, which has a risk of data loss if there are bugs in the implementation. Some people still aren't using version control in their projects, and while Godot's license doesn't provide any warranty, we should still avoid playing with fire when possible.

@akien-mga
Copy link
Member Author

After thinking about this some more, I think it's not worth backporting to 3.x anymore.

We're about to release 3.5 and this is of course too late to merge such a change. It could be merged for 3.6, but there are various issues that this would raise:

  • It creates forward compatibility issues for earlier Godot releases, as scenes opened in a hypothetical 3.6 release with this change would no longer be compatible with 3.5 and earlier. It's fairly common (for me at least) to open projects in older releases when checking if a new bug is an engine regression or not (i.e. did it work fine in 3.4? In 3.5 beta1? In 3.5 RC2? etc.).
  • There are still unresolved bugs with this implementation in master (e.g. Duplicated files or resources retains the same UID #54774), so it's not ready for the 3.x branch unless we can really commit to fixing those bugs in a timely manner. Some bugs might not have been well described yet because master is still in alpha so bugs with the UID system can be hidden by other bugs that are on top of it.
  • It's a nice-to-have, but not critical for making games with 3.x and changing the workflow in what would likely be the last minor release in 3.x doesn't seem really useful.

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.

6 participants