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] Convert TSCN files into binary .scn on export by default #51096

Open
wants to merge 1 commit into
base: 3.x
Choose a base branch
from

Conversation

Listwon
Copy link
Contributor

@Listwon Listwon commented Jul 31, 2021

Fixes godotengine/godot-docs#5131 (.tscn scenes are properly compiled into compressed .scn)
Possible fix for #42115

@YuriSizov
Copy link
Contributor

YuriSizov commented Jul 31, 2021

The setting still needs to be respected. The only change here should be the introduction of compression, possibly to all types of resources, not just scenes. Well, I guess the extension can also be changed for scenes, if you are really pedantic about it, but it’s not necessary.

Also, all PRs must be made against master first and foremost, unless there is a very special need to fix something in a 3.x branch. Changes can then be backported to 3.x anyway, either via a cherrypick (if compatible) or a dedicated PR (like this one).

@Listwon
Copy link
Contributor Author

Listwon commented Jul 31, 2021

The plugin name EditorExportTextSceneToBinaryPlugin is misleading by itself as it doesn't mention text resources (in general) but only the scenes so maybe that should be changed as well?

Also, all PRs must be made against master first and foremost, unless there is a very special need to fix something in a 3.x branch. Changes can then be backported to 3.x anyway, either via a cherrypick (if compatible) or a dedicated PR (like this one).

I will prepare separate PR for master, but I'm working on a huge project on 3.x where I can check this better and mentioned bugs related to 3.x, that's why I reveresed the priority. I don't see prepared test project for 4.0 so i'll have to make one.

@Listwon Listwon changed the title Compile TSCN files into binary .scn on export [3.x] Compile TSCN files into binary .scn on export Jul 31, 2021
@Listwon Listwon force-pushed the compile-tscn-files branch from fee3125 to 75a1da4 Compare July 31, 2021 10:44
@YuriSizov
Copy link
Contributor

YuriSizov commented Jul 31, 2021

The plugin name EditorExportTextSceneToBinaryPlugin is misleading by itself as it doesn't mention text resources (in general) but only the scenes so maybe that should be changed as well?

If all kinds of resources are piped through it, possibly it can be renamed. Though this can be left for a master PR for compatibility reasons.

@Listwon
Copy link
Contributor Author

Listwon commented Jul 31, 2021

If all kinds of resources are piped through it, possibly.l it can be renamed. Though this can be left for a master PR for compatibility reasons.

Maybe I will leave it here and change in separate PR, but it looks like they are. At least this is the only place (*two places in one plugin) where GLOBAL_DEF("editor/convert_text_resources_to_binary_on_export") is respected.

@@ -1776,19 +1776,46 @@ void EditorExportTextSceneToBinaryPlugin::_export_file(const String &p_path, con
if (!convert) {
return;
}
String tmp_path = EditorSettings::get_singleton()->get_cache_dir().plus_file("tmpfile.res");
Error err = ResourceFormatLoaderText::convert_file_to_binary(p_path, tmp_path);
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is desired, I think compression should be added to this ResourceFormatLoaderText::convert_file_to_binary method.

With your change, every converted scene has to be loaded and saved but the commit message for 2514338, which added ResourceFormatLoaderText::convert_file_to_binary, mentions that "This method works by directly converting text to binary, so the scene does not need to be loaded and saved". There was probably a reason by @reduz to do it this way (I guess it's faster?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"This method works by directly converting text to binary, so the scene does not need to be loaded and saved" - internally it is loaded and after conversion saved as binary tmpfile.res and then we are loading it with FileAccess::get_file_as_array(). I'm not sure if it's a good approach to duplicate functionality instead of reusing ResourceSaver::save() (which is used by editor when we save the scene or other resources).

@fire
Copy link
Member

fire commented Jul 31, 2021

A brief look at the design on export tscns to final project. Here are the trade-offs, and comments on the choices we can make when exporting.

  1. text should not be default because it's big
  2. binary (un-compressed) should be the default because steam or itch will also do compression
  3. binary (zstd compressed) should be an option because sometimes the game is not shipped on a store
  4. .pck vs zip is unrelated

@xix-xeaon
Copy link

Compressed binary should be the default. Even if the transfer is compressed by Steam etc, there's no reason the game should take up more space on disk (even after extracting a zip for instance). Especially with zstd reading compressed data (and decompressing it) is faster than reading not compressed data.

Also, it would be really interesting if someone looked into using the zstd training mode to create a dictionary during export - that should significantly improve compression of these types of files.

@Listwon Listwon force-pushed the compile-tscn-files branch 2 times, most recently from 44722bd to 96ffd83 Compare August 20, 2021 11:35
@Listwon
Copy link
Contributor Author

Listwon commented Aug 20, 2021

Regarding compression - now it's respecting filesystem/on_save/compress_binary_resources.
I agree that binary should be the default (as it is in most engines, I think that in major ones you don't have any choice), so I changed the default value of editor/convert_text_resources_to_binary_on_export to true.

@Listwon Listwon changed the title [3.x] Compile TSCN files into binary .scn on export [3.x] Convert TSCN files into binary .scn on export by default Aug 20, 2021
@TokisanGames
Copy link
Contributor

@Listwon

so I changed the default value of editor/convert_text_resources_to_binary_on_export to true.

Maybe you should add this note to the top comment. It is a backport of #33228.

What else does this PR need to make it into 3.x? Compression is great, but I'm especially interested in binary everything on export by default.

Thanks for the effort.

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.

7 participants