-
Notifications
You must be signed in to change notification settings - Fork 5
Added Drop Packet Handler #20
base: master
Are you sure you want to change the base?
Conversation
Fixed param of create ground item factory and added brackets ah code quality checker suggested. |
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.
Thanks for your PR, when you have fixed existing problems, could be cool if you create unit test for this packet handler.
return; | ||
} | ||
|
||
Entity entity = _entityFactory.CreateGroundItem(packet.DropId, packet.VNum, packet.Amount); |
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.
Set variable type as GroundItem instead of Entity since CreateGroundItem will always return a GroundItem
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.
Got it, ill will try to fix this and the rest of the issue maybe later this week. Thank you for your keen eye.
Entity entity = _entityFactory.CreateGroundItem(packet.DropId, packet.VNum, packet.Amount); | ||
entity.Position = new Position(packet.PositionX, packet.PositionY); | ||
|
||
if (entity is GroundItem drop) |
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.
By fixing previous problem, you can also remove this type check/cast.
|
||
if (map == null) | ||
{ | ||
_logger.Warn("Handling InPacket but character map is null"); |
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.
Change warning message to Handling DropPacket instead of Handling InPacket
} | ||
|
||
map.AddEntity(entity); | ||
_logger.Info($"Entity {entity.EntityType} {entity.Id} joined map"); |
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.
Change info message to Drop with id {entity.Id} added to map instead or something like this
map.AddEntity(entity); | ||
_logger.Info($"Entity {entity.EntityType} {entity.Id} joined map"); | ||
|
||
_eventManager.Emit(new EntityJoinEvent(client) |
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.
Maybe you can also create a DropEvent or something with GroundItem & Map as property
Had issue with packet drop handler, it seems it wasnt' yet implemented. I saw that the GroundItem packets is currently beig handled in InPacketHandler.cs but its current header is "in" while the drop packet header is "drop".
This is my first time contributing in an opensource project. Sorry if codes wasn't that good.