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

Extend NavigationMeshSourceGeometryData[23]D to allow data merging #88221

Merged

Conversation

Scony
Copy link
Contributor

@Scony Scony commented Feb 11, 2024

This PR adds merge method to NavigationMeshSourceGeometryData3D

This PR therefore adds a way to avoid boilerplate code such as lampe-games/godot-open-rts@487e995#diff-0291eaa88b59eec77f3d1546e6404bea1391d58eec1ee646874dded17f0bfe16R60-R71

@Scony Scony requested review from a team as code owners February 11, 2024 22:20
@smix8
Copy link
Contributor

smix8 commented Feb 11, 2024

Imo it would be better to add a merge() function on the source geometry resource with another source geometry resource as the parameter, e.g. like Dictionary does it.

Just because you want to merge something does not mean you want to have all the overhead of the single-threaded parsing again as well. Many users create their entire source geometry procedual in scripts never using the parse function. They might still want the ability to merge two of them together which sounds useful doing in C++ compared to doing a slow copy in scripts.

Also changing the parse function on the NavigationServer breaks the function signature and compatibility while adding a merge() function does not.

@smix8 smix8 added this to the 4.x milestone Feb 11, 2024
@Scony Scony marked this pull request as draft February 12, 2024 16:36
@Scony Scony force-pushed the extend-parse_source_geometry_data branch from 72ea860 to 85fd0d7 Compare February 20, 2024 19:03
@Scony Scony force-pushed the extend-parse_source_geometry_data branch from 85fd0d7 to ed62070 Compare March 6, 2024 22:13
@Scony Scony marked this pull request as ready for review March 6, 2024 22:14
@Scony Scony changed the title Add ability to perform cumulative geometry parsing in NavigationServer3D Extend NavigationMeshSourceGeometryData[23]D to allow data merging Mar 6, 2024
@Scony Scony changed the title Extend NavigationMeshSourceGeometryData[23]D to allow data merging Extend NavigationMeshSourceGeometryData[23]D to allow data merging Mar 6, 2024
Copy link
Contributor

@smix8 smix8 left a comment

Choose a reason for hiding this comment

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

Approve for the API.

Not sure about the code style, the use of ::Size looks exotic compared to the rest of the engine.

@smix8 smix8 modified the milestones: 4.x, 4.3 Mar 7, 2024
@Mickeon
Copy link
Contributor

Mickeon commented Mar 7, 2024

Seems like the "Size" alias is a relatively new thing since the 64-bit CowData upgrade. A quick look-up tells me it has never been used like this before. Other code just... puts the size into an int64_t or int32_tfor simplicity, although this is more future-proof.

Personally I would just do that, int64_t for consistency. Alternatively it should be better as a global Size alias but that would be a change to apply across the whole codebase...?

@Scony Scony force-pushed the extend-parse_source_geometry_data branch from ed62070 to bb7c28f Compare March 7, 2024 18:55
@Scony Scony force-pushed the extend-parse_source_geometry_data branch from bb7c28f to ab24276 Compare March 7, 2024 18:57
@Scony Scony requested a review from akien-mga March 7, 2024 18:58
@akien-mga akien-mga merged commit 6f51f73 into godotengine:master Mar 8, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

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