-
Notifications
You must be signed in to change notification settings - Fork 178
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
Entity/AI: Range tracking other entities #330
base: develop
Are you sure you want to change the base?
Conversation
Refactored to use more standard methods Code tidy Further tidy Refactored further for better tracking
Refactored to use more standard methods Code tidy Further tidy Refactored further for better tracking
if (!(entity is WorldEntity we)) | ||
return; | ||
|
||
if (!(this is Player)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be an override in player.
/// </summary> | ||
protected bool HasEnteredRange(WorldEntity target) | ||
{ | ||
return inRangeEntities.Keys.Contains(target.Guid); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return inRangeEntities.Keys.Contains(target.Guid); | |
return inRangeEntities.ContainsKey(target.Guid); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you elaborate on the difference here? I assume one is native and one is LINQ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, so, I double checked here, because I remembered something from older .NET versions.
dict.ContainsKey
is going to do a O(1) lookup in the dictionary.
dict.Keys.Contains
will do a linear aka O(n) lookup due to the fact that KeyCollection is not a hash-based collection, but just an array.
But it seems like dict.Keys.Contains
is the same on .NET 5 (and older versions?) https://source.dot.net/#System.Private.CoreLib/Dictionary.cs,1424
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, one of my usual preferred reasons is: reduced cognitive load.
Dictionary.ContainsKey
is a pretty standard API and well known behaviour.
By adding .Keys
, there are increased considerations to take into account, like Keys implementation and behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is no performance benefit, I'll leave it up to @Rawaho to decide the best choice for this so that it stays consistent with rest of framework
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
O(1) vs O(n) is a big difference tho, ContainsKey
is the better way here.
Refactored to use more standard methods Code tidy Further tidy Refactored further for better tracking
No description provided.