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

Implemented .ru Viewer #28

Merged
merged 12 commits into from Jan 3, 2018

Conversation

Projects
None yet
2 participants
@feliwir
Collaborator

feliwir commented Jan 2, 2018

  • many additions to the Apt system
  • Added triangle rendering to Graphics2D
@feliwir

This comment has been minimized.

Collaborator

feliwir commented Jan 2, 2018

Here is an example for a displayed .ru file.
image

Those shapes are later combined and animated in the .apt format.

@tgjones

This looks great, thank you! My comments are only regarding style and some minor things. The actual functionality looks good to me.

@@ -3,6 +3,7 @@
<TargetFramework>net462</TargetFramework>
<ApplicationIcon>Resources\AppIcon.ico</ApplicationIcon>
<LangVersion>latest</LangVersion>
<Platforms>AnyCPU</Platforms>

This comment has been minimized.

@tgjones

tgjones Jan 2, 2018

Collaborator

Is this change necessary?

This comment has been minimized.

@feliwir

feliwir Jan 2, 2018

Collaborator

No idea where it came from

namespace OpenSage.Content
{
class AptLoader

This comment has been minimized.

@tgjones

tgjones Jan 2, 2018

Collaborator

Will this be filled in later? Should it be removed for now?

This comment has been minimized.

@feliwir

feliwir Jan 2, 2018

Collaborator

Will be filled

{
case SageGame.CncGenerals:
case SageGame.CncGeneralsZeroHour:
contentManager.IniDataContext.LoadIniFile(@"Data\English\HeaderTemplate.ini");

This comment has been minimized.

@tgjones

tgjones Jan 2, 2018

Collaborator

Are header templates and mapped images really necesary for .ru? I guess this is left in from WindowLoader.

This comment has been minimized.

@feliwir

feliwir Jan 2, 2018

Collaborator

Yes, they are left. Will remove them

tris.Clear();
break;
case GeometryStyle.SolidTri:
geometry.Entries.Add(new GeometrySolidTriangles(tris, color));

This comment has been minimized.

@tgjones

tgjones Jan 2, 2018

Collaborator

I think it would be cleaner to pass new List<Triangle2D>(tris) here, rather than doing it in the GeometrySolidTriangles constructor.

This comment has been minimized.

@feliwir

feliwir Jan 2, 2018

Collaborator

Will do

tris.Clear();
break;
case GeometryStyle.Line:
geometry.Entries.Add(new GeometryLines(lines, color, thickness));

This comment has been minimized.

@tgjones

tgjones Jan 2, 2018

Collaborator

Same here - I think it would be cleaner to pass new List<Line2D>(lines) here.

foreach (var e in Shape.Entries)
{
if(e is GeometryLines l)

This comment has been minimized.

@tgjones

tgjones Jan 2, 2018

Collaborator

This could be replaced with a switch/case:

switch (entry)
{
  case GeometryLines l:
    break;

  // etc.
}
@@ -16,7 +19,8 @@ private void PlatformConstruct(GraphicsDevice2D graphicsDevice, Texture targetTe
var dxgiSurface = targetTexture.DeviceResource.QueryInterface<Surface>();
_bitmap = AddDisposable(new Bitmap1(graphicsDevice.DeviceContext, dxgiSurface));
_bitmap = AddDisposable(new Bitmap1(graphicsDevice.DeviceContext, dxgiSurface));

This comment has been minimized.

@tgjones

tgjones Jan 2, 2018

Collaborator

Somehow we lost a space here...

}
}
private void PlatformFillTriangle(in RawTriangleF tri, in Texture tex, in RawMatrix3x2 transform)

This comment has been minimized.

@tgjones

tgjones Jan 2, 2018

Collaborator

Please remove the in modifier from the tex parameter - it's only useful on struct parameters. For reference parameters it's already the default. And please could you use full names, like texture instead of tex?

PlatformFillTriangle(tri, fillColor);
}
public void FillTriangle(in RawTriangleF tri, in Texture texture,RawMatrix3x2 transform)

This comment has been minimized.

@tgjones

tgjones Jan 2, 2018

Collaborator

Please remove the in modifier from the texture parameter.

@@ -13,34 +13,56 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "OpenSage.DataViewer", "Open
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "OpenSage.DataViewer.Windows", "OpenSage.DataViewer.Windows\OpenSage.DataViewer.Windows.csproj", "{1D432BD6-B14F-4008-88B3-E3DC52AC7380}"
EndProject
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "OpenSage.LowLevel", "OpenSage.LowLevel\OpenSage.LowLevel.csproj", "{AA6CCAEB-CC90-4463-A67E-336580BCD036}"
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "OpenSage.LowLevel", "OpenSage.LowLevel\OpenSage.LowLevel.csproj", "{AA6CCAEB-CC90-4463-A67E-336580BCD036}"

This comment has been minimized.

@tgjones

tgjones Jan 2, 2018

Collaborator

Are any of these .sln changes necessary? If not please could you revert them?

This comment has been minimized.

@feliwir

feliwir Jan 2, 2018

Collaborator

Not sure where those came from, will change

tris.Clear();
break;
case GeometryStyle.Line:
geometry.Entries.Add(new GeometryLines(lines, color, thickness));

This comment has been minimized.

@tgjones

tgjones Jan 3, 2018

Collaborator

This should be new List<Line2D>(lines), instead of doing it inside the constructor?

return new SharpDX.Mathematics.Interop.RawMatrix3x2(m.M11, m.M12, m.M21, m.M22, m.M31, m.M32);
}
private static SharpDX.Mathematics.Interop.RawVector2 ToRawVector2(in float x, in float y)

This comment has been minimized.

@tgjones

tgjones Jan 3, 2018

Collaborator

No need for in on primitive types - on a 64-bit platform, it will actually end up using more memory (64-bit pointer vs 32-bit float).

@tgjones

tgjones approved these changes Jan 3, 2018

@tgjones tgjones merged commit 8bfc0f4 into OpenSAGE:master Jan 3, 2018

@tgjones

This comment has been minimized.

Collaborator

tgjones commented Jan 3, 2018

Thank you for this! Looking forward to the next one :)

@tgjones tgjones added this to the v0.1.0 milestone Apr 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment