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

Improve TextureAtlas parsing, store name/value pairs per region #6316

Merged
merged 27 commits into from Dec 20, 2020
Merged

Improve TextureAtlas parsing, store name/value pairs per region #6316

merged 27 commits into from Dec 20, 2020

Conversation

NathanSweet
Copy link
Member

@NathanSweet NathanSweet commented Dec 17, 2020

TextureAtlas parsing is quite brittle because it relies on the order of fields and this makes evolving the format difficult. The first part of this PR is to use the names in the data when parsing. This allows fields to be omitted when they have default values, for example when not using indexes we don't need to write index: -1 for every region.

Next and the main reason for this PR, there are use cases to store additional data per region. For example, for a series of images that make up a frame-by-frame animation, the origin is needed to know how to position the character, rotate it around it's feet, etc. This can vary from animation to animation. Another use case is when exporting skeletal animation as an image sequence, it would be useful to know bone positions and rotations for each frame, eg to add particle effects on top.

I propose storing arbitrary name/value pairs for unrecognized fields for each region in the texture atlas file. The names are String and the values int[]. Since the number of these name/value pairs is likely low and there are many regions, we could use a String[] and matching int[][] rather than an actual map per region. When there are no name/value pairs, these fields are null.

We currently have splits and pads fields on AtlasRegion. It feels a little bad to have many extraneous fields that often are not used for many regions. We could move those two fields into the name/value pairs and this initial PR does that. The fields were only used in a few places and are only needed for initialization, not every frame.

Besides the use cases above, knowing the origin could also be used by AtlasSprite. Tools such as Texture Packer Pro support choosing a pivot point and could write that data. So far this PR doesn't make use of the origin, to do so we'd need to decide:

  1. if it is stored as a percentage or in pixels. Since Sprite uses pixels, that may be a reasonable choice.
  2. if it should be stored in originX and originY fields on AtlasRegion or in the name/value pairs. I lean toward the name/value pairs, like splits and pads, rather than adding fields that are often unused.

This PR also adds a pma field for each page. We could use this in the unpacker to unpremultiply the alpha. I didn't bother to add name/value pairs to each page, as I'm not sure there are use cases that need it.

This PR can still read all previously created texture atlas files. However, note that since the texture packer now omits fields that older TextureAtlas code won't be able to read the new atlas files.

…ue pairs. Moved split and pad to name/value pairs. Changed TexturePacker to omit entries that are default values and write a pma entry for each page.
@obigu
Copy link
Contributor

obigu commented Dec 17, 2020

I like the spirit of the change, it makes the atlas format more flexible and smarter.

I have not yet tested it yet (I will) but design wise I have a concern related to atlas file format especification and validation.

Before the change validation of correctness was not great and seemed to be based on reading line by line and, if I'm not wrong, as long as the value was in the expected format it would assign it the the expected field. In case for example 2 name value pairs with the same value format were inverted it would not fail but on the other hand, the fact that the parsing was so rigid was adding some kind of indirect validation (it would fail with missing properties, unexpected formats, etc...). With the changes, correct me if I'm wrong, there's no mechanism in place to validate the file format either and check, for example, that required properties are defined but because it's more flexible, I think validation is more necessary than it was before.

Another related thing with validation is defaults. We can benefit from fields that are defined per region (such as index) but we should chose them carefully. For example, now format is optional and not put in the atlas file (using the libGDX TexturePacker) if it's RGBA8888. In how many places different places is the default value defined (it is not in the TextureAtlas class for example)? What happens if we open this atlas on a 3rd party tool such as Texture Packer PRO, will it default to the correct format? The atlas files are not versioned, what if we wanted to change the default value in the future? Is it worth having optional fields just to save 1 line?

IMO we should formally specify the libGDX atlas file format including which fields are required and which ones are optional as well as default values for those.

@MrStahlfelge MrStahlfelge added core affecting all platforms enhancement labels Dec 17, 2020
@NathanSweet
Copy link
Member Author

Previously parsing ignored the names completely, relied on their order, and required all entries to be present. An error like a duplicate or missing field breaks everything.

The new parsing uses the names, order doesn't matter, and entries are optional. It's true that this makes validation less than before. For example if an important field like size is omitted, defaults are used. For size that would be 0,0. Parsing would succeed and the resulting texture atlas could be used. If an unknown or misspelled field is specified for a region, it goes into the name/value pairs and parsing still succeeds.

Given that texture atlas files are generated and seldom edited by hand, I think it's OK for parsing to not try to catch errors in return for more flexible parsing and name/value pairs. We'll be able to add new region fields in the feature, eg origin, without breaking parsing. Old parsers would put the field into name/value pairs. Note that adding a page field would break parsing, because of this failure:

if (field == null) throw new GdxRuntimeException("Invalid line: " + line);

Maybe we should change this to silently ignore unknown fields? I could go either way. It's nice to not break older parsers with format changes, though not terribly important. I think I'd lean toward ignoring unknown page fields just because it reduces the impact of future format changes.

Regarding defaults, all entries are optional (ie not a parsing error), though it doesn't make sense to omit entries that are required to make a usable atlas, like size. Filter is nearest and wrap is clampToEdge, which are the defaults used by Texture. orig is equal to size (just caught this one!), index is -1, and everything else gets Java's default values. I agree that documenting default values is important, as what happens when an entry is omitted is a part of the format specification and must be followed for all parsers/tools.

Changing the defaults is possible but would mean the format would not be parsed correctly for older parsers in some cases (they'd use the old defaults). I don't think it's worth adding a version number to the format though. We're unlikely to ever change the defaults since changing them has this downside and makes very little difference otherwise.

Is it worth having optional fields just to save 1 line?

Note it's a few lines per region, eg orig, split, pad, and index are often not needed. Still it's true that optional fields are not really a very important feature. They make the format a little smaller (in one test a 10 page atlas went from ~55KB to ~45KB) which means faster parsing but is unlikely to be noticeable. The new parsing gives us optional fields mostly for free and makes it a little nicer when digging through the file to find a region. Including defaults in the format specification is easy so we may as well allow it.

For the Spine documentation I have documented the libgdx atlas format and it's been updated for the changes in this PR:
http://esotericsoftware.com/spine-atlas-format
We could transfer this somewhere to our wiki. The markdown for that page is here:
http://esotericsoftware.com/spine-atlas-format/markdown

@NathanSweet
Copy link
Member Author

NathanSweet commented Dec 17, 2020

Note I've made one more change, adding bounds and offsets entries. We always write xy and size, so they may as well be a single entry. When offset or orig is written almost always the other is too, so also may as well be a single entry. Plus I dislike orig as a name (original size? origin?). The xy, size, offset, and orig parsing is kept so can still read atlas files that use those.

Sorry for the last second change. I don't have any other changes, just fix ups if reviewers find more.

@tommyettinger
Copy link
Member

Well despite all my tiny comment nitpicks, I think this is a good PR, and some of my gargantuan atlas files will be a fair bit smaller as a result. I'm not entirely sure why we have a custom atlas format in the first place, though, or why JSON is unsuitable. There's also an odd issue that I'm unsure about:

	/** Returns all regions with the specified name as sprites, ordered by smallest to largest {@link AtlasRegion#index index}. This
	 * method uses string comparison to find the regions and constructs new sprites, so the result should be cached rather than
	 * calling this method multiple times.
	 * @see #createSprite(String) */
	public Array<Sprite> createSprites (String name) {
//...

In createSprites(), I don't think it actually does order the sprites by index, unless the atlas already has them in order. That method also requires a lot of String comparisons, one per TextureRegion in the atlas, so it gets slower as the atlas gets larger or if more files share a common prefix (such as a parent folder) with the given name. If the result is cached, the performance of createSprites() doesn't matter nearly as much, but then the question is why TextureAtlas can't create a cache with some method it provides. I'm in favor of merging when you feel it's ready.

@NathanSweet
Copy link
Member Author

NathanSweet commented Dec 18, 2020

I'm not entirely sure why we have a custom atlas format in the first place, though, or why JSON is unsuitable.

A line based format is straightfoward and efficient to parse. How would we parse JSON? libgdx's JsonReader can do it, but reading it all into a DOM (JsonValue objects) then copying the data out isn't efficient. JsonReader has methods that can be overridden to do event based parsing, but I'd guess the resulting code wouldn't be pretty (needs to track state so as values are encountered they are stuffed into the right objects).

What benefits would JSON give us? It still needs a parser (DOM or events) and a specification (what JSON data makes up a valid texture atlas). The main benefit would be interoperability with other tools, as they could read it more easily than a line based format. They still need to make sense of the data though, and that has the same problems mentioned above.

I don't think a reasonable format existed back when libgdx added its own. I've not looked for alternative formats recently. FWIW, an alternate loader could be written to populate a TextureAtlasData from a different format. A binary format could be interesting, though probably overkill.

In createSprites(), I don't think it actually does order the sprites by index, unless the atlas already has them in order.

Looks like the regions are sorted after loading:

regions.sort(indexComparator);

That's a little wasteful if the user doesn't care about indexes. Maybe we could skip it in that case (edit: done!).

If the result is cached, the performance of createSprites() doesn't matter nearly as much, but then the question is why TextureAtlas can't create a cache with some method it provides.

I think it's OK the atlas doesn't try to keep/manage a cache for sprites. These are just convenience methods for some common tasks, eg so someone can easily hand sprites to libgdx's Animation class. It's true there's a lot of string comparisons. If that were a problem I guess an API user would do something on their own, eg iterate the regions themselves and stuff them into a map of arrays or similar. FWIW, I don't think I've ever needed these sprite methods.

Would anyone else like to review further? How about we give it one more day? If more time is needed that's OK, just let me know.

@mgsx-dev
Copy link
Contributor

@NathanSweet to me it's a breaking change for custom parsers since atlas output changed. Could you please add something to CHANGES file. We recently adopt a convention to put all breaking changes at the top of the list to help people picking them up more easily.

@mgsx-dev mgsx-dev added the breaking change Label for breaking changes label Dec 18, 2020
@tommyettinger
Copy link
Member

We should make sure there's still a way to download the runnable texture packer jar from 1.9.12, because the jar made after this is merged won't make atlases that are compatible with 1.9.12 or earlier.

@obigu
Copy link
Contributor

obigu commented Dec 18, 2020

@NathanSweet Thanks for the answer but I think the devil is in the details so let's go back to the specific example of format. Currently if set to RGBA8888 on libGDX TexturePacker it will not output it because it's a default.

First question is: where is this default value for format defined exactly so that libGDX sets it to RGBA8888 when missing on the atlas? I would expect all defaults from the parser in one place.

My second question was about opening on another 3rd party tool such as Texture Packer Pro a new texture atlas generated with libGDX Texture Atlas that doesn't have the format defined or other key properties, would it work? Probably not. And same problem occurs for current code that relies on the current format. Making important properties like format optional and not outputting them when value is default we are breaking backwards compatibility in some cases just to skip 1 line.

IMO having everything optional doesn't make it a good file especification, it makes life harder for 3rd party integrations or custom extensions that expect some minimal stuff to be there and breaks backwards compatibility for minimal gain. I think if we define what is required and what is not we would have the benefits of both skipping the properties that are really optional (like index) and take a lot of space while keeping bakwards compatibility in most cases and 3rd party integration support (and as an extra we get file validation).

EDIT: Forgot to say, good stuff on the Spine Wiki, I definitely think we should have it on libGDX wiki once we merge this.

@NathanSweet
Copy link
Member Author

NathanSweet commented Dec 18, 2020

@mgsx-dev Good point, CHANGES updated.

We should make sure there's still a way to download the runnable texture packer jar from 1.9.12, because the jar made after this is merged won't make atlases that are compatible with 1.9.12 or earlier.

@tommyettinger True. I see the current packer link. Is there an existing URL for older builds?

As an alternative we could have a "legacy" mode in the new packer so it can write the old way. I'm not a huge fan of keeping around the old code, but it's not much. We'd need to add a legacy packer setting. I guess it would default to false? If it had to default to true that would be OK, since what is most important to me is that the new format can be parsed. Do we want a legacy mode?

where is this default value for format defined exactly

@obigu It's in the format specification, currently here but also on the libgdx wiki after this PR. In the libgdx source it's here:

static public class Page {
public FileHandle textureFile;
public Texture texture;
public float width, height;
public boolean useMipMaps;
public boolean pma;
public Format format;
public TextureFilter minFilter = TextureFilter.Nearest, magFilter = TextureFilter.Nearest;
public TextureWrap uWrap = ClampToEdge, vWrap = ClampToEdge;
}
static public class Region {
public Page page;
public String name;
public boolean flip;
public int index = -1;
public float offsetX, offsetY;
public int originalWidth, originalHeight;
public boolean rotate;
public int degrees;
public int left, top;
public int width, height;
public @Null String[] names;
public @Null int[][] values;

My second question was about opening on another 3rd party tool such as Texture Packer Pro

Other tools need to follow the specification in order to read the format. Parsers using the old specification cannot read the updated format.

IMO having everything optional doesn't make it a good file especification, it makes life harder for 3rd party integrations or custom extensions that expect some minimal stuff to be there

It's not difficult for parser implementations to use default values.

and breaks backwards compatibility for minimal gain.

To prevent making breaking changes for existing parsers (ie old versions of libgdx or tools outside of libgdx) then we need to:

  1. always write all fields and in a specific order, and
  2. never write additional fields.

In other words, we can't change the format at all. If we want that, we don't need this PR.

This PR improves the parsing and format specification so parsers can tolerate unknown fields, allowing us to add fields (whether a new standard like origin or something application specific like bone data) without breaking parsing. The cost of improving things is that the old, brittle parser won't be able to handle the updated format.

I think if we define what is required and what is not we would have the benefits of both skipping the properties that are really optional (like index) and take a lot of space while keeping bakwards compatibility in most cases and 3rd party integration support (and as an extra we get file validation).

I don't see a connection between making some fields required and compatibility.

We don't perform thorough validation before or after this PR and I don't think it's worth adding the code to do so. The atlas file is almost always generated, so validation only benefits tools generating the data, of which there are few. If important fields are omitted, the resulting atlas will work but probably not in a useful way: garbage in, garbage out.

@NathanSweet
Copy link
Member Author

I added a legacyOutput setting so we don't need to provide multiple downloads.

For kicks I also added a prettyPrint setting which removes all whitespace except newlines. My atlas files are ~40% of the size of the old format! That it makes no tangible difference is beside the point. ;)

I'd like to merge tomorrow morning.

@MrStahlfelge
Copy link
Member

I think tomorrow is a bit fast considering this is a breaking and not a minor change. Based on experiences with former file format changes which always broke GWT and I still regularly have to convert p files because older games don't start on GWT because of the format change, I want to check how this behaves on GWT and others might want to check some other stuff.

@tommyettinger
Copy link
Member

The legacyOutput option is very good; we should definitely wait long enough to test this fully on GWT, though. I think there's several PRs in progress that should optimally be addressed before the next release, so there's no urgency.

@obigu
Copy link
Contributor

obigu commented Dec 18, 2020

@obigu In the libgdx source it's here:

It isn't, I've found it, the default format gets set on Pixmap constructor, maybe we want to set it on TextureAtlas class like the rest of defaults?

Other tools need to follow the specification in order to read the format. Parsers using the old specification cannot read the updated format.

The legacyOutput option fixes my concerns on backwards compatibility/3rd party support.

I've run some tests and it seems to be working fine, I think it's a good change, thanks!

@mgsx-dev
Copy link
Contributor

mgsx-dev commented Dec 18, 2020

We'd need to add a legacy packer setting. I guess it would default to false? If it had to default to true that would be OK, since what is most important to me is that the new format can be parsed.

I think it'd be better to have legacy mode enabled by default: We need old settings files to produce the old format (used by TexturePacker and TiledMapPacker). Only people who really need the new format have to enabled it which will be a lot less impacting for other people. That would make this PR non breaking if i'm not wrong.

I would reconsider using a version tag, it would be a lot more easier for custom parser and mostly to help people debugging their issues by clearly distinguish old and new format. It could also be very useful if we plan to remove old format support in the future or need to break compatibility at some point. No version tag was a big problem for some other formats (eg. particle2D files), i think we shouldn't make the same mistake here.

As i reminder, wiki will need an update about settings file format : https://github.com/libgdx/libgdx/wiki/Texture-packer#settings

Copy link
Member

@MrStahlfelge MrStahlfelge left a comment

Choose a reason for hiding this comment

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

AtlasIssueTest is broken on - at least - GWT and Lwjgl3.
On GWT the console log is written full of error messages until browser stops logging, on Lwjgl3 there's no error message given.

@tommyettinger
Copy link
Member

tommyettinger commented Dec 19, 2020

AtlasIssueTest appears to have been broken before on GWT -- it uses MipMapLinearNearest filtering mode, which isn't supported by WebGL (according to Firefox). I locally changed tests/gdx-tests-android/assets/data/issue_pack to use Linear filtering, and the test works again. The other atlas-related tests work.

EDIT: with my change, LWJGL3 also renders correctly, on my machine at least: https://i.imgur.com/JOmjIIf.png

I just pushed the one-line change; I don't think anything will break.

@NathanSweet
Copy link
Member Author

I think tomorrow is a bit fast considering

Where is your sense of adventure!? I was hoping not to need to dork around with my build system to build with a different libgdx repo, but now that that's done there is no rush on this PR.

It isn't, I've found it, the default format gets set on Pixmap constructor, maybe we want to set it on TextureAtlas class like the rest of defaults?

@obigu Oh, good catch. I missed the format wasn't being set. Fixed!

think it'd be better to have legacy mode enabled by default

@mgsx-dev That works for me. The important part for me is that libgdx can parse the new format.

I would reconsider using a version tag

We're missing data outside page/regions. Without that, data applicable to the whole file (like version) would have to go on each page. It'd be reasonable to have an optional "header" section for entries before the first page. I've added that now. Since we don't actually make use of such values, they are currently ignored but give us a place to add version if needed later. We would just give it a default value when omitted.

It's good to think about how the format would evolve and minimize breakage if it does, though we also don't want to over engineer it. Some considerations about how the parser works that may be an issue later:

  • Entries are limited to 4 values. We could ensure parsing ignores values after the 4th.
  • Unknown region entry values are always int[]. We could silently ignore non-int values, using 0.

I've made the changes for those. The changes are small and may help in the future, though we've gone so long with the old format I doubt we'll do much more with it. We could make this parsing behavior part of the format spec, though I'm reaching the end of the time I can spend on this.

As i reminder, wiki will need an update about settings file format

Good point. So far post merge we need:

  1. Copy texture atlas file format from Spine webpage to libgdx wiki.
  2. Update TexturePacker settings wiki page.

AtlasIssueTest is broken

@MrStahlfelge Good catch! Wasn't creating mipmaps when the min filter needs them.

@NathanSweet NathanSweet dismissed MrStahlfelge’s stale review December 19, 2020 00:41

Fixed by setting useMipMaps when min filter needs them.

And fixed in a more thorough way, too.
@mgsx-dev mgsx-dev added breaking change Label for breaking changes and removed breaking change Label for breaking changes labels Dec 19, 2020
@mgsx-dev
Copy link
Contributor

sorry i'm not sure if it's a breaking change or not, the diff is hard to read since some classes have been moved in the file (Page, Region and such)... could you please make the diff more readable (eg. moving back classes where they were) ? I understand you're out of time but it would really help and keep history a bit cleaner.

it seams that some public fields have been removed (eg. region#splits), if so, could you please update CHANGES file with breaking changes?

sorry to be nit-picky.

@NathanSweet
Copy link
Member Author

@mgsx-dev No worries, you're right there was a breaking change with AtlasRegion and Region. I've added a CHANGES entry.

It's not great to have member classes at the top of a class for the sake of a diff. I go to a class I want to look at and have to scroll past other junk.

Drives my OCD nuts to put member classes at the top.
* Reuse region names/values arrays.
* Initialize index to -1 by default.
* Comments to note deprecated fields.
* Use @null.
* 1024 BufferedReader size.
* Don't need to store index comparator on a field.
* Changed equalsIgnoreCase to equals. Most of the format is case sensitive so seemed odd to ignore case only in some places.
* Use ensureCapacity for textures and regions collections.
* Clean up wonky texture initialization.
* Don't need a static field for reading the entry, plus it wasn't thread safe.
@NathanSweet
Copy link
Member Author

Did another pass to make it gud. Assuming builds pass, I think it's ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Label for breaking changes core affecting all platforms enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants