-
-
Notifications
You must be signed in to change notification settings - Fork 232
Option to use compressed color map #842
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
#825 has been merged. From now on the principle is any setter function that is called with the same value is silently ignored. Logs, signals, and updates are not triggered unless the value is different. Once you rebase, you can use SET_IF_DIFF(_member_var, p_new_param); which will return if they are the same or set it if different, for most values including native types, pointers, and Godot Dictionaries and Arrays (compared by internal pointer). Occasionally a bit more complexity is warranted so a few setters accomplish the same idea with different means. |
08bd30b to
2734d35
Compare
TokisanGames
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job so far with the code and the testing. Running the demo shows the right format change and VRAM reduction.
I have noticed one issue so far:
If compression mode is set and free_uncompressed_color_maps is false, it still frees the uncompressed colormap, when it shouldn't.
var region: Terrain3DRegion = $Terrain3D.data.get_region(Vector2i(0,0))
print("Color map: ", region.color_map)
print("Compressed color map: ", region.compressed_color_map)
color_compression = bptc
free_uncompressed_color_maps = false (same if true)
Color map: <Object#null>
Compressed color map: <Image#-9223371995985476204>
color_compression = none
free_uncompressed_color_maps = false (same if true)
Color map: <Image#-9223371996035807859>
Compressed color map: <Object#null>
You can also rebase and squash all of your commits.
|
Hello! I have now fixed the changes you requested. Let me know if anything should be changed. Just wondering, but regarding squashing would it not be easier for you to squash the commits through the "squash and merge" GitHub option? |
Great, thank you.
No, it needs to be squashed and rebased. Otherwise it will make a mess of the history. Github does one or other other, not both. And it doesn't rebase if there are conflicts, which is the case. There are currently conflicts and 25 commits. Please squash your commits down to two: code changes in one, and docs in the other. Then rebase onto the latest main and resolve any conflicts. The docs explain how, and I can help on discord. When I prepare it for merging I will tweak the code and docs, generate the rst files, and csharp bindings. It's helpful if I don't also have to squash, rebase, and fix conflicts myself. |
25c7dc8 to
b93c7d6
Compare
…runtime. Option to free the uncompressed color map during runtime and export when the compression mode is set.
b93c7d6 to
a19e599
Compare
TokisanGames
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking better, thank you. I have now taken over this PR. Comments are for your edification only.
Pending:
- Review Terrain3DRegion:
- free_uncompressed_color_map still frees when disabled.
- rework santisize_maps()
src/terrain_3d.cpp
Outdated
|
|
||
| void Terrain3D::set_color_compression_mode(const ColorCompressMode p_color_compression_mode) { | ||
| SET_IF_DIFF(_color_compression_mode, p_color_compression_mode); | ||
| switch (_color_compression_mode) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to convert this on the very rare get godot version call, then we don't need to store the extra variable.
src/terrain_3d.cpp
Outdated
| } | ||
| LOG(INFO, "Setting compression mode for color maps: ", _image_color_compression_mode); | ||
| TypedArray<Terrain3DRegion> regions = _data->get_regions_active(); | ||
| for (int i = 0; i < regions.size(); i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We now use ranged for loops
for (Ref<Terrain3DRegion> region : regions) {
region->set_modified(true);
}
|
|
||
|
|
||
| func _customize_scene(scene: Node, path: String) -> Node: | ||
| var _terrain: Terrain3D = scene.find_child("Terrain3D", true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_ is our convention for class private variables like _hash.
terrain is local, so does not use _.
| # Includes | ||
| const Terrain3DUI: Script = preload("res://addons/terrain_3d/src/ui.gd") | ||
| const RegionGizmo: Script = preload("res://addons/terrain_3d/src/region_gizmo.gd") | ||
| const ExportPlugin = preload("res://addons/terrain_3d/src/export_plugin.gd") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
: Script
src/terrain_3d_data.h
Outdated
| // File I/O | ||
| void save_directory(const String &p_dir); | ||
| void save_region(const Vector2i &p_region_loc, const String &p_dir, const bool p_16_bit = false); | ||
| void save_region(const Vector2i &p_region_loc, const String &p_dir, const bool p_16_bit = false, const Image::CompressMode p_color_compression_mode = Image::COMPRESS_MAX); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should use our color compression mode.
src/terrain_3d.h
Outdated
| SIZE_2048 = 2048, | ||
| }; | ||
|
|
||
| enum ColorCompressMode { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a property of the maps, so should be in Terrain3DRegion, along with the Image Format.
src/terrain_3d_data.cpp
Outdated
| // Generate all or only those marked edited | ||
| if (region && (p_all_regions || region->is_edited())) { | ||
| region->get_color_map()->generate_mipmaps(); | ||
| region->get_map(TYPE_COLOR)->generate_mipmaps(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this cannot be done on a compressed map. So, it must be done on the uncompressed if valid or not at all.
src/terrain_3d_region.h
Outdated
| void set_compressed_color_map(const Ref<Image> &p_map); | ||
| Ref<Image> get_compressed_color_map() const { return _compressed_color_map; } | ||
| void free_uncompressed_color_map(); | ||
| void sanitize_maps(const bool p_free_uncompressed_color_maps); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should have a default, rather than changing all usage cases of it.
| ClassDB::bind_method(D_METHOD("set_color_map", "map"), &Terrain3DRegion::set_color_map); | ||
| ClassDB::bind_method(D_METHOD("get_color_map"), &Terrain3DRegion::get_color_map); | ||
| ClassDB::bind_method(D_METHOD("sanitize_maps"), &Terrain3DRegion::sanitize_maps); | ||
| ClassDB::bind_method(D_METHOD("set_compressed_color_map", "map"), &Terrain3DRegion::set_compressed_color_map); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR doesn't currently sanitize() compressed color maps when set. I don't think we even need to offer the ability to set them to the user. I think we offer: get, clear, or generate. Then we don't need to sanitize, since the raw color map is already sanitized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, but I see we need this function in order to load the compressed color map from disk.
src/terrain_3d_region.cpp
Outdated
| _control_map = map; | ||
| if (p_free_uncompressed_color_maps) { | ||
| free_uncompressed_color_map(); | ||
| return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is the right function to clear the uncompressed maps. And returning here I'm pretty sure is a bug. The purpose of this function is to ensure the existing data is clean and safe to use, not to free data or optimize memory.
a19e599 to
113e392
Compare
src/terrain_3d_region.cpp
Outdated
| case TYPE_COLOR: | ||
| if (!IS_EDITOR && _compressed_color_map.is_valid()) { | ||
| return *_compressed_color_map; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't the right place for this either. The expectation should be if you ask for a color map you get the editable color map. We need a new function to get the active one which implies the one you get may be compressed and uneditable.
Other functions like set_maps(), get_maps(), get_data() don't even consider the compressed map. It needs to be all or nothing. I think it's cleaner to be nothing - so the compressed map is only created on save, freed in the editor, only accessed by get_compressed_color_map() or get_active_color_map(), everything else gives you the editable color map, though it may be null.
1214070 to
865865f
Compare
|
I have reorganized some of the code. Now:
Next, I will pull a commit from my dev mode PR to entirely disable the color map. Then I'll remove free_color_map and instead introduce a color map mode with these options:
Does this look like the full list of desired possibilities? |
a16d4e9 to
b80f011
Compare
|
Have exported projects to web/mobile been tested for this yet? Ommiting to test those has caught me out before. |
|
There might be an issue with compressed maps on linux. https://discord.com/channels/691957978680786944/1065519581013229578/1463464825316180061 |

Fixes #379
I have implemented an option to use compressed color maps when the game is running (i.e when not editing). When the compress option is used and the scene is saved, the regions will all get a compressed color map. When the game is run (either launched from editor or when exported), the compressed color map will be used and the uncompressed color map will be unloaded.
I have tested that the compressed texture is used instead of the normal one when the game is running with the option enabled by analyzing the frame in Renderdoc. The compressed color map is also updated after the terrain has been sculpted/painted in the editor.
With the compressed color map, the color map size is 4x smaller.
I have also added some documentation in the existing style.
One thing that I noticed is that when there is a large terrain with a lot of regions, the saving can take a long time (several minutes) when the option has been clicked, but this should only happen once. Maybe this time could be reduced if the compression happend on the entire color map at once instead of for the tile in each region.