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

Support loading sprites from PNG sheets with embedded frame metadata #15336

Merged
merged 1 commit into from Sep 29, 2018

Conversation

Projects
None yet
6 participants
@IceReaper
Copy link
Contributor

IceReaper commented Jul 9, 2018

Implemented a simple loader for PNG based sequences.
Information about frame sizes, number of frames and the Offset is read from PNG metadata properties if available.
These properties can simply be edited with available PNG tools (there are a lot, i used this one: http://entropymine.com/jason/tweakpng/ ).

The easy Method A:
If no Offset is set, the offset will be defaulted to the frame center.
If FrameSize is set, but no FrameAmount is set, it will auto calculate FrameAmount
If FrameAmount is set, but no FrameSize is set, it will slice the sheet horizontaly and auto set the FrameSize.
if no FrameAmount and no FrameSize is set, it will assume the whole sheet is a single frame.

The complexer but optimized Method B:
For each frame, a key Frame[0] is required. however 0 will be 1 for the second frame and so on.
The value is a comma seperated list in this format: x,y,width,height,offsetX,offsetY. This allows to select a particular region in the sheet, without the need of adding transparent rows / columns for performance reasons.

Additionaly PaletteFromPng can be used to load a palette from a png file. If your png's share a single palette, you can do this once and reuse it as usual. Otherwise you can load palettes for a single png too, but that requires additional palette rules ofcourse ;)

The core reason is to allow modders to not struggle with Westwood file formats and their restrictions (like 6 bits per color channel palettes) and simply load PNG based assets for mods. Also for modders, this makes their assets visible on GitHub (example: https://github.com/IceReaper/OpenRA/blob/d81998185072eb0e0eb60d3246b3d237b55c1727/mods/cnc/bits/bunny.png ), and allow to use image diff tools in combination with git. So basically: This PR is for mod-friendliness :)

Ingame: https://imgur.com/3XYkTUB

Example: #15800

Closes #5075.

@IceReaper

This comment has been minimized.

Copy link
Contributor

IceReaper commented Jul 9, 2018

Ideas of future features:

Utility PngSheetApplyPaletteCommand
This will first check wether the png applies to a specific palette (uses only colors present in palette). If it does, it will replace the palette and reindex the pixels. However, this requires decoding and re-encoding of IDAT pixels chunks, so not part of this PR.

@IceReaper IceReaper force-pushed the IceReaper:png_spritesheets branch from 13c04b7 to ada62ee Jul 9, 2018

Show resolved Hide resolved mods/cnc/mod.yaml Outdated
@Mailaender

This comment has been minimized.

Copy link
Member

Mailaender commented Jul 10, 2018

We could use this for the official mods as well and convert https://github.com/OpenRA/OpenRA/tree/bleed/mods/ra/bits to PNG.

@IceReaper IceReaper force-pushed the IceReaper:png_spritesheets branch 2 times, most recently from b6f13e2 to d819981 Jul 12, 2018

@IceReaper

This comment has been minimized.

Copy link
Contributor

IceReaper commented Jul 13, 2018

Updated PR, the description, and added a preview gif ;)

@IceReaper IceReaper force-pushed the IceReaper:png_spritesheets branch 4 times, most recently from 66b46f1 to 7b50773 Jul 13, 2018

@IceReaper

This comment has been minimized.

Copy link
Contributor

IceReaper commented Jul 13, 2018

And as this might be the only way to reach you @pchote i added png <-> yaml metadata utility commands, as you suggested.

@IceReaper IceReaper force-pushed the IceReaper:png_spritesheets branch 4 times, most recently from 5e758c1 to ed21666 Jul 22, 2018

@drogoganor

This comment has been minimized.

Copy link
Contributor

drogoganor commented Jul 29, 2018

I'd really like this one. Nice work IceReaper.

@IceReaper IceReaper force-pushed the IceReaper:png_spritesheets branch from ed21666 to 919294a Jul 29, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Aug 4, 2018

I want this for Next+1 (and in bleed sooner rather than later).

@reaperrr reaperrr added this to the Next + 1 milestone Aug 4, 2018

@drogoganor

This comment has been minimized.

Copy link
Contributor

drogoganor commented Aug 10, 2018

About the name of IPaletteLoader - I think this might be a little too close to ILoadsPalettes. Maybe change it to IReadsPalette since the method is ReadPalette(...)?

Otherwise I've merged this into my DR branch and the cursor palette side of things works as advertised.

Syntax of my cursor.yaml is:

Palettes:
	cursor: units.pal

PaletteLoaders:
	cursor: JascPalette

[...]

Actually this IPaletteLoader is also needed for PaletteFromGimpOrJascFile, but I will make those changes in a separate PR because this one is large enough.

@drogoganor

This comment has been minimized.

Copy link
Contributor

drogoganor commented Aug 10, 2018

I was also successful in using PngSheet. I altered the PNG metadata using TweakPNG as follows:

pngmetadata

As you can see the Offset, FrameSize, and FrameAmount are added as tEXt blocks.

@IceReaper IceReaper force-pushed the IceReaper:png_spritesheets branch from 919294a to 6d95522 Aug 15, 2018

@IceReaper

This comment has been minimized.

Copy link
Contributor

IceReaper commented Aug 15, 2018

Rebased.

@IceReaper IceReaper force-pushed the IceReaper:png_spritesheets branch from 6d95522 to 9c2b10b Aug 15, 2018

@Mailaender
Copy link
Member

Mailaender left a comment

The bunnies work as promised and this seems to be a very complete implementation. Thanks!

@pchote
Copy link
Member

pchote left a comment

Here's a first cut, focusing mainly on code style. As discussed on IRC before I feel bad about leaving this so long, but couldn't be avoided.


namespace OpenRA.FileFormats
{
public class Png

This comment has been minimized.

@pchote

pchote Sep 23, 2018

Member

Can you please move this to OpenRA.Mods.Common.FileFormats? This doesn't appear to be used anywhere in OpenRA.Game.

This comment has been minimized.

@pchote

pchote Sep 23, 2018

Member

There seems to be something weird going on with the encoding on this file - the indentation displayed by GitHub is inconsistent with the normal formatting.

This comment has been minimized.

@IceReaper

IceReaper Sep 23, 2018

Contributor

I was thinking about that too, but i thought png is a so standard format, that it would almost be an engine specific feature, while common ist mostly the rts core module. But i dont mind, sure i can move it.

/*var filter = */cr.ReadByte();
var interlace = cr.ReadByte();

if (compression != 0) throw new InvalidDataException("Compression method not supported");

This comment has been minimized.

@pchote

pchote Sep 23, 2018

Member

Style nit: these throws should ideally be on the line below.

throw new InvalidDataException("Invalid PNG file - header does not appear first.");

using (var ms = new MemoryStream(content))
using (var cr = new BinaryReader(ms))

This comment has been minimized.

@pchote

pchote Sep 23, 2018

Member

Style nit: add braces around here to avoid a hanging block

This comment has been minimized.

@pchote

pchote Sep 23, 2018

Member

Would be good if we could drop the BinaryReader here too.

switch (type)
{
case "IHDR":
{

This comment has been minimized.

@pchote

pchote Sep 23, 2018

Member

Style nit: to improve readability we prefer to indent switch cases like so:

switch (type)
{
	case "IHDR":
	{
		if (headerParsed)
			/* etc */
	}
}

(minus one indentation level on the braces)

s.Position += 8;
var headerParsed = false;

using (var br = new BinaryReader(s))

This comment has been minimized.

@pchote

pchote Sep 23, 2018

Member

Would you mind replacing the BinaryReader with our stream extensions here instead?

public readonly string Tileset = null;

[FieldLoader.Require]
[Desc("filename to load")]

This comment has been minimized.

@pchote

pchote Sep 23, 2018

Member

Capitalization


for (var y = 0; y < png.Height; y++)
for (var x = 0; x < png.Width; x++)
bitmap.SetPixel(x, y, png.Palette[png.Data[x + y * png.Width]]);

This comment has been minimized.

@pchote

pchote Sep 23, 2018

Member

SetPixel is realllly slow. Can you please copy over the low level LockBits / Scan0 etc code from the original file?

{
var s = File.OpenRead(args[1]);
var png = new Png(s);
s.Close();

This comment has been minimized.

@pchote

pchote Sep 23, 2018

Member

Should be able to use a using block to drop some of this manual management

case "IEND":
rs.Close();

MiniYaml.FromFile(args[1]).ForEach(node =>

This comment has been minimized.

@pchote

pchote Sep 23, 2018

Member

OpenRA code style is to prefer proper foreach blocks over the LINQ ForEach

var rs = File.OpenRead(args[2]);
var ws = new MemoryStream();

using (var br = new BinaryReader(rs))

This comment has been minimized.

@pchote

pchote Sep 23, 2018

Member

Again, prefer our stream exts over BinaryReader

@IceReaper IceReaper force-pushed the IceReaper:png_spritesheets branch from 62b075b to af4725d Sep 24, 2018

@IceReaper

This comment has been minimized.

Copy link
Contributor

IceReaper commented Sep 25, 2018

updated :)

@abcdefg30 abcdefg30 removed the PR: Needs +2 label Sep 25, 2018

@IceReaper IceReaper force-pushed the IceReaper:png_spritesheets branch from af4725d to b4bcd1c Sep 26, 2018

@pchote pchote changed the title PNG sequence, palette and cursor integration. Support loading sprites from PNG sheets with embedded frame metadata Sep 28, 2018

@IceReaper IceReaper force-pushed the IceReaper:png_spritesheets branch 6 times, most recently from 308df7b to 474bf3c Sep 28, 2018

@pchote
Copy link
Member

pchote left a comment

Looks good overall, so I expect this to be my last set of major requests here:

public PixelFormat PixelFormat { get; set; }
public Color[] Palette { get; set; }
public byte[] Data { get; set; }
public Dictionary<string, string> Meta = new Dictionary<string, string>();

This comment has been minimized.

@pchote

pchote Sep 28, 2018

Member

Can we please rename this to EmbeddedData or similar? It would be helpful to keep this clearly separate from the sprite metadata that I plan to PR soon.

s.Position += 8;
var headerParsed = false;

using (var br = new BinaryReader(s))

This comment has been minimized.

@pchote

pchote Sep 28, 2018

Member

This BinaryReader can be dropped, and the stream read directly just below.

var key = new List<byte>();
byte b;

while ((b = ms.ReadUInt8()) != 0x00)

This comment has been minimized.

@pchote

pchote Sep 28, 2018

Member

This can simplify down to

case "tEXt":
{
	var key = ms.ReadASCIIZ();
	Meta.Add(key, ms.ReadASCII(length - key.Length - 1));
	break;
}
Show resolved Hide resolved OpenRA.Mods.Common/SpriteLoaders/PngSheetLoader.cs
Data = new byte[frameRegions[i].Width * frameRegions[i].Height]
};

var frameStart = frameRegions[i].X + frameRegions[i].Y * png.Width;

This comment has been minimized.

@pchote

pchote Sep 28, 2018

Member

Can then group this with the frameSize variable above.

{
public class PngSheetImportMetadataCommand : IUtilityCommand
{
string IUtilityCommand.Name { get { return "--PngSheetImport"; } }

This comment has been minimized.

@pchote

pchote Sep 28, 2018

Member

This should be --png-sheet-import to fit the command naming conventions.

{
public class PngSheetExportMetadataCommand : IUtilityCommand
{
string IUtilityCommand.Name { get { return "--PngSheetExport"; } }

This comment has been minimized.

@pchote

pchote Sep 28, 2018

Member

This should be --png-sheet-export to fit the command naming conventions.

using (var s = File.OpenRead(args[1]))
{
var png = new Png(s);
var yaml = "";

This comment has been minimized.

@pchote

pchote Sep 28, 2018

Member

This simplifies to

png.Meta.Select(m => new MiniYamlNode(m.Key, m.Value))
	.ToList()
	.WriteToFile(args[2]);
return args.Length == 3;
}

[Desc("PNGFILE YAMLFILE", "Export png metadata to yaml")]

This comment has been minimized.

@pchote

pchote Sep 28, 2018

Member

For conversion commands we typically use the same filename as the input but change the extension (so calling on bunny.png would write bunny.yaml)

This comment has been minimized.

@pchote

pchote Sep 29, 2018

Member

Need to remove the YAMLFILE here

[Desc("YAMLFILE PNGFILE", "Import yaml metadata to png")]
void IUtilityCommand.Run(Utility utility, string[] args)
{
// This could eventually be merged into the Png class (add a Write method), however this requires complex png writing algorythms.

This comment has been minimized.

@pchote

pchote Sep 28, 2018

Member

I think this justifies a // HACK: prefix 😄

@IceReaper IceReaper force-pushed the IceReaper:png_spritesheets branch 3 times, most recently from 0a7c8d2 to 4343c55 Sep 28, 2018

@pchote
Copy link
Member

pchote left a comment

LGTM, just a couple of remaining minor nits. This might be a good time to remove the testcase too.

if (png.EmbeddedData.ContainsKey("Offset"))
{
var coords = png.EmbeddedData["Offset"].Split(',');
offset = new float2(int.Parse(coords[0]), int.Parse(coords[1]));

This comment has been minimized.

@pchote

pchote Sep 29, 2018

Member

FieldLoader.GetValue<float2>("Offset", coords[1])

else if (png.EmbeddedData.ContainsKey("FrameAmount"))
{
// Otherwise, calculate the number of frames by splitting the image horizontaly by FrameAmount.
frameAmount = int.Parse(png.EmbeddedData["FrameAmount"]);

This comment has been minimized.

@pchote

pchote Sep 29, 2018

Member

FieldLoader.GetValue<int>("FrameAmount", png.EmbeddedData["FrameAmount"])

{
// If FrameSize exist, use it and...
var dimensions = png.EmbeddedData["FrameSize"].Split(',');
frameSize = new Size(int.Parse(dimensions[0]), int.Parse(dimensions[1]));

This comment has been minimized.

@pchote

pchote Sep 29, 2018

Member

FieldLoader.GetValue<Size>("FrameSize", png.EmbeddedData["FrameSize"])


// ... either use FrameAmount or calculate how many times FrameSize fits into the image.
if (png.EmbeddedData.ContainsKey("FrameAmount"))
frameAmount = int.Parse(png.EmbeddedData["FrameAmount"]);

This comment has been minimized.

@pchote

pchote Sep 29, 2018

Member

FieldLoader.GetValue<int>("FrameAmount", png.EmbeddedData["FrameAmount"])

Show resolved Hide resolved OpenRA.Mods.Common/UtilityCommands/ConvertPngToShpCommand.cs
return args.Length == 3;
}

[Desc("PNGFILE YAMLFILE", "Export png metadata to yaml")]

This comment has been minimized.

@pchote

pchote Sep 29, 2018

Member

Need to remove the YAMLFILE here

[Desc("PNGFILE YAMLFILE", "Export png metadata to yaml")]
void IUtilityCommand.Run(Utility utility, string[] args)
{
using (var s = File.OpenRead(args[1] + ".png"))

This comment has been minimized.

@pchote

pchote Sep 29, 2018

Member

Please keep the extension in the arg, then you can use System.IO.Path.ChangeExtension to switch it to yaml.

@IceReaper IceReaper force-pushed the IceReaper:png_spritesheets branch from 4343c55 to 779061f Sep 29, 2018

PNG spritesheet support, along with PaletteFromPng.
Cursor palette loader can now be specified via yaml.

@IceReaper IceReaper force-pushed the IceReaper:png_spritesheets branch from 779061f to 7ea73cf Sep 29, 2018

@pchote

pchote approved these changes Sep 29, 2018

@pchote pchote added the PR: Needs +2 label Sep 29, 2018

@reaperrr
Copy link
Contributor

reaperrr left a comment

Code is out of my league, but I still had the bunny testcase locally, tested with the latest updated code and still works fine, so 👍

@reaperrr reaperrr merged commit 693b5a5 into OpenRA:bleed Sep 29, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@IceReaper IceReaper deleted the IceReaper:png_spritesheets branch Sep 29, 2018

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