godotengine / godot

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

GridMap only supports 16bit IDs while MeshLibrary supports 32bit IDs #43836

Open wd40bomber7 opened 3 years ago

wd40bomber7 commented 3 years ago

Godot version: v3.2.3.stable.mono.official

OS/device including version: Microsoft Windows 19041.630

Issue description: I am working on a very simple game and decided I wanted to use gridmap to paint the terrain. I wrote a tiny bit of C# script to take a list of textures and build simple 3d mesh tiles using them to create my mesh library. For the MeshLibrary IDs I decided to just hash the texture names so the IDs would stay consistent every time I regenerated the MeshLibrary. This is the important bit because I generated those ids like this: Math.Abs(itemName.GetHashCode())

This generated entirely valid MeshLibrary entries: image

However when I went to actually draw my grid tiles nothing happened (looking like this): GridMapBroken

The problem is this code from gridmap.cpp:

void GridMap::set_cell_item(const Vector3i &p_position, int p_item, int p_rot) {
   // ... Lots of unrelated stuff ...
    Cell c;
    c.item = p_item;
    c.rot = p_rot;

    cell_map[key] = c;
}

Cell is defined like this:

    union Cell {
        struct {
            unsigned int item : 16;
            unsigned int rot : 5;
            unsigned int layer : 8;
        };
        uint32_t cell;
                // Constructor omitted...
    };

And there's the problem. Item is bitmapped to only the first 16 bits. This means that the latter 16 bits are just chopped right off without any error or warning! The shorter and completely wrong ID is then stored in the gridmap.

In summary:

This issue was very frustrating since GridMap doesn't even bother to put out a warning on invalid IDs. I suggest adding a warning (at least to the editor). As for actually fixing the issue I'm not sure what would be best, maybe switch MeshLibrary to use a short so this specific mistake becomes impossible? Also set_cell_item should absolutely take a short.

Minimal reproduction project: GridMapKeyLimitation.zip

ATHellboy commented 1 year ago

Any plans on supporting 32bit IDs for GridMap ?