Open lyuma opened 2 years ago
Related to https://github.com/godotengine/godot/issues/45296 and https://github.com/godotengine/godot/issues/54679.
Making CowData use 64-bit unsigned integers is welcome, but it's a significant undertaking as it requires replacing the type in all places where relevant.
Is this still reproducible in the latest betas?
Still can happen on 4.0.0 stable.
MRP that just generates a few GB of random mesh data and saves it:
extends Node
func _ready():
const size = 50000000
var mesh = ArrayMesh.new()
var arrays = []
var vertices = PackedVector3Array()
vertices.resize(size)
for j in range(vertices.size()):
vertices[j] = Vector3(randf(), randf(), randf())
var uvs = PackedVector2Array()
uvs.resize(size)
for j in range(uvs.size()):
uvs[j] = Vector2(randf(), randf())
var uvs2 = PackedVector2Array()
uvs2.resize(size)
for j in range(uvs2.size()):
uvs2[j] = Vector2(randf(), randf())
var colors = PackedColorArray()
colors.resize(size)
for j in range(colors.size()):
colors[j] = Color(randf(), randf(), randf(), randf())
arrays.resize(ArrayMesh.ARRAY_MAX)
arrays[ArrayMesh.ARRAY_VERTEX] = vertices
arrays[ArrayMesh.ARRAY_TEX_UV] = uvs
arrays[ArrayMesh.ARRAY_TEX_UV2] = uvs2
arrays[ArrayMesh.ARRAY_COLOR] = colors
mesh.add_surface_from_arrays(Mesh.PRIMITIVE_TRIANGLES, arrays);
ResourceSaver.save(mesh, "res://test.res", ResourceSaver.FLAG_COMPRESS)
Also since I didn't see it in any of the linked issues: The MRP for just any of the CowData crashes is just [].resize(100000000)
ERROR: Condition "p_size < 0" is true. Returning: ERR_INVALID_PARAMETER at: resize (./core/templates/cowdata.h:262)
when saving big map from glb to SCN. TSCN saving normally.
Godot 4.0 Release.
I can confirm this on 4.1.dev 33957aee6 (Linux) using this MRP. Note that while I see this error message:
ERROR: Condition "p_size < 0" is true. Returning: ERR_INVALID_PARAMETER
at: resize (./core/templates/cowdata.h:262)
I don't get a crash though.
This is now causing a big problem for us and all users of Terrain3D. It does cause Godot to crash and does not save data. 4.1.1-stable, Windows 11/64.
We can cause it by importing 2 8k maps into our 16k world space and using the ResourceSaver to save. Godot can handle a full 16k map in memory, but it only takes half, 8k by 16k to crash Godot when saving. The crash occurs when we call ResourceSaver.save() or by clicking the arrow in the inspector on the .res and selecting save.
The resource is tied to a binary .res file in the inspector. Pre-crash it specifically has:
So call it 1.4gb of memory. Upon saving godot crashes with:
ERROR: Condition "p_size < 0" is true. Returning: ERR_INVALID_PARAMETER
at: resize (./core/templates/cowdata.h:262)
@akien-mga The new PR didn't fix this issue. It may have fixed part of it, but I attempted to save a 1.2gb resource file and saved it and it crashed.
I just built Godot 0bcc0e92b3f0ac57d4c4650722f347593a258572 with godot-cpp https://github.com/godotengine/godot-cpp/commit/0145e900f335fa3691564bb51080125b51c38c52 (though it's not relevant). I loaded up a terrain in 8k x 8k segments. First segment saved at 413mb fine. Two segments at 800mb. Three segments crashed.
This has nothing to do with our code. Our code loaded the data into a resource, then I manually used the inspector, right-click, and save the resource as a binary .res (contents listed above, defaults to compressed).
Crash without any console error messages. Debugging, I found it crashed in https://github.com/godotengine/godot/blob/master/core/io/file_access_compressed.cpp#L344-L350
PR #77306 was intended to be part of the solution for this issue. When I apply that PR on this commit Godot doesn't crash. However it doesn't save the resource either. It writes 11.7mb and has a popup,
By modifying the MRP at https://github.com/godotengine/godot/issues/62585#issuecomment-1450900291 to save roughly 2.8 GB through ResourceSaver
on a dev build from https://github.com/godotengine/godot/commit/0bcc0e92b3f0ac57d4c4650722f347593a258572 I'm able to reproduce a crash at the same line as @TokisanGames.
Reopening issue.
Likely caused by https://github.com/godotengine/godot/blob/master/core/io/file_access_compressed.cpp#L49 in
write_buffer_size = next_power_of_2(write_max);
since next_power_of_2
returns a uint32.
f317cc713aa4dbcee2efa10db764473a56680be7
I tested converting next_power_of_2 to uint64_t, but it's not enough.
This is needed:
file_access_compressed.h
uint32_t write_buffer_size = 0;
uint64_t write_buffer_size = 0;
In store_8, I expanded the WRITE(1) macro and replaced next_power_of_2. The last working power of 2 is 1<<31
. So once 2GB is full, the old algorithm starts allocating a 4gb block, 1<<32. Below changes this to allocate in 512mb blocks, which had no different result to allocating 4gb next.
void FileAccessCompressed::store_8(uint8_t p_dest) {
ERR_FAIL_COND_MSG(f.is_null(), "File must be opened before use.");
ERR_FAIL_COND_MSG(!writing, "File has not been opened in write mode.");
if (write_pos + (1) > write_max) {
write_max = write_pos + (1);
}
if (write_max > write_buffer_size) {
//write_buffer_size = next_power_of_2(write_max);
uint64_t value = 1;
while (value <= write_max) {
value = value << 1;
}
write_buffer_size += MIN(value, 1<<29);
buffer.resize(write_buffer_size);
write_ptr = buffer.ptrw();
}
write_ptr[write_pos++] = p_dest;
}
These changes get this function working beyond 2gb and no longer crashes, however it never returns from the save. The engine is hung. The msvc debugger seems to think it's running. CPU usage is minimal. Temporary files exist, but files are 0.
I tested importing terrain maps and saving to a resource file:
@lyuma this may be fixed with the cow size increase to 64 bit
Godot version
4.0.alpha c41e4b1
System information
Windows 10, 64-bit
Issue description
I ran into this when saving a large scn with lots of textures, and used "Make Local" so all textures got embedded.
I tried saving this scene, and it gave an error in resize
After this error, it proceeded to crash.
CowData resizes to the next power of two, which is 2147483648, and overflows a signed int.
I think everything here should be made less sloppy. Array operations should always use unsigned, not signed values. Integers should be 64-bit where possible: GDScript already uses 64-bit, so why are they downgraded to 32-bit in C++? Finally, FileAccess should check the return value of
resize()
and fail gracefully.Steps to reproduce
Sorry I don't have more specific steps. The asset I used is not redistributable, and the save was automated by a script in order to enable ZSTD compression.
Minimal reproduction project
N/A