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

Use compatible text resource format when possible #90889

Merged
merged 1 commit into from
Apr 23, 2024

Conversation

timothyqiu
Copy link
Member

@timothyqiu timothyqiu commented Apr 19, 2024

Use the new format only when saving a resource / scene containing a PackedByteArray with at least 64 elements.

This way, editor settings can be kept, and users can still open most scenes with previous versions of Godot.

This PR is a proof of concept of my suggestion on RocketChat.

Bugsquad edit: Fixes #90846

@timothyqiu timothyqiu requested a review from a team as a code owner April 19, 2024 08:17
@AThousandShips AThousandShips added this to the 4.3 milestone Apr 19, 2024
@timothyqiu timothyqiu requested a review from a team as a code owner April 19, 2024 08:49
@akien-mga akien-mga requested review from KoBeWi and groud April 19, 2024 08:51
@KoBeWi
Copy link
Member

KoBeWi commented Apr 19, 2024

This makes TileMapLayer save in the old PackedByteArray format, which is wrong.
Tileset does not use PackedByteArray AFAIK.

@timothyqiu
Copy link
Member Author

This makes TileMapLayer save in the old PackedByteArray format, which is wrong.

If you mean the change in TileMapLayer.xml, it's because only resource files are possible to use the new format. In this case, serializing XML still uses the old format indeed.

@KoBeWi
Copy link
Member

KoBeWi commented Apr 19, 2024

No I mean I tested with a TileMapLayer in a scene. The whole format change was because of TileMapLayer.

@timothyqiu
Copy link
Member Author

Ah, sorry I missed the recursion part when in VariantWriter. I'll get rid of the setup for TileSet and push again.

@timothyqiu
Copy link
Member Author

It should work correctly now.

prefer_compat_format() is now PackedScene only.

@KoBeWi
Copy link
Member

KoBeWi commented Apr 19, 2024

This works correctly and even resolves Resources issue, making my PR irrelevant.

But it's hacky. And also it makes Base64 hard-coded only for TileMapLayer, which means nothing else can benefit from it. I think perfectly scenes/resources should always save under new format if they contain PackedByteArray.

@timothyqiu timothyqiu force-pushed the compat branch 2 times, most recently from e73358a to c46b038 Compare April 19, 2024 14:02
@timothyqiu
Copy link
Member Author

It turns out ResourceFormatSaverTextInstance::_find_resources() goes through every value to be stored. It's a good place to check whether PackedByteArray is used.

Now the resource saver will use the new format if there's a PackedByteArray with at least 64 elements. The hard-coded TileMapLayer check is removed.

The downside is that the format version may change depending on what's inside the resource / scene. Maybe the array size limit is an overthinking, a non-empty check is enough.

@KoBeWi
Copy link
Member

KoBeWi commented Apr 19, 2024

Yeah the size check is unecessary. This also means that we have to keep the old PackedByteArray serialization code.
PackedByteArrays are uncommon in scenes/resources and if they exist, they are usually longer. I checked with TileMapLayer and the size is exceeded with just 6 tiles, so it's pointless.

@timothyqiu
Copy link
Member Author

The main reason I added the size check is for potential custom nodes / resources that export PackedByteArray variables.

If the array only contains a few elements, using Base64 won't save much space and is not human readable. Seems not worth breaking compat for.

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

It's still somewhat hacky, but at least it's no longer hard-coded to one class. It's a nice solution.

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.

That approach looks fine to me overall.

I discussed it with @reduz, and we agreed to merge this for now to address the immediate issue, and then:

  • Backport support for parsing the new base64 PackedByteArray to 4.1.5 and 4.2.3 (but not saving - so keeping version format 3 there, but being able to parse version 4).
  • Eventually consider reverting this compat handling once the currently supported stable versions no longer need it.

@akien-mga akien-mga merged commit ff9d78c into godotengine:master Apr 23, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

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.

Editor settings wiped out when going from latest 4.3.dev to older releases
4 participants