Skip to content
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

[3.5] Fix broken TileSet Navigation due to wrong NavigationPolygon format #61266

Merged
merged 1 commit into from
May 31, 2022

Conversation

smix8
Copy link
Contributor

@smix8 smix8 commented May 22, 2022

This is targeting 3.5 to fix very old and persistent 2D navigation bugs in 3.x TileSets.
The entire tileeditor has a full rework in Godot 4.0 that, as far as I am aware, has not copied this issue over to Godot 4.0.

Since this pr would change how NavigationPolygon resources are created and stored by TileSets this would be a breaking change to old navigation tilesets but imo necessary for at least 3.5 with the new NavigationServer introduced.

TileSets created and stored NavigationPolyons in a format that did not work for Navigation or if fixed displayed the NavigationPolygon wrong in the TileSetEditor. This problem was not so noticeable on simple square grids but as soon as users would try to create more complex shapes around corners the navigation polygon would break and navigation would create ugly paths.

tilsetpolygon

I was able to get the TileSet navigation with not simple square shapes working again for the NavigationServer in 3.5 and also make it display correct in the Editor. I might have missed something as 2D and especially the TileEditor is not my forte and I could not figure out some parts e.g. the undo_redo part at FIXME that I commented out so help is welcome.

tilesetnavbug

Still, not everything is solved for 2D tileset navigation with this. There seems to be a lot of issues in the outdated 2D NavigationServer 3.5 backport related to 2D edge merging that were fixed by groud in Godot 4.0 e.g. I could barely get same edges to merge but not different, shorter edges while the same NavigationPolygon work perfectly in Godot 4.0 but this should be a different topic.

Fixes #30883
Fixes #41349
Fixes #45045
Fixes #43079
and a ton of other duplicates and 2D navigation issues that I randomly sighted but forgot to bookmark while digging through old 2D navigation issues.

@smix8
Copy link
Contributor Author

smix8 commented May 22, 2022

Note that without upgrading the 3.5 navigation backport with another backport of #47024 the navmesh edges will still not merge in many cases.

edited_navigation_shape->clear_outlines();
edited_navigation_shape->add_outline(polygon);
edited_navigation_shape->make_polygons_from_outlines();
// FIXME Couldn't figure out the undo_redo quagmire
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe @KoBeWi can help?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know the exact methods, but probably something like

undo_redo->create_action(TTR("Edit Navigation Polygon"));
undo_redo->add_do_method(edited_navigation_shape.ptr(), "clear_outlines");
undo_redo->add_do_method(edited_navigation_shape.ptr(), "add_outline", polygon);
undo_redo->add_do_method(edited_navigation_shape.ptr(), "make_polygons_from_outlines");
undo_redo->add_undo_method(edited_navigation_shape.ptr(), "clear_outlines");
for (int i = 0; i < edited_navigation_shape->get_polygon_count(); i++) {
    undo_redo->add_undo_method(edited_navigation_shape.ptr(), "add_polygon", edited_navigation_shape->get_polygon(i));
}
undo_redo->commit_action();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I tested this but it seems something is still missing. It does not go back to previous state on undo only visually deletes it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code looks similar to that one, maybe it can do something similar?

w.release();
shape->clear_outlines();
shape->add_outline(polygon);
shape->make_polygons_from_outlines();
undo_redo->create_action(TTR("Create Navigation Polygon"));
if (tileset->tile_get_tile_mode(get_current_tile()) == TileSet::AUTO_TILE || tileset->tile_get_tile_mode(get_current_tile()) == TileSet::ATLAS_TILE) {
undo_redo->add_do_method(tileset.ptr(), "autotile_set_navigation_polygon", get_current_tile(), shape, edited_shape_coord);
undo_redo->add_undo_method(tileset.ptr(), "autotile_set_navigation_polygon", get_current_tile(), tileset->autotile_get_navigation_polygon(get_current_tile(), edited_shape_coord), edited_shape_coord);
} else {
undo_redo->add_do_method(tileset.ptr(), "tile_set_navigation_polygon", get_current_tile(), shape);
undo_redo->add_undo_method(tileset.ptr(), "tile_set_navigation_polygon", get_current_tile(), tileset->tile_get_navigation_polygon(get_current_tile()));
}
tools[TOOL_SELECT]->set_pressed(true);
undo_redo->add_do_method(this, "_select_edited_shape_coord");
undo_redo->add_undo_method(this, "_select_edited_shape_coord");
undo_redo->commit_action();

Notably, it looks like the methods called on edited_navigation_shape don't need to be in the undo redo, it's edited_navigation_shape itself which is used in the do methods.

TileSets created and stored NavigationPolyons in a format that did not work for Navigation.
@smix8 smix8 force-pushed the navigation_tilemap_poly_3.x branch from 8ada72b to 1bac95b Compare May 24, 2022 17:07
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's merge as is to fix the bug and we can figure out the UndoRedo later.

@akien-mga akien-mga merged commit 0acd1ca into godotengine:3.x May 31, 2022
@akien-mga
Copy link
Member

Thanks!

@smix8 smix8 deleted the navigation_tilemap_poly_3.x branch June 1, 2022 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants