Zip #745

FurkanS1821 · 2018-10-01T18:37:25Z

.zip file support for content. (Unzipped files may overwrite what is in .zip)

No other mentionable change, except compatibility.
Requires LeagueSandbox/LeagueSandbox-Default#84 to work, because the content has been regenerated.

FurkanS1821 · 2018-10-01T18:39:32Z

GameServerLib/Content/CharData.cs

@@ -110,10 +115,11 @@ public void Load(string name)
            HpRegenPerLevel = file.GetFloat("Data", "HPRegenPerLevel", HpRegenPerLevel);
            MpRegenPerLevel = file.GetFloat("Data", "MPRegenPerLevel", MpRegenPerLevel);
            AttackSpeedPerLevel = file.GetFloat("Data", "AttackSpeedPerLevel", AttackSpeedPerLevel);
-            IsMelee = file.GetString("Data", "IsMelee", IsMelee ? "Yes" : "No").Equals("yes");
+            IsMelee = file.GetString("Data", "IsMelee", IsMelee ? "Yes" : "No").ToLower().Equals("yes");


Self note: make this ToLowerInvariant() just in case.

codecov-io · 2018-10-03T17:33:34Z

Codecov Report

Merging #745 into indev will increase coverage by 7.27%.
The diff coverage is 50.56%.

@@           Coverage Diff            @@
##           indev    #745      +/-   ##
========================================
+ Coverage   1.88%   9.15%   +7.27%     
========================================
  Files        131     131              
  Lines       7798    7742      -56     
  Branches     614     616       +2     
========================================
+ Hits         147     709     +562     
+ Misses      7651    7028     -623     
- Partials       0       5       +5

Impacted Files	Coverage Δ
...meServerLib/Exceptions/ContentNotFoundException.cs	`0% <ø> (ø)`
GameServerLib/Program.cs	`0% <ø> (ø)`	⬆️
GameServerLib/GameObjects/Missiles/Cone.cs	`0% <0%> (ø)`	⬆️
GameServerLib/Game.cs	`8.28% <0%> (+8.28%)`	⬆️
GameServerLib/Content/SpellData.cs	`0% <0%> (ø)`	⬆️
GameServerLib/GameObjects/Spells/Spell.cs	`0% <0%> (ø)`	⬆️
GameServerLib/GameObjects/Missiles/Projectile.cs	`0% <0%> (ø)`	⬆️
GameServerLib/Config.cs	`0% <0%> (ø)`	⬆️
GameServerLib/Content/CharData.cs	`0% <0%> (ø)`	⬆️
GameServerLib/GameObjects/Missiles/Laser.cs	`0% <0%> (ø)`	⬆️
... and 30 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4055a0f...3678cdf. Read the comment docs.

danil179

some indexes instead of arrays, don't sure if it's from your PR or was already, but should be changed.

danil179 · 2018-10-04T14:11:24Z

GameServerLib/Items/ItemData.cs

+        public int RecipeItem1 { get; private set; }
+        public int RecipeItem2 { get; private set; }
+        public int RecipeItem3 { get; private set; }
+        public int RecipeItem4 { get; private set; }


do you need only 4? maybe use array? and do RecipeItem[i] = content.GetInt("Data", "RecipeItem{i}", -1),.

Yep, there doesn't seem to be a good reason to limit this to just 4 items. My vote also goes out to arrays (for potential custom content in the future).

danil179 · 2018-10-04T14:12:04Z

GameServerLib/Items/ItemRecipe.cs

+                itemManager.SafeGetItemType(_owner.RecipeItem1),
+                itemManager.SafeGetItemType(_owner.RecipeItem2),
+                itemManager.SafeGetItemType(_owner.RecipeItem3),
+                itemManager.SafeGetItemType(_owner.RecipeItem4)


also use array and loop.

molenzwiebel

For now requesting changes because of the recipe and argument name. Do also check if LINQ can make your code cleaner though.

Other than that, what is the memory usage of loading all game data upon startup? I understand that it makes the rest easier, but isn't ZIP a random access format (meaning you should be able to just store offsets/lengths in your dictionary and read from file on demand)? I won't make you change all the stuff, but keep it in mind.

molenzwiebel · 2018-10-05T17:36:28Z

GameServerLib/Content/ContentManager.cs

                {
-                    contents.Add(directory.Replace('\\', '/').Split('/').Last());
+                    return path;


Is there no faster LINQ way to do this? possibilities.First or something?

GameServerLib/Content/ContentManager.cs

molenzwiebel · 2018-10-05T17:37:07Z

GameServerLib/Content/ContentManager.cs

-                throw new ContentNotFoundException($"Spell data for {spellName} was not found.");
+                if (Content.ContainsKey(path))
+                {
+                    return path;


molenzwiebel · 2018-10-05T17:39:16Z

GameServerLib/Items/ItemData.cs

+        public int RecipeItem1 { get; private set; }
+        public int RecipeItem2 { get; private set; }
+        public int RecipeItem3 { get; private set; }
+        public int RecipeItem4 { get; private set; }


Yep, there doesn't seem to be a good reason to limit this to just 4 items. My vote also goes out to arrays (for potential custom content in the future).

molenzwiebel · 2018-10-05T17:40:22Z

GameServerLib/Items/ItemRecipe.cs

+                if (item != null)
+                {
+                    _totalPrice += item.TotalPrice;
+                }


This most likely can also be a LINQ sum

molenzwiebel · 2018-10-05T17:40:51Z

GameServerLib/Items/Shop.cs

            _game.PacketNotifier.NotifyRemoveItem(_owner, slotId, stackSize);
            _owner.RemoveSpell((byte)(slotId + ITEM_ACTIVE_OFFSET));
            inventory.RemoveItem(item);
        }

-        private bool AddItem(ItemType itemType)
+        private bool AddItem(ItemData ıtemData)


Consider using the proper normal english i here 😋

moonshadow565

While i went through your changes i noticed that you also changed some enums and while i'm pretty sure that those changes have nothing to do with .zip support it can be overlooked.

That being said i can't help but notice that the way you implemented this is by loading all file contents in memory as bytes while checking if .zip file is present.
I don't think this is acceptable solution even for a short term.
Aside from performance issues this will cause, it is not really clear how this interacts with multiple packages providing the same file and doesn't provide any mechanism for possible further exstensions.

It would be shame to just drop this PR because of other changes that you made to organize content loading.

moonshadow565 · 2018-10-10T20:19:31Z

GameServerLib/Content/FileEntry.cs

+
+        private byte[] _data;
+
+        public FileEntry(string name, string fullPath, Stream stream)


This is still storing full content of the file into memory.
However this time you did define a class that describes an interface for a FileEntry.

You should extract this interface so that you can have multiple implementations for FileEntry(example .zip or file system).
And yes this would mean that your ContentManager would have to implement IDisposable to get rid of open .zip handles which should be trivial to implement(store them in list).

So what you should implement is FilesystemFileEntry which should be wraper for IFilerEntry around this class:
https://docs.microsoft.com/en-us/dotnet/api/system.io.fileinfo?view=netframework-4.7.2

For ZipFileEntry you can wrap this class(ZipArchiveEntry):
https://docs.microsoft.com/en-us/dotnet/standard/io/how-to-compress-and-extract-files

Both of those provide you a way to:

File name

File name relative to root path of package

Open or OpenRead which returns a stream

FurkanS1821 mentioned this pull request Oct 1, 2018

Zip LeagueSandbox/LeagueSandbox-Default#84

Open

FurkanS1821 commented Oct 1, 2018

View reviewed changes

Zip support

760a97c

FurkanS1821 force-pushed the Zip branch from 3d5645c to 760a97c Compare October 3, 2018 17:24

FurkanS1821 requested a review from a team October 3, 2018 17:28

danil179 reviewed Oct 4, 2018

View reviewed changes

molenzwiebel suggested changes Oct 5, 2018

View reviewed changes

Fixed requests

322b525

FurkanS1821 force-pushed the Zip branch from ac7d751 to e7e1cf5 Compare October 5, 2018 20:36

Even more changes

c40c71b

FurkanS1821 force-pushed the Zip branch from e7e1cf5 to c40c71b Compare October 6, 2018 09:43

Idk what I changed either

a3b5ae7

moonshadow565 suggested changes Oct 8, 2018

View reviewed changes

FurkanS1821 added 2 commits October 10, 2018 22:44

Changed according to moon's requests

80b3f67

try/finally doesn't handle exceptions you dumbass

3678cdf

moonshadow565 suggested changes Oct 10, 2018

View reviewed changes

danil179 mentioned this pull request Nov 22, 2018

Resolve #750 - Remove Explicit Interface Implementations #751

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Zip #745

Zip #745

FurkanS1821 commented Oct 1, 2018 •

edited

Loading

FurkanS1821 Oct 1, 2018

codecov-io commented Oct 3, 2018 •

edited

Loading

danil179 left a comment

danil179 Oct 4, 2018

molenzwiebel Oct 5, 2018

danil179 Oct 4, 2018

molenzwiebel left a comment

molenzwiebel Oct 5, 2018

molenzwiebel Oct 5, 2018

molenzwiebel Oct 5, 2018

molenzwiebel Oct 5, 2018

molenzwiebel Oct 5, 2018

moonshadow565 left a comment

moonshadow565 Oct 10, 2018


		private byte[] _data;

		public FileEntry(string name, string fullPath, Stream stream)

Zip #745

Are you sure you want to change the base?

Zip #745

Conversation

FurkanS1821 commented Oct 1, 2018 • edited Loading

Choose a reason for hiding this comment

codecov-io commented Oct 3, 2018 • edited Loading

Codecov Report

danil179 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

molenzwiebel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moonshadow565 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FurkanS1821 commented Oct 1, 2018 •

edited

Loading

codecov-io commented Oct 3, 2018 •

edited

Loading