Update to use Action Queue - Requested Changes Made 4/14/2017 #229
Conversation
Source/ACE/Entity/Landblock.cs
Outdated
| using global::ACE.Entity.Enum.Properties; | ||
| using global::ACE.Network.Sequence; | ||
|
|
||
| using static LandblockManager; |
There was a problem hiding this comment.
???? This makes for non-obvious inline code. Please no.
There was a problem hiding this comment.
Just making sure I understand the issue. It looks like I must have added something in landblock that pulled in a reference to landblockmanager. That is the issue right?
There was a problem hiding this comment.
You used some automatic tool that did the "using static" stuff which is new in .Net 6 and that quite frankly, I absolutely think is a bad thing they did. It's akin to VB6's "with" statement, only scoped to the entire file.
There was a problem hiding this comment.
I did not even notice it. Thanks and I will clean that up - along with the other items.
Source/ACE/Entity/Landblock.cs
Outdated
| @@ -19,6 +19,11 @@ | |||
|
|
|||
| namespace ACE.Entity | |||
| { | |||
| using global::ACE.Entity.Enum.Properties; | |||
There was a problem hiding this comment.
move above namespace please.
Source/ACE/Entity/Landblock.cs
Outdated
| { | ||
| Player aPlayer = null; | ||
| WorldObject inventoryItem = null; | ||
| lock (objectCacheLocker) |
There was a problem hiding this comment.
You know, I don't think you actually need to lock up the landblock's mutex here. You're not modifying the object collection in this code block.
Source/ACE/Entity/Landblock.cs
Outdated
| } | ||
|
|
||
| // We are droping the item - let's keep track of change in burden | ||
|
|
There was a problem hiding this comment.
Removing the item from the player's inventory should do all this, not the landblock.
Source/ACE/Entity/Landblock.cs
Outdated
|
|
||
| // TODO: I need to look at the PCAPS to see the delta in position from the dropper and the item dropped. Temp position. | ||
|
|
||
| inventoryItem.PositionFlag = UpdatePositionFlag.Contact |
There was a problem hiding this comment.
modification of the inventoryItem's position/whatever data should stay here in the landblock, yes.
Source/ACE/Entity/WorldObject.cs
Outdated
| @@ -51,7 +51,7 @@ public virtual Position Location | |||
|
|
|||
| public WeenieHeaderFlag2 WeenieFlags2 { get; protected set; } | |||
|
|
|||
| public UpdatePositionFlag PositionFlag { get; protected set; } = UpdatePositionFlag.Contact; | |||
| public UpdatePositionFlag PositionFlag { get; set; } = UpdatePositionFlag.Contact; | |||
There was a problem hiding this comment.
Um.... Not a huge fan of this. This isn't the way to solve the always-falling-other-players bug.
Source/ACE/Entity/WorldObject.cs
Outdated
| { | ||
| lock (this.inventoryMutex) | ||
| lock (inventoryMutex) |
There was a problem hiding this comment.
also modify burden here
Source/ACE/Entity/WorldObject.cs
Outdated
| } | ||
| } | ||
|
|
||
| public void RemoveFromInventory(ObjectGuid objectGuid) | ||
| public virtual void RemoveFromInventory(ObjectGuid inventoryItemGuid) | ||
| { |
There was a problem hiding this comment.
also modify burden here
ogmage78
changed the title
|
Updated and ready for merge upon approval. |
Source/ACE/Entity/WorldObject.cs
Outdated
| { | ||
| lock (inventoryMutex) | ||
| { | ||
| if (inventory.ContainsKey(objectGuid)) | ||
| inventory.Remove(objectGuid); | ||
| if (!inventory.ContainsKey(inventoryItem.Guid)) inventory.Add(inventoryItem.Guid, inventoryItem); |
There was a problem hiding this comment.
whitespace - logic on different line than condition.
There was a problem hiding this comment.
Ok got rid of inline if's, think I fixed the whitespace - I may need remedial training on that to make sure I understand our style. I fixed my last bug and think it is all ready to go.
Source/ACE/Entity/WorldObject.cs
Outdated
| { | ||
| if (!this.inventory.ContainsKey(obj.Guid)) | ||
| this.inventory.Add(obj.Guid, obj); | ||
| if (!this.inventory.ContainsKey(inventoryItemGuid)) return; |
There was a problem hiding this comment.
as before. no inline ifs.
… I may need a remedical lesson on how to format this correctly. :)
Source/ACE/Entity/Landblock.cs
Outdated
| if (worldObjects.ContainsKey(playerId)) | ||
| { | ||
| aPlayer = (Player)worldObjects[playerId]; | ||
| inventoryItem = worldObjects[inventoryId]; |
There was a problem hiding this comment.
We can't assume inventoryId is in this landblock. We need something like FindWorldObject which walks this and adjacent landblocks to find it.
There was a problem hiding this comment.
Thanks for the review. We were chatting about it in Discord - was going to address that later as it is such an edge case. I would have to try to pickup something someone had dropped across a landblock boundary. At the moment all it should do is fail to pick it up. I can refactor to use the FindWorldObject later if that is ok? Thanks
|
Stackoverflow pointed out a part of code that was not checking for existence prior to usage - I made that fix. |
ogmage78
changed the title
Can someone with more experience with Action Queue take a look at the file Landblock and the case GameActionType.DropItem: I don't want to refactor all of my stuff until someone can tell me if this looks correct. It complies and works fine. I would just like a sanity check.