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

Pixly #1177

Merged
merged 7 commits into from Jul 4, 2016

Conversation

Projects
None yet
2 participants
@zed9h
Contributor

zed9h commented Jul 2, 2016

First try at implementing the Pixly file format, just loading for now and I tested with only two files, but it could be useful to some people (for instance this guy), so I am pushing right away.

@dacap

This comment has been minimized.

Show comment
Hide comment
@dacap

dacap Jul 2, 2016

Member

Hi @zed9h, could this use the same png encoder/decoder to avoid duplicating the whole code? I see that it opens an XML file and a png file, so to load the png file you might try to use the png decoder (or calling load_document() to load the png file as a temporal doc::Sprite/app::Document).

Also could you please sign our CLA?

Member

dacap commented Jul 2, 2016

Hi @zed9h, could this use the same png encoder/decoder to avoid duplicating the whole code? I see that it opens an XML file and a png file, so to load the png file you might try to use the png decoder (or calling load_document() to load the png file as a temporal doc::Sprite/app::Document).

Also could you please sign our CLA?

@zed9h

This comment has been minimized.

Show comment
Hide comment
@zed9h

zed9h Jul 2, 2016

Contributor

@dacap, maybe It would be cleaner if I create a helper to decode/encode the PNG for both file formats, but first I will work on the Pixly format save, which will add more value upfront.

PS: I will read the CLA.

Contributor

zed9h commented Jul 2, 2016

@dacap, maybe It would be cleaner if I create a helper to decode/encode the PNG for both file formats, but first I will work on the Pixly format save, which will add more value upfront.

PS: I will read the CLA.

@dacap

This comment has been minimized.

Show comment
Hide comment
@dacap

dacap Jul 2, 2016

Member

I'll take a look at this on next Tuesday. At the moment I recommend you to use load_document() if it's possible, so we can avoid modifying the actual png encoder/decoder (the code review from my side would be too much work if you refactor the code).

You can use Document* tmpDoc = load_document(nullptr, pngfilename) to "subload" the png file, and save_document(nullptr, tmpDoc) to save the png file.

Member

dacap commented Jul 2, 2016

I'll take a look at this on next Tuesday. At the moment I recommend you to use load_document() if it's possible, so we can avoid modifying the actual png encoder/decoder (the code review from my side would be too much work if you refactor the code).

You can use Document* tmpDoc = load_document(nullptr, pngfilename) to "subload" the png file, and save_document(nullptr, tmpDoc) to save the png file.

@zed9h

This comment has been minimized.

Show comment
Hide comment
@zed9h

zed9h Jul 3, 2016

Contributor

I signed the CLA and finished the Pixly file save; the code ended up much simpler than the original PNG format, because it only handle RGBA, take a look if you think it worth the generalization of the codec. For me it was easier to handle a standard lib code than aseprite internal structure, even when I found the latter pretty clear and organized.

Contributor

zed9h commented Jul 3, 2016

I signed the CLA and finished the Pixly file save; the code ended up much simpler than the original PNG format, because it only handle RGBA, take a look if you think it worth the generalization of the codec. For me it was easier to handle a standard lib code than aseprite internal structure, even when I found the latter pretty clear and organized.

@dacap

This comment has been minimized.

Show comment
Hide comment
@dacap

dacap Jul 3, 2016

Member

@zed9h Remember that there is an email required in the CLA signing process too.

Member

dacap commented Jul 3, 2016

@zed9h Remember that there is an email required in the CLA signing process too.

"<PixlyAnimation version=\"1.5\">\n"
"\t<Info "
"sheetWidth=\"%d\" sheetHeight=\"%d\" "
"totalCollumns=\"%d\" totalRows=\"%d\" "

This comment has been minimized.

@dacap

dacap Jul 3, 2016

Member

Is this totalCollumns or totalColumns?

@dacap

dacap Jul 3, 2016

Member

Is this totalCollumns or totalColumns?

Show outdated Hide outdated src/app/file/pixly_format.cpp
sprite->folder()->addLayer(new LayerImage(sprite));
}
int *visible = (int*)malloc(layerCount * sizeof(int));

This comment has been minimized.

@dacap

dacap Jul 3, 2016

Member

visible can be a std::vector, e.g.

std::vector<int> visible(layerCount, 0);
@dacap

dacap Jul 3, 2016

Member

visible can be a std::vector, e.g.

std::vector<int> visible(layerCount, 0);
Show outdated Hide outdated src/app/file/pixly_format.cpp
@@ -0,0 +1,487 @@
// Aseprite
// Copyright (C) 2001-2016 David Capello

This comment has been minimized.

@dacap

dacap Jul 3, 2016

Member

You can add a new Copyright (C) 2016 [your name] here.

@dacap

dacap Jul 3, 2016

Member

You can add a new Copyright (C) 2016 [your name] here.

Show outdated Hide outdated src/app/file/pixly_format.cpp
TiXmlHandle xml(doc.get());
fop->setProgress(0.75);
TiXmlElement* xmlAnim = xml.FirstChild("PixlyAnimation").ToElement();

This comment has been minimized.

@dacap

dacap Jul 3, 2016

Member

What happen if PixlyAnimation element does not exist?

@dacap

dacap Jul 3, 2016

Member

What happen if PixlyAnimation element does not exist?

Show outdated Hide outdated src/app/file/pixly_format.cpp
TiXmlElement* xmlAnim = xml.FirstChild("PixlyAnimation").ToElement();
TiXmlElement* xmlInfo = xmlAnim->FirstChild("Info")->ToElement();

This comment has been minimized.

@dacap

dacap Jul 3, 2016

Member

Same here, what happen if Info element does not exist? (Or the attributes in the following lines, layerCount, frameWidth, frameHeight?)

@dacap

dacap Jul 3, 2016

Member

Same here, what happen if Info element does not exist? (Or the attributes in the following lines, layerCount, frameWidth, frameHeight?)

Show outdated Hide outdated src/app/file/pixly_format.cpp
TiXmlElement* xmlInfo = xmlAnim->FirstChild("Info")->ToElement();
int layerCount = strtol(xmlInfo->Attribute("layerCount"), NULL, 10);

This comment has been minimized.

@dacap

dacap Jul 3, 2016

Member

You should use base::convert_to<int>(...) to convert from std::string/const char* to int

@dacap

dacap Jul 3, 2016

Member

You should use base::convert_to<int>(...) to convert from std::string/const char* to int

Show outdated Hide outdated src/app/file/pixly_format.cpp
const char * visible_str = xmlFrame->Attribute("visible");
if(visible_str) {
visible[(int)layer_index] += std::string(visible_str) == "true";

This comment has been minimized.

@dacap

dacap Jul 3, 2016

Member

Add (and ) in the right-side expression:

visible[(int)layer_index] += (std::string(visible_str) == "true");
@dacap

dacap Jul 3, 2016

Member

Add (and ) in the right-side expression:

visible[(int)layer_index] += (std::string(visible_str) == "true");
@dacap

This comment has been minimized.

Show comment
Hide comment
@dacap

dacap Jul 3, 2016

Member

As a side note, I'll need several .anim files to test these routines (original files generated with Pixly), because I don't have an Android device and cannot install Pixly. You can send me some test files at support@aseprite.org (I'll add them into an internal test suite I use for decoders/encoders)

Member

dacap commented Jul 3, 2016

As a side note, I'll need several .anim files to test these routines (original files generated with Pixly), because I don't have an Android device and cannot install Pixly. You can send me some test files at support@aseprite.org (I'll add them into an internal test suite I use for decoders/encoders)

@zed9h

This comment has been minimized.

Show comment
Hide comment
@zed9h

zed9h Jul 4, 2016

Contributor

@dacap Great points, I placed many more check()s and improved the error messages. I will send some good files and some bad ones to test the exceptions.

Contributor

zed9h commented Jul 4, 2016

@dacap Great points, I placed many more check()s and improved the error messages. I will send some good files and some bad ones to test the exceptions.

Show outdated Hide outdated src/app/file/pixly_format.cpp
{
const Sprite* sprite = fop->document()->sprite();
if(sprite->pixelFormat() != IMAGE_RGB || !fop->document()->sprite()->needAlpha()) {

This comment has been minimized.

@dacap

dacap Jul 4, 2016

Member

There is no need to check needAlpha. Also, you can remove the pixelFormat() check if you remove the FILE_SUPPORT_GRAY, FILE_SUPPORT_GRAYA, and FILE_SUPPORT_INDEXED from onGetFlags() (and you should remove those constants from onGetFlags() as this format doesn't support Grayscale, Indexed)

@dacap

dacap Jul 4, 2016

Member

There is no need to check needAlpha. Also, you can remove the pixelFormat() check if you remove the FILE_SUPPORT_GRAY, FILE_SUPPORT_GRAYA, and FILE_SUPPORT_INDEXED from onGetFlags() (and you should remove those constants from onGetFlags() as this format doesn't support Grayscale, Indexed)

Show outdated Hide outdated src/app/file/pixly_format.cpp
FILE_SUPPORT_INDEXED |
FILE_SUPPORT_LAYERS |
FILE_SUPPORT_FRAMES |
FILE_SUPPORT_PALETTE_WITH_ALPHA;

This comment has been minimized.

@dacap

dacap Jul 4, 2016

Member

Remove FILE_SUPPORT_PALETTE_WITH_ALPHA too as this format doesn't support indexed images with color palettes (with alpha or without alpha).

@dacap

dacap Jul 4, 2016

Member

Remove FILE_SUPPORT_PALETTE_WITH_ALPHA too as this format doesn't support indexed images with color palettes (with alpha or without alpha).

Show outdated Hide outdated src/app/file/pixly_format.cpp
#include <stdio.h>
#include <stdlib.h>
#include <math.h>

This comment has been minimized.

@dacap

dacap Jul 4, 2016

Member

As a minor detail, you can #include <cstdio>, <cstdlib>, and <cmath> here.

@dacap

dacap Jul 4, 2016

Member

As a minor detail, you can #include <cstdio>, <cstdlib>, and <cmath> here.

@zed9h

This comment has been minimized.

Show comment
Hide comment
@zed9h

zed9h Jul 4, 2016

Contributor

If I remove the FILE_SUPPORT_PALETTE_WITH_ALPHA and the auto-generated palette has some alpha on it, there will be a warning every time the file is saved, so I think I will need to keep this flag, even if the palette will not be saved. I also added FILE_SUPPORT_BIG_PALETTES for the same reason. (Not really sure if this is the right way to go.)

I cleaned up some includes that seemed unnecessary.

Contributor

zed9h commented Jul 4, 2016

If I remove the FILE_SUPPORT_PALETTE_WITH_ALPHA and the auto-generated palette has some alpha on it, there will be a warning every time the file is saved, so I think I will need to keep this flag, even if the palette will not be saved. I also added FILE_SUPPORT_BIG_PALETTES for the same reason. (Not really sure if this is the right way to go.)

I cleaned up some includes that seemed unnecessary.

@dacap

This comment has been minimized.

Show comment
Hide comment
@dacap

dacap Jul 4, 2016

Member

Mhh, I think it's a bug in the code that check conditions in file.cpp. I'll be working in a fix for this so FILE*PALETTE* flags are checked only for Indexed images. (But I've to think more about those situations.)

The FILE_SUPPORT_BIG_PALETTES is in case the file format can save a color palette with more than 256 colors.

You can leave the flags and I'll then modify when the fix is done. Don't worry about that.

Member

dacap commented Jul 4, 2016

Mhh, I think it's a bug in the code that check conditions in file.cpp. I'll be working in a fix for this so FILE*PALETTE* flags are checked only for Indexed images. (But I've to think more about those situations.)

The FILE_SUPPORT_BIG_PALETTES is in case the file format can save a color palette with more than 256 colors.

You can leave the flags and I'll then modify when the fix is done. Don't worry about that.

int imageCount = check_number<int>(xmlFrames->Attribute("length"));
if(layerCount <= 0 || imageCount <= 0) {
throw Exception("No cels found");

This comment has been minimized.

@dacap

dacap Jul 4, 2016

Member

I see that these throw can generate some leaks (mainly the png structures that are not freed). Anyway, don't worry, I'll integrate this with the png encoder/decoder later. I'll merge this code as it is.

@dacap

dacap Jul 4, 2016

Member

I see that these throw can generate some leaks (mainly the png structures that are not freed). Anyway, don't worry, I'll integrate this with the png encoder/decoder later. I'll merge this code as it is.

This comment has been minimized.

@dacap

dacap Jul 4, 2016

Member

Oh sorry, I've just saw the try{}catch block to avoid leaks.

@dacap

dacap Jul 4, 2016

Member

Oh sorry, I've just saw the try{}catch block to avoid leaks.

@dacap dacap merged commit 7b4a1ec into aseprite:master Jul 4, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dacap

This comment has been minimized.

Show comment
Hide comment
@dacap

dacap Jul 4, 2016

Member

Finally I got a crash saving this file:

screen shot 2016-07-04 at 12 34 09 pm

Could you please check this issue?

Member

dacap commented Jul 4, 2016

Finally I got a crash saving this file:

screen shot 2016-07-04 at 12 34 09 pm

Could you please check this issue?

@zed9h

This comment has been minimized.

Show comment
Hide comment
@zed9h

zed9h Jul 4, 2016

Contributor

As I suspected when I saw the line of the error, the cel image can be smaller than the sprite size, I will compensate for that.

Contributor

zed9h commented Jul 4, 2016

As I suspected when I saw the line of the error, the cel image can be smaller than the sprite size, I will compensate for that.

zed9h added a commit to pseudogames/aseprite that referenced this pull request Jul 4, 2016

Add support for Pixly file format (#1177)
New Pixly .anim format decoder/encoder based on png decoder/encoder.

zed9h added a commit to pseudogames/aseprite that referenced this pull request Jul 4, 2016

Add support for Pixly file format (#1177)
New Pixly .anim format decoder/encoder based on png decoder/encoder.
@dacap

This comment has been minimized.

Show comment
Hide comment
@dacap

dacap Jul 5, 2016

Member

Returning to this one: I'll finally remove FILE_SUPPORT_BIG_PALETTES and FILE_SUPPORT_PALETTE_WITH_ALPHA flags from .anim format as this format doesn't support these features (and the user must be warned that the palette will be lost). I've added the new issue #1182 how to fix the issues commented here.

Member

dacap commented Jul 5, 2016

Returning to this one: I'll finally remove FILE_SUPPORT_BIG_PALETTES and FILE_SUPPORT_PALETTE_WITH_ALPHA flags from .anim format as this format doesn't support these features (and the user must be warned that the palette will be lost). I've added the new issue #1182 how to fix the issues commented here.

@zed9h

This comment has been minimized.

Show comment
Hide comment
@zed9h

zed9h Jul 5, 2016

Contributor

Of course, if the palette is user created there should be a warning.

@dacap BTW, I didn't explain it properly here, the "collums" (sic) are on the official Pixly format, nothing I could do about it.

Contributor

zed9h commented Jul 5, 2016

Of course, if the palette is user created there should be a warning.

@dacap BTW, I didn't explain it properly here, the "collums" (sic) are on the official Pixly format, nothing I could do about it.

@dacap

This comment has been minimized.

Show comment
Hide comment
@dacap

dacap Jul 5, 2016

Member

BTW, I didn't explain it properly here, the "collums" (sic) are on the official Pixly format, nothing I could do about it

👍 yeah, that's what I thought

Member

dacap commented Jul 5, 2016

BTW, I didn't explain it properly here, the "collums" (sic) are on the official Pixly format, nothing I could do about it

👍 yeah, that's what I thought

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