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

Inventory Phase 1 and Drop Item #189

Merged
merged 5 commits into from
Apr 3, 2017

Conversation

ogmage78
Copy link
Contributor

This is at a good place for code review and possible merge. Once approved, this will allow others to start on the numerous tasks around inventory management. You can spawn a Training wand in your backpack or on the ground. That is just throw away code to test with. It is in the debug handler.
usage:
@ctw me OR @ctw ground

Once the item is spawned, you can then drop it on the ground.

Included in this PR. Starts to address Issue #144

World object now has inventory dictionary. Added this here as anything that can be a container needs to have this ability. Not all world objects are containers - so also added property IsContainer

Added methods to:

  • Add to inventory
  • Remove from inventory
  • Lookup item in inventory
  • Handle Drop item:
    1. Calls lookup
    2. Update Container burden
    3. Sets some properties in preparation for going 3D
    4. GameMessageUpdatePosition.
    5. Alerts Landblock to new arrival
    6. Removes the item the player's inventory
      Added GameActionDropItem
      This handles the client's request to drop an item.
      Added GameMessagePutObjectIn3d

As usual - any coding faux pas are not by design. Feedback is wanted and welcome. If there is a better way to do what I did - please let me know.

…te an item in your backpack and drop the item into 3D space. Start of inventory management
@ogmage78
Copy link
Contributor Author

ogmage78 commented Apr 2, 2017

I will fix this first thing in the morning. I looked they are easy to resolve. Thanks for checking it over.

@ogmage78
Copy link
Contributor Author

ogmage78 commented Apr 2, 2017

I fixed the merge issues. When you get a chance to review this - if you look at:
public void HandleDropItem(ObjectGuid objectGuid, Session session) - I am not sure if it needs to be on world object or player. I cannot remember if any NPC's would drop anything as part of a quest. I put in in WO just in case. That might be a mistake. If it needs to go into player - I need to create a method on world object to update containerId. Anyway - any feedback is appreciated.

Copy link
Contributor

@Mogwai-TheFurry Mogwai-TheFurry left a comment

Choose a reason for hiding this comment

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

Fix the mutex and ReadOnlyCollection and I'll take it. The Objects coming out of the Landblock is a necessary evil until we have an action queue.

@@ -72,6 +85,10 @@ public ushort ForcePositionIndex

public SequenceManager Sequences { get; }

public object InventoryMutex => this.inventoryMutex;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm really hesitant to think a mutex needs to be public. Red flag for other code being nosey into this class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. I got confused there -

@@ -72,6 +85,10 @@ public ushort ForcePositionIndex

public SequenceManager Sequences { get; }

public object InventoryMutex => this.inventoryMutex;

public Dictionary<ObjectGuid, WorldObject> Inventory => this.inventory;
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be a ReadOnlyDictionary. You'll have to new it up in the constructor. The default for "inventory" may fire after this, rendering this default incorrect. I'm not exactly sure when the "=>" operator binds on the property, so it may work after all. If you feel this is correct as is, I'm open to discussion as always.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

{
// Find the item we want to pick up
var obj = LandblockManager.GetWorldObject(session, itemGuid);
if (Object.ReferenceEquals(null, obj))
Copy link
Contributor

Choose a reason for hiding this comment

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

if (obj == null)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

this.Position.positionZ = obj.Position.positionZ;
this.Position.rotationW = obj.Position.rotationW;
this.Position.rotationZ = obj.Position.rotationZ;

Copy link
Contributor

Choose a reason for hiding this comment

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

whoa, does this actually work?

this.Position.rotationZ = obj.Position.rotationZ;

session.Network.EnqueueSend(new GameMessageUpdatePosition(this));

Copy link
Contributor

Choose a reason for hiding this comment

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

remove large chunks of commented code. If you need to keep them, feel free to make a blog page on the acemulator wiki.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed - I did not see I pushed that. Comments gone. Sorry about that.

@@ -61,8 +61,17 @@ public static void HandleQueryHealth(Session source, ObjectGuid targetId)
}

/// <summary>
/// return wo in preparation to take it off the landblock and put it in a container.
/// </summary>
public static WorldObject GetWorldObject(Session source, ObjectGuid targetId)
Copy link
Contributor

Choose a reason for hiding this comment

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

ugh... we really need that action queue...

Copy link
Contributor Author

@ogmage78 ogmage78 left a comment

Choose a reason for hiding this comment

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

Fixed the issues you outlined.

@@ -72,6 +85,10 @@ public ushort ForcePositionIndex

public SequenceManager Sequences { get; }

public object InventoryMutex => this.inventoryMutex;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. I got confused there -

@@ -72,6 +85,10 @@ public ushort ForcePositionIndex

public SequenceManager Sequences { get; }

public object InventoryMutex => this.inventoryMutex;

public Dictionary<ObjectGuid, WorldObject> Inventory => this.inventory;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

{
// Find the item we want to pick up
var obj = LandblockManager.GetWorldObject(session, itemGuid);
if (Object.ReferenceEquals(null, obj))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

this.Position.rotationZ = obj.Position.rotationZ;

session.Network.EnqueueSend(new GameMessageUpdatePosition(this));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed - I did not see I pushed that. Comments gone. Sorry about that.

@Mogwai-TheFurry Mogwai-TheFurry merged commit cc7db4a into ACEmulator:master Apr 3, 2017
@ogmage78 ogmage78 deleted the inventorypart1 branch April 14, 2017 19:37
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.

None yet

3 participants