-
Notifications
You must be signed in to change notification settings - Fork 241
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
Added Animation parsing from the client_portal.dat and all relevant hooks and properties. #495
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dude, you've learned a lot! This is excellent usage of objects and well structured. I've left some comments to help take that next step, but if you don't have time to get to them I can take this as is. Just let me know :)
uint numHooks = datReader.ReadUInt32(); | ||
for (uint i = 0; i < numHooks; i++) | ||
{ | ||
AnimationHook ah = new AnimationHook(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the "new AnimationHook()" isn't necessary. You could shorten these 3 lines to:
a.Hooks.Add(AnimationHook.Read(datReader));
public AnimationHookType HookType { get; set; } | ||
public uint Direction { get; set; } | ||
private IHook _hook = null; | ||
public IHook Hook { get { return _hook; } set { _hook = value; } } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should probably be "private set". The reason being that this class should be the only place that hook value is ever set, and everybody else strictly reads it.
{ | ||
public uint PES { get; set; } | ||
public float Pause { get; set; } | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
both of these should be "private set". That's going to apply to almost all of these hook sub-types so I won't spam you with comments about it. Reason being is that no other class should be changing these values.
ReplaceObjectHook r = new ReplaceObjectHook(); | ||
|
||
AnimationPartChange ao = new AnimationPartChange(); | ||
ao.PartIndex = (byte)datReader.ReadUInt16(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it would be worth adding a "Read(DatReader datReader)" method to AnimationPartChange here? Seems like it would make sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't bother, since long term the goal is to remove this class and replace with the AnimationOverride object. I've got that on my to-do list (there's a few places this is being used currently that can be replaced so we just have the one object).
Perhaps an extension, like you suggested with the Positions, would a better solution.
hook.PartIndex = datReader.ReadUInt32(); | ||
|
||
Position p = new Position(); | ||
p.PositionX = datReader.ReadSingle(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider making an extension method for Position.Read(DatReader datReader) to reduce redundancy in reading these 7 singles. You can put this class in the DatLoader project somewhere, and then call it like you would any other method on Position.
public static class PositionExtensions
{
public static Position ReadPosition(DatReader datReader)
{
Position p = new Position();
p.Read(datReader);
return p;
}
public static void Read(this Position p, DatReader datReader)
{
p.PositionX = datReader.ReadSingle();
p.PositionY = datReader.ReadSingle();
p.PositionZ = datReader.ReadSingle();
p.RotationW = datReader.ReadSingle();
p.RotationX = datReader.ReadSingle();
p.RotationY = datReader.ReadSingle();
p.RotationZ = datReader.ReadSingle();
}
}
You could then call:
hook.Offset = PositionExtensions.ReadPosition(datReader);
{ | ||
SetOmegaHook so = new SetOmegaHook(); | ||
AceVector3 v = new AceVector3(datReader.ReadSingle(), datReader.ReadSingle(), datReader.ReadSingle()); | ||
so.Axis = v; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so.Axis = new AceVector3(...) would be easier to read. No biggie.
AttackHook a = new AttackHook(); | ||
AttackCone ac = new AttackCone(); | ||
|
||
ac.PartIndex = datReader.ReadUInt32(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider giving AttackCode a "Read(DatReader datReader)" method, and then making AttackCone's property setters private.
{ | ||
for (uint i = 0; i < a.NumFrames; i++) | ||
{ | ||
Position p = new Position(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You would be able to reuse that Position code block I did here as well.
a.PosFrames.Add(PositionExtensions.Read(datReader));
@OptimShi dude! Seriously great work! |
All feedback implemented except the one I noted. Thanks for the help with this, I've learned a little bit about some C# workings--which is always good! |
No description provided.