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

NavigationPolygon: Implement get/set_polygon fast paths. #93171

Merged
merged 1 commit into from
Aug 16, 2024

Conversation

libklein
Copy link
Contributor

@libklein libklein commented Jun 14, 2024

Follow up to PR #92955. This PR implements the changes outlined in #92955 (comment).

As a first step, it implements fast-path get/set_polygons and get/set_outlines functions, updating NavMeshGenerator2D accordingly. Implemented get/set_polygons for NavigationMesh as well, replacing add_polygon whenever a direct update was preferable.

@libklein libklein force-pushed the implement-set-get-polygons branch from fca467d to bc48650 Compare June 14, 2024 21:04
@libklein libklein requested a review from smix8 June 14, 2024 21:06
@libklein libklein force-pushed the implement-set-get-polygons branch 2 times, most recently from 788ad1e to 9420f1d Compare June 14, 2024 22:54
@libklein libklein force-pushed the implement-set-get-polygons branch 3 times, most recently from 10e2af5 to cd453bd Compare June 15, 2024 22:55
@akien-mga akien-mga changed the title NavigationPolygon: Implement get/set_polygon fast paths. NavigationPolygon: Implement get/set_polygon fast paths. Jun 17, 2024
@akien-mga akien-mga modified the milestones: 4.x, 4.3, 4.4 Jun 17, 2024
@smix8
Copy link
Contributor

smix8 commented Aug 16, 2024

Needs both a rebase for 4.4 and the newer thread locks added to the functions.

@libklein libklein force-pushed the implement-set-get-polygons branch from cd453bd to fbafddc Compare August 16, 2024 14:01
@libklein
Copy link
Contributor Author

Rebased onto the latest master:

  • added read/write locks where appropriate

scene/resources/3d/importer_mesh.cpp Outdated Show resolved Hide resolved
modules/navigation/3d/nav_mesh_generator_3d.cpp Outdated Show resolved Hide resolved
@libklein
Copy link
Contributor Author

I've added two commits. d121458 addresses the issues you've outlined.

53d5b05 adds a overload to set_data that allows to set vertices, polygons, and outlines simultaneously in a thread safe way. Moreover, it avoids a for loop in get/set_data. Happy to push that to a different PR/revert if required.

@smix8
Copy link
Contributor

smix8 commented Aug 16, 2024

Thank! Looks fine. I dont have any objection against the overload, that at least makes it clear that this function should never get exposed by accident.

@smix8 smix8 requested a review from a team August 16, 2024 20:27
@akien-mga
Copy link
Member

Looks good! Could you squash the commits? See PR workflow for instructions.

@libklein libklein force-pushed the implement-set-get-polygons branch from 53d5b05 to 63126a4 Compare August 16, 2024 21:51
@akien-mga akien-mga force-pushed the implement-set-get-polygons branch from 63126a4 to a1fe6ff Compare August 16, 2024 22:32
@akien-mga
Copy link
Member

Something went wrong with your rebase, which seemed to have amended changes into an unrelated merge commit from the master branch:
image

I fixed it for you by resetting to the previous commit (53d5b05) and just running git rebase -i upstream/master (with upstream pointing at godotengine/godot and up to date), and marking commits 2 to 5 as fixup.

@akien-mga akien-mga merged commit 66cbdc9 into godotengine:master Aug 16, 2024
17 checks passed
@akien-mga
Copy link
Member

Thanks!

@libklein
Copy link
Contributor Author

Thank you! Is there any other issue I could assist with?

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.

5 participants