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

Adaptive delta animations #152

Merged
merged 8 commits into from
Nov 11, 2018
Merged

Adaptive delta animations #152

merged 8 commits into from
Nov 11, 2018

Conversation

feliwir
Copy link
Contributor

@feliwir feliwir commented Nov 10, 2018

Working for some BFME II models. Need to get some issues resolved before merging this.

@feliwir feliwir requested a review from tgjones November 10, 2018 13:47
@codecov-io
Copy link

codecov-io commented Nov 10, 2018

Codecov Report

Merging #152 into master will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #152      +/-   ##
=========================================
- Coverage    9.21%   9.16%   -0.05%     
=========================================
  Files         798     801       +3     
  Lines       27589   27710     +121     
=========================================
  Hits         2541    2541              
- Misses      25048   25169     +121
Impacted Files Coverage Δ
....Game/Data/W3d/W3dAdaptiveDeltaAnimationChannel.cs 0% <0%> (ø) ⬆️
....Launcher/src/OpenSage.Game/Content/ModelLoader.cs 0% <0%> (ø) ⬆️
...age.Game/Data/W3d/W3dMotionChannelTimeCodedData.cs 0% <0%> (ø) ⬆️
...her/src/OpenSage.Game/Data/W3d/W3dMotionChannel.cs 0% <0%> (ø) ⬆️
...Game/Data/W3d/W3dMotionChannelAdaptiveDeltaData.cs 0% <0%> (ø)
...src/OpenSage.Game/Data/W3d/W3dMotionChannelData.cs 0% <0%> (ø)
...her/src/OpenSage.Game/Data/W3d/W3dAdaptiveDelta.cs 0% <0%> (ø)

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 4e60810...0c75a8e. Read the comment docs.

@feliwir feliwir changed the title WIP: Adaptive delta Adaptive delta animations Nov 11, 2018
@feliwir
Copy link
Contributor Author

feliwir commented Nov 11, 2018

Fixed all the remaining bugs. Ready for review/merge

private static AnimationClip CreateAnimationClip(W3dCompressedAnimation w3dAnimation, W3dMotionChannel w3dChannel)
{
var bone = w3dChannel.Pivot;
var data = w3dChannel.Data;
Copy link
Contributor

Choose a reason for hiding this comment

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

Unused variable.

src/OpenSage.Game/Content/ModelLoader.cs Show resolved Hide resolved
@@ -542,5 +542,17 @@ public static Matrix2x2 ReadMatrix2x2(this BinaryReader reader)
M22 = reader.ReadSingle()
};
}

public static float[] ReadSingleArray(this BinaryReader reader, int len)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is never used.

{
class W3dAdaptiveDelta
{
private static float[] Table = CalculateTable();
Copy link
Contributor

Choose a reason for hiding this comment

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

Mark this as readonly.

}
else if (nBits == 8)
{
var val = (byte)deltaBytes[i];
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: space between cast and value.

result.Scale,
nBits);

reader.ReadBytes(3);
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace with reader.BaseStream.Seek(3, SeekOrigin.Current); to remove an unnecessary array allocation.

Could you add a comment why you're skipping 3 bytes?

@@ -49,12 +49,12 @@ public static W3dMotionChannel Parse(BinaryReader reader, uint chunkSize)

case W3dMotionChannelDeltaType.Delta4:
// TODO
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these 2 still TODO if we can now decode them?

@@ -0,0 +1,34 @@
using System;
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove unused usings.

@feliwir feliwir merged commit 37a569e into OpenSAGE:master Nov 11, 2018
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.

4 participants