godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
88.12k stars 19.94k forks source link

Geometry.make_atlas() fatal fails with large atlas sizes in x-direction #93795

Open SachsKaylee opened 1 month ago

SachsKaylee commented 1 month ago

Tested versions

Reproducibe in:

System information

Godot v4.3.beta2.mono - Windows 10.0.22631 - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 4080 SUPER (NVIDIA; 32.0.15.5612) - AMD Ryzen 7 5700X 8-Core Processor (16 Threads)

Issue description

Calling Geometry2d.make_atlas (https://docs.godotengine.org/en/stable/classes/class_geometry2d.html#class-geometry2d-method-make-atlas) with an array in which one size has a large x value causes the game to crash with a fatal error that cannot be caught (in C# raises an SEHException and crashes despite catch, GDScript just exits the game). Interestinly enough, the same does not appear with a large y-value. I was not able to pinpoint the exact value at which the crash occurs. It seems to be between 4000 and 5000. See this forum post for possible discussion: https://forum.godotengine.org/t/geometry2d-makeatlas-throws-sehexception/69237

Steps to reproduce

extends Node

func _ready() -> void:
  # No crash
  var small_atlas = Geometry2D.make_atlas([Vector2(10, 10)])
  print(small_atlas)

  # No crash
  var large_y_atlas = Geometry2D.make_atlas([Vector2(10, 5000)])
  print(large_y_atlas)

  # Crash
  var large_x_atlas = Geometry2D.make_atlas([Vector2(5000, 10)])
  print(large_x_atlas)

Godot Log in latest beta version (stable is the same without the crash handler):

Godot Engine v4.3.beta2.mono.official.b75f0485b - https://godotengine.org
WARNING: Project setting "rendering/limits/global_shader_variables/buffer_size" exceeds maximum uniform buffer size of:.
     at: MaterialStorage (drivers/gles3/storage/material_storage.cpp:1115)
OpenGL API 3.3.0 NVIDIA 556.12 - Compatibility - Using Device: NVIDIA - NVIDIA GeForce RTX 4080 SUPER

Editing project: S:/Projects/AtlasBug
Godot Engine v4.3.beta2.mono.official.b75f0485b - https://godotengine.org
Vulkan 1.3.278 - Forward+ - Using Device #0: NVIDIA - NVIDIA GeForce RTX 4080 SUPER

Godot Engine v4.3.beta2.mono.official.b75f0485b - https://godotengine.org
Vulkan 1.3.278 - Forward+ - Using Device #0: NVIDIA - NVIDIA GeForce RTX 4080 SUPER

{ "points": [(0, 0)], "size": (10, 10) }
{ "points": [(0, 0)], "size": (10, 5000) }
ERROR: FATAL: Index p_index = -1 is out of bounds (size() = 0).
   at: get (./core/templates/cowdata.h:205)

================================================================
CrashHandlerException: Program crashed with signal 4
Engine version: Godot Engine v4.3.beta2.mono.official (b75f0485ba15951b87f1d9a2d8dd0fcd55e178e4)
Dumping the backtrace. Please include this when reporting the bug to the project developer.
[1] error(-1): no debug info in PE/COFF executable
[2] error(-1): no debug info in PE/COFF executable
[3] error(-1): no debug info in PE/COFF executable
[4] error(-1): no debug info in PE/COFF executable
[5] error(-1): no debug info in PE/COFF executable
[6] error(-1): no debug info in PE/COFF executable
[7] error(-1): no debug info in PE/COFF executable
[8] error(-1): no debug info in PE/COFF executable
[9] error(-1): no debug info in PE/COFF executable
[10] error(-1): no debug info in PE/COFF executable
[11] error(-1): no debug info in PE/COFF executable
[12] error(-1): no debug info in PE/COFF executable
[13] error(-1): no debug info in PE/COFF executable
[14] error(-1): no debug info in PE/COFF executable
[15] error(-1): no debug info in PE/COFF executable
[16] error(-1): no debug info in PE/COFF executable
[17] error(-1): no debug info in PE/COFF executable
[18] error(-1): no debug info in PE/COFF executable
[19] error(-1): no debug info in PE/COFF executable
[20] error(-1): no debug info in PE/COFF executable
[21] error(-1): no debug info in PE/COFF executable
-- END OF BACKTRACE --

Minimal reproduction project (MRP)

MRP for 4.3-beta2 (created with the Mono version, but the sample is in GDScript)

AtlasBug.zip

matheusmdx commented 1 month ago

I took an look in the code and the problem here:

https://github.com/godotengine/godot/blob/4ab8fb809396fa38ba929fec97cfcb7193f1c44d/core/math/geometry_2d.cpp#L109-L119

widest will be the biggest Vector2.x value inside the PackedVector2Array passed to Geometry2D.make_atlas(), using the mrp, this value will be 5000. Inside the for loop, in line 117 we check if w is smaller than widest, w values in this loop will be 1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048 and finally 4096, as you can see any value bigger than 4095 will skip the entire loop.


https://github.com/godotengine/godot/blob/4ab8fb809396fa38ba929fec97cfcb7193f1c44d/core/math/geometry_2d.cpp#L174-L193

Now the code try to find the best aspect ratio for the atlas but that use the size of results Vector for that and results is filled inside the for loop that was entired skipped before, so will be empty. As consequence best will stay as -1 and will cause the crash in line 192 because will be used as an index.


The solution is add a error check here looking if the values of the Vector2.x is not bigger than 4095

https://github.com/godotengine/godot/blob/4ab8fb809396fa38ba929fec97cfcb7193f1c44d/core/math/geometry_2d.cpp#L87-L100


Also, shouldn't the y value be checked as well? And the final size shouldn't also be checked if don't pass the limits before return?

SachsKaylee commented 1 month ago

Are there any chances of allowing atlases larger than 4096x4096? We use a per-tile resolution of 256x256, and dynamically bake the sprite sheet at runtime from various PNG files, etc. Some of those textures are meant to occupy multiple tiles and therefore sometimes can have resolutions of e.g. 1024x5120, exceeding the maximum size of 4096.

matheusmdx commented 1 month ago

Are there any chances of allowing atlases larger than 4096x4096?

I don't know for sure, godot docs says max image size is 16384×16384 but i don't know if that limit apply for this case too, maybe use a lower value because mobile gpus. Anyways, will be good update the function docs to tell the max values allowed and add checks to avoid a crash in cases that use invalid values, also if bigger values can be used in some platforms we can raise the limit and add a warning in docs and in editor saying some platforms can't support that size.

SachsKaylee commented 1 month ago

Thats fair enough - a check and mention in the documentation is definetly needed. We are using Forward+ rendering for our game and have made the design decision to not support mobile platforms. We are also fine with not having compatability with very old hardware that only supports low-res textures. - So it would be great to have that limit increased on Forward+ rendering or behind a config option. :)