-
-
Notifications
You must be signed in to change notification settings - Fork 21.5k
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
Fix Editor navigation mesh baking UndoRedo #91966
base: master
Are you sure you want to change the base?
Conversation
Fixes UndoRedo when baking a navigation mesh in the Editor so users can get back to a previous navigation mesh state if desired.
Ref<NavigationPolygon> navigation_mesh = node->get_navigation_polygon(); | ||
if (navigation_mesh.is_null()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this rename actually makes sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I do it all the time in other PRs to have as much code similarity between 2D and 3D as possible.
There will be a point where NavigationPolygon as a resource name will disappear because frankly, that name sucks soo much. It is a 2D navigation mesh, not a single polygon, and it constantly confuses users in documentation and discussions, because the "real" polygons are the polygons inside a navigation mesh.
Vector<Vector2> redo_vertices = navigation_mesh->get_vertices(); | ||
Vector<Vector<int>> redo_polygon_indices; | ||
int redo_polygon_count = navigation_mesh->get_polygon_count(); | ||
redo_polygon_indices.resize(redo_polygon_count); | ||
for (int i = 0; i < redo_polygon_count; i++) { | ||
redo_polygon_indices.write[i] = navigation_mesh->get_polygon(i); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this block is the same for undo/redo indices/vertices so perhaps we can extract it to helper function to reduce duplication and increase readability a bit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When #93171 gets merged that polygon loop block would become unnecessary as it can be replaced by a simple get_polygons()
function.
Vector<Vector3> redo_vertices = navigation_mesh->get_vertices(); | ||
Vector<Vector<int>> redo_polygon_indices; | ||
int redo_polygon_count = navigation_mesh->get_polygon_count(); | ||
redo_polygon_indices.resize(redo_polygon_count); | ||
for (int i = 0; i < redo_polygon_count; i++) { | ||
redo_polygon_indices.write[i] = navigation_mesh->get_polygon(i); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
Fixes
UndoRedo
when baking a navigation mesh in the Editor so users can get back to a previous navigation mesh state if desired.Along the way of all the navigation mesh baking related changes the UndoRedo was broken. The old implementation did entire full resource duplicates for the UndoRedo which also was not really something to go back to because it broke all the resource ref handles.
The sacrifice of bringing UndoRedo back for navigation mesh baking inside the editor is that the baking needs to happen again on the main thread. There are just too many unsolved issues and changes required to make UndoRedo work with an async process.