NavigationRegion2D doesn't initialize its `navigation_mesh` correctly resulting in an error with a `cell_size` mismatch

mrTag commented 11 months ago

Godot version

4.2 beta1

System information

Windows 10

Issue description

I was trying out the new 2D Navigation mesh baking features (great stuff, btw!) and immediately ran into an error after creating a NavigationRegion2D and a corresponding NavigationPolygon (all in the editor/inspector) :

modules/navigation/nav_region.cpp:128 - Navigation map synchronization error. Attempted to update a navigation region with a navigation mesh that uses a `cell_size` of 1 while assigned to a navigation map set to a `cell_size` of 0.25. The cell size for navigation maps can be changed by using the NavigationServer map_set_cell_size() function. The cell size for default navigation maps can also be changed in the ProjectSettings.

I haven't changed any of the cell_size settings anywhere and I can't even set the cell_size of the NavigationPolygon to 0.25, since the inspector for that is integer based. So I had a look at the code and the culprit seems to be in the constructor of the NavigationPolygon:

NavigationPolygon::NavigationPolygon() {
    navigation_mesh.instantiate();
}

The navigation_mesh is instantiated with its default values (cell_size being 0.25!) and the real initialization, where the cell_size is set, is then not executed, because navigation_mesh is not null:

Ref<NavigationMesh> NavigationPolygon::get_navigation_mesh() {
    MutexLock lock(navigation_mesh_generation);

    if (navigation_mesh.is_null()) {
        navigation_mesh.instantiate();
        Vector<Vector3> verts;
        {
            verts.resize(get_vertices().size());
            Vector3 *w = verts.ptrw();

            const Vector2 *r = get_vertices().ptr();

            for (int i(0); i < get_vertices().size(); i++) {
                w[i] = Vector3(r[i].x, 0.0, r[i].y);
            }
        }
        navigation_mesh->set_vertices(verts);

        for (int i(0); i < get_polygon_count(); i++) {
            navigation_mesh->add_polygon(get_polygon(i));
        }
        navigation_mesh->set_cell_size(cell_size); // Needed to not fail the cell size check on the server
    }

    return navigation_mesh;
}

So I removed the navigation_mesh instantiation from the NavigationPolygon constructor and went on with my testing without any further hitch. As far as I know navigation_mesh is only accessed through get_navigation_mesh(), where it is then instantiated lazily, so the instantiation in the constructor is not needed. I implemented and tested a whole navigation system for our game based on the NavigationRegion2D after that change and haven't noticed any problems (but I am only creating the navigation polygon at runtime via NavigationServer2D.bake_from_source_geometry_data_async, so maybe there are problems in other use cases 🤔).

I just drafted a PR: https://github.com/godotengine/godot/pull/83568

Steps to reproduce

Use the Godot 4.2 beta1 editor
Create a new project (or use an existing one)
Create a NavigationRegion2D node
In the inspector of that node create a "New NavigationPolygon"
Observe the "Navigation map synchronization error" in the Output tab

Minimal reproduction project

A reproduction project isn't really necessary, since the steps are so simple, but here is one anyway: NavigationRegion2DTest.zip (when you open the scene.tscn, you'll get the error as well)

smix8 commented 11 months ago

Thanks for the detailed report.

According to the error msg it is the other way around.

The navigation mesh has the correct cell size in that error. It is the navigation map that has the incorrect cell size.

By default both "2D" navigation map and NavigationPolygon have a cell size of 1.0, or at least they should have.

mrTag commented 11 months ago

I didn't even notice that in the error message. But it didn't line up with my experience with the code and so I had a look at the code of the error message: https://github.com/godotengine/godot/blob/7f884b4e0017368e193d96f425aac6c2d8a86eb0/modules/navigation/nav_region.cpp#L128 and indeed: the two objects are the wrong way around in the format message! 🤦 The mesh cell size should be the first argument and the map cell size the second one.

godotengine / godot