Dat reader - creating a PR so I can comment. #235
Conversation
Source/ACE.DatLoader/DatReader.cs
Outdated
| public int Offset { get; set; } | ||
| private byte[] buffer; | ||
|
|
||
| public DatReader(string datFilePath, uint offset, uint size, uint sectorSize) |
There was a problem hiding this comment.
I think I would rather see DatDatabase return a DatReader when given a file Id. For example, in DatDatabase:
public DatReader GetReaderForFile(uint fileId)
The design paradigm here is that you've already loaded a DatDatabase, so you shouldn't have to go fishing around in objects to find the properties to pass into here. DatDatabase already has them all, you just need to give it a file Id.
Source/ACE.DatLoader/DatReader.cs
Outdated
| size = 0; | ||
| } | ||
| else | ||
| { |
There was a problem hiding this comment.
I'd really like to see this block commented fairly heavily. It took me several minutes of staring at it to grok what it is doing, where comments would have taken 20 seconds to read to get the explanation.
Source/ACE.DatLoader/DatReader.cs
Outdated
| this.buffer = new byte[size]; | ||
| stream.Seek(offset, SeekOrigin.Begin); | ||
|
|
||
| if (size > sectorSize) |
There was a problem hiding this comment.
So, I know what this does because I wrote the original DAT stuff, but this is totally non-obvious to someone who didn't. Just put a comment as to why this is the right thing to do.
|
|
||
| namespace ACE.DatLoader.Entity | ||
| { | ||
| public class AnimationPartChange |
There was a problem hiding this comment.
// TODO: Refactor to merge with existing AnimationOverride object in ACE.
Source/ACE.DatLoader/Entity/Loc.cs
Outdated
|
|
||
| namespace ACE.DatLoader.Entity | ||
| { | ||
| public class Loc |
There was a problem hiding this comment.
// TODO: refactor to use existing Position object
| public List<List<Loc>> StarterAreas { get; set; } = new List<List<Loc>>(); | ||
| public Dictionary<int, HeritageGroupCG> HeritageGroups { get; set; } = new Dictionary<int, HeritageGroupCG>(); | ||
|
|
||
| public static CharGen ReadFromDat(string datFilePath, uint offset, uint size, uint sectorSize) |
There was a problem hiding this comment.
Is CharGen essentially a singleton? That is, there's only ever one of them? If so, just leave this as a todo:
// TODO: refactor into a singleton pattern
| using System.Collections.Generic; | ||
| namespace ACE.DatLoader.Entity | ||
| { | ||
| /* These are client_portal.dat files starting with 0x24------ */ |
There was a problem hiding this comment.
use a ///
tag for this please
|
|
||
| namespace ACE.DatLoader.Entity | ||
| { | ||
| /* These are client_portal.dat files starting with 0x02------ */ |
There was a problem hiding this comment.
/// <summary>
/// These are client_portal.dat files starting with 0x02
/// </summary>
| @@ -15,16 +15,18 @@ public PortalDatDatabase(string filename) : base(filename, DatDatabaseType.Porta | |||
|
|
|||
| public void ExtractCategorizedContents(string path) | |||
| { | |||
| string thisFolder = null; | |||
| // string thisFolder = null; | |||
There was a problem hiding this comment.
just remove the methods if you're going to fully comment them out. it's ok.
| @@ -288,6 +291,54 @@ public void Kill() | |||
|
|
|||
| Location = character.Location; | |||
|
|
|||
| // TODO: Move this all into Character Creation and store directly in the database. | |||
| if (DatManager.PortalDat.AllFiles.ContainsKey(0x0E000002)) | |||
There was a problem hiding this comment.
that is in Character.CreateFromClientFragment
Source/ACE.DatLoader/DatReader.cs
Outdated
| public class DatReader | ||
| { | ||
| public int Offset { get; set; } | ||
| public byte[] buffer { get; private set; } |
There was a problem hiding this comment.
Stylecop rules dictate that we should use uppercase for the first letter of this name: ex. Buffer
There was a problem hiding this comment.
Thanks! It was private previously, but I exposed the getter so I could still export the content to a file using the console "portal-export" command.
| cg.Did = datReader.ReadInt32(); | ||
| datReader.Offset = 8; | ||
|
|
||
| /* STARTER AREA */ |
There was a problem hiding this comment.
Multi-line comment style is not really necessary for single line comments, but I do like how this shows separate logic sections.
You can use /// for a lot of metadata in comments that will be visible within visual studio.
| </ItemGroup> | ||
| <ItemGroup> | ||
| <None Include="app.config" /> | ||
| <None Include="packages.config" /> | ||
| </ItemGroup> | ||
| <ItemGroup /> |
There was a problem hiding this comment.
Yeah, won't hurt anything, though.
There was a problem hiding this comment.
Not sure what/how that got there. Something Visual Studio did for me, apparently. I'll remove it just to keep things clean, though.
There was a problem hiding this comment.
Initial testing and review proves that character generation is working great!
-
I did not test the starting locations.
-
There was some a couple CR:LF messages on a couple files,
-
My eye could not notice any visual discrepancies from character generation, though I cannot confirm all character appearances are correct due to my own limitations - please have another eye check to for visual issues or motion table problems.
-
Testing has shown that this code consistently produces the correct player model on login.
…ly. Also added some additional comments.
|
Just pulled the latest changes and tested again; all functions were working as expected. |
No description provided.