Reworked Items #111

d2inventory · 2020-11-17T20:50:26Z

All the items of the player are now in Items.
The Location attribute of Item is not just BodyLocation anymore,
it contains the position in the inventory along with it's width and height.
The Container indicates where the Item is stored (Equipment, Belt, Cube, Inventory, Stash)

Changed GetItemDescription to take a D2Unit (item) instead of Item and made it public
Added Location for hireling items

All the items of the player are now in Items. The Location attribute of Item is not just BodyLocation anymore, it contains the position in the inventory along with it's width and height. The Container indicates where the Item is stored (Equipment, Belt, Cube, Inventory, Stash) Changed GetItemDescription to take a D2Unit (item) instead of Item and made it public Added Location for hireling items

d2inventory · 2020-11-17T20:51:30Z

It works on my end, but still needs to be tested with Plugy and PD2 etc.

Also maybe the Hireling needs to be done differently. Also nothing done for the PipeServer thingy :D

Zutatensuppe

please see comments =)
I would ask to undo the 'formatting only' change in GameMemoryTableFactory for now. (Probably the whole project code should be reformatted as a whole, if something like this is done, so that it is consistent. I think it is not 100% consistent atm anyway, but doing partly reformat will make it more inconsistent ^^)

All the other comments are just questions and comments that don't require a change now in my opinion.

Zutatensuppe · 2020-11-23T12:31:29Z

src/D2Reader/GameMemoryTableFactory.cs

+                Loading                     = BaseAddress + 0x30F2C0,
+                Saving                      = BaseAddress + 0x3792F8,
+                Saving2                     = BaseAddress + 0x3786D0,
+                InGame                      = BaseAddress + 0x30EE8C,
+                InMenu                      = BaseAddress + 0x379970,

-                GlobalData = BaseAddress + 0x00344304, // game.744304
-                World = BaseAddress + 0x00483D38,
-                PlayersX = BaseAddress + 0x483D70,
-                GameId = BaseAddress + 0x00482D0C,
-                LowQualityItems = BaseAddress + 0x56CC58,
-                ItemDescriptions = BaseAddress + 0x56CA58,
-                MagicModifierTable = BaseAddress + 0x56CA7C,
-                RareModifierTable = BaseAddress + 0x56CAA0,
-                Units113 = null,
-                Units114 = BaseAddress + 0x003A5E70,
-                PlayerUnit = BaseAddress + 0x003A5E74,
-                Area = BaseAddress + 0x003A3140,
-                Pets = BaseAddress + 0x003BB5BC,
-                InventoryTab = BaseAddress + 0x3BCC4C,
-                StringIndexerTable = BaseAddress + 0x4829B4,
-                StringAddressTable = BaseAddress + 0x4829B8,
-                PatchStringIndexerTable = BaseAddress + 0x4829D0,
-                PatchStringAddressTable = BaseAddress + 0x4829BC,
+                GlobalData                  = BaseAddress + 0x344304, // game.744304
+                World                       = BaseAddress + 0x483D38,
+                PlayersX                    = BaseAddress + 0x483D70,
+                GameId                      = BaseAddress + 0x482D0C,
+                LowQualityItems             = BaseAddress + 0x56CC58,
+                ItemDescriptions            = BaseAddress + 0x56CA58,
+                MagicModifierTable          = BaseAddress + 0x56CA7C,
+                RareModifierTable           = BaseAddress + 0x56CAA0,
+                Units113                    = null,
+                Units114                    = BaseAddress + 0x3A5E70,
+                PlayerUnit                  = BaseAddress + 0x3A5E74,
+                Area                        = BaseAddress + 0x3A3140,
+                Pets                        = BaseAddress + 0x3BB5BC,
+                InventoryTab                = BaseAddress + 0x3BCC4C,
+                StringIndexerTable          = BaseAddress + 0x4829B4,
+                StringAddressTable          = BaseAddress + 0x4829B8,
+                PatchStringIndexerTable     = BaseAddress + 0x4829D0,
+                PatchStringAddressTable     = BaseAddress + 0x4829BC,


there is no change here apart from the formatting
the file should not be part of the PR

maybe we can implement some kind of coding standard and then reformat all the code accordingly, but for now i'd like to keep this as it is, if there is no real code change

Zutatensuppe · 2020-11-23T12:32:25Z

src/D2Reader/Models/Item.cs

+        internal bool IsInCube() => !IsEquipped() && ItemData.InvPage == InventoryPage.HoradricCube;
+        internal bool IsInStash() => !IsEquipped() && ItemData.InvPage == InventoryPage.Stash;
+        internal bool IsInInventory() => !IsEquipped() && ItemData.InvPage == InventoryPage.Inventory;


Do you know why IsEquipped check is needed in all those cases?

Zutatensuppe · 2020-11-23T12:43:54Z

src/D2Reader/Struct/Inventory/D2InventoryInfo.cs

@@ -0,0 +1,31 @@
+using System.Runtime.InteropServices;


nice job finding the new structures =D

Zutatensuppe · 2020-11-23T12:46:57Z

src/D2Reader/Readers/UnitReader.cs

        }

-        private D2ItemDescription GetItemDescription(Item item)
+        public D2ItemDescription GetItemDescription(D2Unit item)


Fine for now =)

But I d prefer if this could stay Item (more clear that 100% item comes in, and never another unit), but the change in InventoryReader makes this necessary atm.

Zutatensuppe · 2020-11-23T12:51:35Z

src/D2Reader/Models/Item.cs

+            return this.X            == other.X &&
+                   this.Y            == other.Y &&
+                   this.Width        == other.Width &&
+                   this.Height       == other.Height &&
+                   this.BodyLocation == other.BodyLocation &&
+                   this.Container    == other.Container;


No need to change, but just a note because i saw it in multiple places:

So far we didn't use this kind of alignment of parameters or operators, and at least the reason for me is that a change in one line may increase PR unecessary, because all lines need to be adjusted if longest line changes. But as said, no need to change, it may be better readable this way, and we read this more often than change it :D

And as this is new code and not purely formatting, this is fine for me!

Zutatensuppe · 2020-11-23T12:51:55Z

src/D2Reader/Models/Item.cs

+
+        public override int GetHashCode()
+        {
+            return X.GetHashCode() ^ Y.GetHashCode() ^ Width.GetHashCode() ^ Height.GetHashCode() ^ BodyLocation.GetHashCode() ^ Container.GetHashCode();


pretty long line ^^

d2inventory · 2020-11-23T15:08:33Z

Imma reply to all in this comment.

I would prefer it if the GameMemoryTableFactory.cs would be all formated like that because it very quickly highlights the memory region the data is in. I formatted that because I initially added my own pointers to get to the item containers and frankly it was horrible to read that code with only some addresses normalized to 8 digits and everything "jagged".
This is comming from a perspective of me being a complete stranger to the code and wanting to quickly read into it to get the information I am looking for.
To me this type of information is "spread sheet data" so imo it should be formatted like a spread sheet.
I got it all formatted like this on my end, so I can do either one, remove the file all together from the PR or update it with consistent formatting like the example shown.

I am not sure why I added the IsEquipped() check, probably just didn't understand the way it was done at the time. It doesn't really make any sense if I am looking at it now so I guess I just remove that part.
The functions aren't in use anymore anyway but I thought I leave them in there cuz why not 😄

GetItemDescription()
I kinda disagree here, because the class is UnitReader and imo it should perform actions around units so it handling a D2Unit of type item makes more sense than an Item directly where the Unit has to be extracted first.
I'm more "annoyed" because I had to make it public. I think the UnitReader should offer a single point solution to get the DiabloInterface representation of an item from a D2Unit (be it a Player to get multiple items or one D2Unit-Item to Item "converter").
But yeah, maybe that's just me misunderstanding what the goal is.
I fully agree though that there needs to be a check that no other D2Unit than one of type Item can be passed to the functions. So maybe add something like this to the top of the function?

if (item.eType != D2UnitType.Item)
    throw new Exception("Wrong D2Unit type");

Item Comparison
I already said why I like to format "spread sheet data" in this style.
The line is indeed very long hahaha, one ugly motherfucker 😄
Tbh this is just boilerplate code I copy pasted to satisfy the compiler.
Waddayathink should I just break it into 2 lines to fit more with all the other code?

d2inventory · 2020-11-25T12:25:07Z

alrighty, I reverted the formatting in GameMemoryTableFactory and removed the unnecessary checks in the Item class

I also added the check in GetItemDescription so there will be a sane Exception if the wrong D2Unit type is passed ot the function.

Zutatensuppe

🎉 nice work! =)

Fixed ItemInfoTest

3f50b67

Zutatensuppe self-requested a review November 18, 2020 07:06

Zutatensuppe reviewed Nov 23, 2020

View reviewed changes

huehuehue added 3 commits November 25, 2020 13:18

Check for D2Unit type in GetItemDescription()

dd16a94

Removed not needed checks in Item class

aba990f

Removed formatting in GameMemoryTableFactory

f81a176

Zutatensuppe approved these changes Dec 2, 2020

View reviewed changes

Zutatensuppe merged commit 1a75ac5 into DiabloRun:master Dec 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reworked Items #111

Reworked Items #111

Uh oh!

d2inventory commented Nov 17, 2020

Uh oh!

d2inventory commented Nov 17, 2020

Uh oh!

Zutatensuppe left a comment

Uh oh!

Zutatensuppe Nov 23, 2020

Uh oh!

Zutatensuppe Nov 23, 2020

Uh oh!

Zutatensuppe Nov 23, 2020

Uh oh!

Zutatensuppe Nov 23, 2020

Uh oh!

Zutatensuppe Nov 23, 2020

Uh oh!

Zutatensuppe Nov 23, 2020

Uh oh!

d2inventory commented Nov 23, 2020 •

edited

Loading

Uh oh!

d2inventory commented Nov 25, 2020

Uh oh!

Zutatensuppe left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Reworked Items #111

Reworked Items #111

Uh oh!

Conversation

d2inventory commented Nov 17, 2020

Uh oh!

d2inventory commented Nov 17, 2020

Uh oh!

Zutatensuppe left a comment

Choose a reason for hiding this comment

Uh oh!

Zutatensuppe Nov 23, 2020

Choose a reason for hiding this comment

Uh oh!

Zutatensuppe Nov 23, 2020

Choose a reason for hiding this comment

Uh oh!

Zutatensuppe Nov 23, 2020

Choose a reason for hiding this comment

Uh oh!

Zutatensuppe Nov 23, 2020

Choose a reason for hiding this comment

Uh oh!

Zutatensuppe Nov 23, 2020

Choose a reason for hiding this comment

Uh oh!

Zutatensuppe Nov 23, 2020

Choose a reason for hiding this comment

Uh oh!

d2inventory commented Nov 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

d2inventory commented Nov 25, 2020

Uh oh!

Zutatensuppe left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

d2inventory commented Nov 23, 2020 •

edited

Loading