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

[Navigation] Restore 2D only navigation #88681

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AThousandShips
Copy link
Member

@AThousandShips AThousandShips commented Feb 22, 2024

Starting in 4.2 navigation was restricted to 3D enabled builds only, in my testing this isn't an actual limitation, and my testing shows navigation works (haven't tested baking in 2D, but pathfinding works fine, and unit tests work correctly)

Edit: Tested and runtime baking works fine too

@AThousandShips AThousandShips added bug regression topic:navigation topic:2d cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Feb 22, 2024
@AThousandShips AThousandShips added this to the 4.3 milestone Feb 22, 2024
@AThousandShips AThousandShips requested a review from a team February 22, 2024 19:38
@AThousandShips AThousandShips requested review from a team as code owners February 22, 2024 19:38
@AThousandShips AThousandShips changed the title [Navigation] Allow 2D only navigation [Navigation] Restore 2D only navigation Feb 22, 2024
@smix8
Copy link
Contributor

smix8 commented Feb 23, 2024

As mentioned there is no real 2D navigation server. The 2D server forwards and backwards all queries to the 3D server so you can not have a build with a disabled 3D server. If you try and it works it does because guards were added against crashes from missing e.g. 3D thirdparty libs but it will not be a true 3D disabled build because the 3D server needs to be included.

@AThousandShips
Copy link
Member Author

True, but I'd say that's worth it to have a non "true" build, unless we want to accept the regression

@AThousandShips AThousandShips removed regression cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Feb 23, 2024
@AThousandShips AThousandShips modified the milestones: 4.3, 4.4 Feb 23, 2024
@AThousandShips AThousandShips marked this pull request as draft February 23, 2024 09:11
@AThousandShips
Copy link
Member Author

Following some discussion I'm going to look more into the specifics of this and test

@AThousandShips AThousandShips marked this pull request as ready for review March 26, 2024 13:25
@AThousandShips AThousandShips requested a review from a team as a code owner March 26, 2024 13:26
@AThousandShips
Copy link
Member Author

Temporarily including these fixes to build template builds, the first commit isn't part of this commit:

@AThousandShips
Copy link
Member Author

AThousandShips commented Mar 26, 2024

This is now ready for review, need testing to help with anything I've missed but it works correctly and tests fine, so should work perfectly assuming I haven't missed anything, removed some 3D only parts to reduce the size on 2D only builds

Windows, debug build:

  • Before (with the build fix): 89'101'824 bytes
  • After: 89'786'880 bytes
    Without the last commit: 89'837'056 bytes
  • Difference: +685'056 bytes (about +0.77%)
    Without the last commit: +735'232 bytes (about +0.83%)

Windows, release build:

  • Before: 56'551'424 bytes
  • After: 56'911'360 bytes
    Without the last commit: 89'837'056 bytes
  • Difference: +359'936 bytes (about +0.62%)
    Without the last commit: +371'712 bytes (about +0.66%)

So this doesn't really come with any significant cost compared to default

When comparing a 3D enabled build the difference in saving is about 13.7%/14.5% with this, and 14.2%/15.2% with the normal build, so the savings are still very significant (the savings drop by about 3.9%/4.3%), for release and debug respectively

Copy link

@growingbrain growingbrain left a comment

Choose a reason for hiding this comment

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

Thanks for making these updates. Just a few questions.

modules/navigation/nav_map.h Show resolved Hide resolved
modules/navigation/nav_agent.cpp Show resolved Hide resolved
modules/navigation/nav_agent.cpp Show resolved Hide resolved
@smix8
Copy link
Contributor

smix8 commented Apr 4, 2024

I really would prefer to split the navobjects into 2d and 3d versions instead of having many new ifdef landmines in the code. That would require to give the NavigationServer2D some actual backbone.

@AThousandShips
Copy link
Member Author

I can look at working on that, either in this or as a follow-up

@smix8
Copy link
Contributor

smix8 commented Nov 15, 2024

What is the state of this PR.

Is the goal to just allow to compile the more optional 3D parts out to save a few bytes or is the intention to "restore" an actual 2D navigation.

@AThousandShips
Copy link
Member Author

AThousandShips commented Nov 15, 2024

It restores 2D navigation, but doesn't separate out any 2D only code separate from the 3D elements, haven't gotten around to testing any such solution yet (though it reduces some of the unused code being compiled)

But it works as advertised, 2D navigation will work in 3D-disabled builds (I've tested it in my own projects)

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.

3 participants