Skip to content

Performance improvement for large scenes #198

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

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

Conversation

Extrys
Copy link

@Extrys Extrys commented Jan 3, 2021

Now the removal of obstacles and agents when processing the navMeshBuildSource list has improved a lot for scenes with high density of GameObjects

Now the removal of obstacles and agents when processing the navMeshBuildSource list has improved a lot for scenes with high density of GameObjects
@Extrys
Copy link
Author

Extrys commented Jan 3, 2021

@turadr I have been tested in a large scene and it takes approximately only 8% of the time now

@mattluard
Copy link

Thanks for sharing, I've been trying out this optimisation in my game and it's working great.

@Extrys
Copy link
Author

Extrys commented Jan 17, 2021

Thanks for sharing, I've been trying out this optimization in my game and it's working great.

I'm glad to hear that! let's see if this gets merged into the official master branch :)

@turadr
Copy link
Collaborator

turadr commented Jan 18, 2021

@Extrys Thank you very much for sharing these changes with the community. We are currently not merging any external contributions. Once that will become possible again we will evaluate this PR.

@Extrys
Copy link
Author

Extrys commented Jan 18, 2021

@Extrys Thank you very much for sharing these changes with the community. We are currently not merging any external contributions. Once that will become possible again we will evaluate this PR.

Nice, thanks for letting me know about that!

@Extrys
Copy link
Author

Extrys commented Aug 4, 2021

I stronly recommend merging this PR, since it improves a lot the baking time for realtime continuous simulation

@@ -341,10 +341,11 @@ List<NavMeshBuildSource> CollectSources()
}

if (m_IgnoreNavMeshAgent)
sources.RemoveAll((x) => (x.component != null && x.component.gameObject.GetComponent<NavMeshAgent>() != null));
sources.RemoveAll((x) => x.component != null && x.component is NavMeshAgent);
Copy link
Author

Choose a reason for hiding this comment

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

This has been made in this way due to Its much more efficient a type check tan getting from the "already obtained component" the component with he typed object, and checking if is not null

So
x.component != null && x.component is NavMeshAgent
Is better than
x.component != null && x.component.gameObject.GetComponent() != null

This even let you use this in an asynchronous way since no unity API is being called
I have this oriented to async building, in my project, and it works fantastic

Is for that the second part of the condition is changed in both lines
So i see like a bad move not merging this, honestly

@Nrosa01
Copy link

Nrosa01 commented Aug 6, 2021

I've tested this changed and now my navmesh builds a lot faster, When are you merging this?

@Extrys
Copy link
Author

Extrys commented Aug 27, 2021

Thanks for approving

@mattluard
Copy link

Haha, I think anyone can (like I just did) go and click Review Changes and 'approve' them, but that's not the same as turadr merging them in.

For what it's worth, I'm still using this performance improvements and it's been fine for me. So that's my 'approval', as far as it goes.

@Extrys
Copy link
Author

Extrys commented Aug 28, 2021

Haha, I think anyone can (like I just did) go and click Review Changes and 'approve' them, but that's not the same as turadr merging them in.

For what it's worth, I'm still using this performance improvements and it's been fine for me. So that's my 'approval', as far as it goes.

Ahh the. Thanks a lot!

Currently this improvement is made with de asynchronous navmesh building in mind

I can not post that improvement because for that second part of the improvement you need Cysharp.Unitasks

That's where the true improvement appears

I use this for my game Wich requires recalculating the navmesh constantly in large scenes

@bermudalocket
Copy link

Haha, I think anyone can (like I just did) go and click Review Changes and 'approve' them, but that's not the same as turadr merging them in.
For what it's worth, I'm still using this performance improvements and it's been fine for me. So that's my 'approval', as far as it goes.

Ahh the. Thanks a lot!

Currently this improvement is made with de asynchronous navmesh building in mind

I can not post that improvement because for that second part of the improvement you need Cysharp.Unitasks

That's where the true improvement appears

I use this for my game Wich requires recalculating the navmesh constantly in large scenes

Is the UniTask branch available anywhere?

@Extrys
Copy link
Author

Extrys commented Nov 30, 2021

Haha, I think anyone can (like I just did) go and click Review Changes and 'approve' them, but that's not the same as turadr merging them in.
For what it's worth, I'm still using this performance improvements and it's been fine for me. So that's my 'approval', as far as it goes.

Ahh the. Thanks a lot!
Currently this improvement is made with de asynchronous navmesh building in mind
I can not post that improvement because for that second part of the improvement you need Cysharp.Unitasks
That's where the true improvement appears
I use this for my game Wich requires recalculating the navmesh constantly in large scenes

Is the UniTask branch available anywhere?

No, but if you want I can do it if that helps

@dycoon
Copy link

dycoon commented Oct 1, 2022

Good Fix!
I think we should merge this pull request first and improve it later if needed.

@Nrosa01
Copy link

Nrosa01 commented Dec 3, 2022

Is this going to be merged? I'm still waiting :(

@psociety
Copy link

Hi! At Unity our ego doesn't allow us to merge external improvements nor fixes.
However we are open to merge PRs that introduce bugs to make our software even worse than it already is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants