feat: Port Stride.Navigation native code to C# using DotRecast #2987
Conversation
|
I started a Copilot reriew, if it helps at all.. |
There was a problem hiding this comment.
Pull request overview
This PR successfully ports the Stride.Navigation module's native C++ code to managed C# using the DotRecast library. This represents a significant architectural improvement that eliminates the need for platform-specific native binaries and MSVC compiler, making the codebase more maintainable and accessible to contributors. The changes include:
- Complete removal of C++ source files (Navigation.cpp, NavigationBuilder.cpp, NavigationMesh.cpp) and headers
- New C# implementations: NavigationBuilder.cs, InternalNavigationMesh.cs, and DtMeshDataSerializer.cs
- Updated existing C# files to work with the new managed implementation
Key Changes
- Replaced native P/Invoke calls with DotRecast library APIs
- Changed navigation mesh tile data from
byte[]toDtMeshData(breaking change) - Removed
IDisposablepattern fromRecastNavigationMeshclass - Modernized C# syntax with collection expressions
[]andfieldkeyword - Added custom serialization for
DtMeshData
Reviewed changes
Copilot reviewed 48 out of 48 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| NavigationBuilder.cs | New C# implementation replacing C++ NavigationBuilder |
| InternalNavigationMesh.cs | New wrapper for DotRecast navigation mesh operations |
| DtMeshDataSerializer.cs | New custom serializer for DotRecast mesh data structures |
| RecastNavigationMesh.cs | Updated to use managed InternalNavigationMesh |
| NavigationMeshTile.cs | Changed Data type from byte[] to DtMeshData with custom serializer |
| NavigationProcessor.cs | Removed Dispose() calls, modernized syntax |
| NavigationComponent.cs | Updated to use field keyword for auto-properties |
| Various utility files | Modernized with collection expressions and pattern matching |
| C++ files, headers, build scripts | All removed |
| Recast library files | All removed |
Doprez
left a comment
Doprez
left a comment
There was a problem hiding this comment.
LGTM, also tested with existing scenes I had for Bullet and didnt see any new issues come up.
Eideren
left a comment
Eideren
left a comment
There was a problem hiding this comment.
There's a bunch of areas that could use some polish, but given that those come from c++, might as well leave it at that for now.
| public void RemoveTile(Point coord) | ||
| { | ||
| if (!tileCoordinates.Contains(coord)) | ||
| return false; | ||
| if (!tileCoordinates.Contains(coord)) | ||
| return; | ||
|
|
||
| tileCoordinates.Remove(coord); | ||
| return Navigation.RemoveTile(navmesh, coord); | ||
| navmesh.RemoveTile(coord); |
There was a problem hiding this comment.
Could you comment on this change in return type ?
There was a problem hiding this comment.
The logic behind this was that the consuming method (AddOrReplaceTile) did not take the return value into account in any way, so I changed it into void type.
There was a problem hiding this comment.
But now that I think about it, and considering that this is a public method, I should actually undo it.
|
Thanks ! |
4 participants
PR Details
PR ports C++ code to manageable .NET code using the DotRecast library. Additionally, I simplified/removed many code fragments in the module.
The benefits I see from this changes:
PR was tested on the TopDownRPG sample.
Related Issue
#1394
Types of changes
Checklist