Pencil: Minor image export bugs

[scribblemaniac]

This issue lumps together many small problems with the image exporter and the image sequence exporter that could all be tackled at once.

Problems with the Image Exporter (single frame):

• [x] Transparency can be checked off even for image formats that do not support transparency. The checkbox should instead be disabled when jpg is selected.
• [x] Bmp export with transparency is broken. I think this is a Qt issue, but I can't get bmps to save properly with transparency. Theoretically the file format supports it, so perhaps experiment with different QImage formats and search around the docs for some more information. Failing that, disable transparency for bmp as well.

Problems with the Image Sequence Exporter (multi frame):

• [x] Again the transparency checkbox should be disabled for jpg.
• [x] Bmp has the same issue here, so the same solution should be used here as with the Image Exporter.
• [x] When changing the image file format, the filename changes incorrectly. It goes from frame.test..png to frame.jpg for example, removing everything after the first period when it should be removing everything after the last period to strip the extension.
• [x] For some reason frame.bmp exports as frame.bmp0001... but frame.jpg exports as frame0001.jpg. The numbering behavior should be consistent for all formats.

[przet]

I'm going to have a look at this one :)

[przet]

A question on preferred communication style: as I am working through this issue, I was going to mention some of what I have done and what I am thinking to do, so that its done before a PR. If this is okay, and how it is done here, where is the best place to do it? Here under the issue, or on the forums or chat channel? If not, is it a matter of: fix -> PR-> get feedback and fix->PR, until accepted? Thanks!

[przet]

When exporting, I can't edit the name in the text box - this can only be done if I click "browse"
However if I click save for the default export name (untitled) I will _not_ be prompted with an overwrite warning if I already have a file with that name and location in existence.

Should this be fixed? If so I can see if its simple enough to do as part of this issue, otherwise I will open a new issue.

[Jose-Moreno]

@przet Hi. We usually discuss features on the issue thread, but once the PR has been pushed you can also discuss on the PR. The reviewers will normally comment on the code composition of course, but if you express your intent or describe the feature the PR can also serve as a place to discuss the feature in order to have everything related to it on a single place.

As for the text box not being editable, that to me is a ux bug, since the only way to change the name as you have noticed is when you're browsing for a save location. The overwrite behavior seems like a bug indeed, so I think it should be fixed. I believe to keep track of Pencil2D's general progress this would be better in a separate issue instead of a PR related to the current export issues.

I'd like to ask @scribblemaniac or @CandyFace to chime in as well to see if there are any considerations regarding the communication style and the bugs you've noticed.

Thank you for taking the time to help the project! :slightly_smiling_face:

[scribblemaniac]

Yes this is a fine place to talk about that. For larger issues, make a new issue. If it's an issue related to a PR you've submitted than comment there. The chat channel can be a good place for getting clarifications or help with development. The forum tends to be less technical.

I would also like to see the textbox editable if possible. You have to double check sure that the directories exist before attempting to write out then.

The overwrite behavior is a bit more complicated. On mac (I'm not sure about other platforms), when you browse and set a file location, the operating system will warn you if you are about to overwrite something. But it won't warn you if you do not change the save location from whatever the default is. Ideally it should only warn a user once per save-as before overwriting, no matter if the save location is changed or not or what operating system you are on.

[przet]

@Jose-Moreno okay, thanks. And yep, I will create a separate issue for covering the overwrite behaviour and editable text box. And my pleasure :) I am enjoying it, this seems like a very cool project!

@scribblemaniac thanks for the response. On Linux when I am browsing from the Pencil2D export window you also get the OS warning, and it would be the same for Windows too I'm sure (although I will test it just in case).

[przet]

I have disabled the transparency for JPG export (both single and multiframe), and I had a look at what is going on for BMP transparency. I couldn't figure it out after a bit of effort, so I disabled transparency for BMP too for the moment, while I work on the remaining points in this issue. I would like to come back and dig more into the BMP transparency before submitting a PR, I think there is a bit more to explore:)

For the transparency disabling I used the following logic:

 if (format == "JPG" || format == "BMP")
        ui->cbTransparency->setDisabled(true); 
    else
        ui->cbTransparency->setDisabled(false); 

And I put this in a private function (I did not think it needed to be public) in the ExportImageDialog class.
This results in the following call (which takes care of both single and multiframe):

void ExportImageDialog::formatChanged(const QString& format)
{
    setFileExtension(format.toLower());
    setTransparencyOptionVisability(format); //The new function
}

The name is a bit long, but it does describe the functionality - I welcome any other suggestions/feedback (on the name or otherwise).

[przet]

I was able to fix the problem of saving with a filename containing a "." in it (that is not an extension). All I had to do was call completeBaseName() on info rather than baseName() in void ImportExportDialog::setFileExtension(QString extension).

However I stumbled across another issue: it is possible to select to export a JPG but still end up with a PNG (should be the same issue for other formats). This happens because when I load up one of the export dialogs (multi or single frame), and say there is in the fileEdit box a file name untitled.png. If I select JPG, the filename will update, but if I browse, the filename in the file browser will still be untitled.png. I can save this, the fileEdit box will have untitled.png now, but the export option will be JPG. So there is a mismatch. In pictures:

This will happen on load too, because(I think) of the getLastSavePath(...) in void importExportDialog::init().

Create a seperate issue for this too?

[przet]

For some reason frame.bmp exports as frame.bmp0001... but frame.jpg exports as frame0001.jpg. The numbering behavior should be consistent for all formats.

This is now fixed. bool Object::exportFrames was missing the following (note my comment about transparency and bmp):

if (formatStr == "BMP" || formatStr == "bmp")
    {
        format = "BMP";
        extension = ".bmp";
        transparency = false; // Even though BMP supports transparency, we don't allow it for now.
    }

But TIFF is not exporting - for single frames, there is an error. For multi frames, looks like a silent fail: there is no warning, but I can't find it in the save location.

[przet]

BMP transparency export is still not working, after playing around with a few different QImage formats, and other ideas. It seems like it should be possible, and people on the net have said they got it to work. I would be happy to keep trying to get it to work, but should I make a new issue and submit a PR with bmp transparency off?

By the way I have been asking if I should create issues since I'm still new here and didn't want to just create issues spawning from this one without checking first - let me know if you want me to continue to check, or if I should just go ahead and create them if they seem issue worthy.

[CandyFace]

The old and known BMP format has never had a transparency channel. it's a new thing that's been added via the BMPV5Header and It doesn't look like QT uses that header yet.
https://forum.qt.io/topic/86938/alpha-channel-not-written-to-bmp-by-qimage-save/9

[scribblemaniac]

@CandyFace Yes, it does seem from the code in the linked in that topic that they are not saving the alpha channel currently. That's too bad. @przet Thanks anyway for trying to get bmp transparency working, but it seems we will have to wait for such a feature to be added to Qt. Looking into it more, it seems that the transparency checkbox should only be enabled for png (so disabled for jpg, bmp, and tiff).

However I stumbled across another issue: it is possible to select to export a JPG but still end up with a PNG (should be the same issue for other formats)...

Yes that's a problem. What we should do is use the same technique as the "Export movie" option, which has no dropdown for selecting the format, but instead uses an extension filter in the qfiledialog, and parses the extension to determine the format to export as.

But TIFF is not exporting - for single frames, there is an error. For multi frames, looks like a silent fail: there is no warning, but I can't find it in the save location.

TIFF export is working fine for me on the nightly build. Can you export with a nightly build first to see if this is caused by some change you've made. It may be possible you have a Qt installation without the TIFF codec. Check to see what QImageWriter::supportedImageFormats() outputs.

By the way I have been asking if I should create issues since I'm still new here and didn't want to just create issues spawning from this one without checking first - let me know if you want me to continue to check, or if I should just go ahead and create them if they seem issue worthy.

Just make issues using your best judgement. If we don't think it's should be an issue, we will say so and close it, no big deal. Just do a search first to make sure that the issue has not been brought up before.

If you are working on this for hacktoberfest but you're not quite finished, feel free to submit a [WIP] pull request so it will count towards your PR count :wink:

[Jose-Moreno]

@przet @scribblemaniac @CandyFace Thank you all for looking into this. Since you guys are working on the transparency checks for different formats, care to take a look into #1113 ? My main question is would there need to be any additional implementation to have it working for Animated GIF files and the more distant APNG file format found on the Movie export?

If it goes beyond implementing a similar solution to what is being done here then don't worry, we'll leave them for a future milestone, just wanted to ask :slightly_smiling_face:

[scribblemaniac]

@Jose-Moreno The solution for transparent movie export is pretty much a separate from image/image sequence export. The issue with ffmpeg export transparency is with pngs and ffmpeg, which is not used for image export. So let's leave that for a separate issue. If it makes you feel any better, it should be relatively easy to add support for transparency to most movie export formats.

[przet]

@scribblemaniac yes originally I found this project inspired by hacktoberfest, but I have missed the date now...but I guess it has served its purpose as I like this project and am keen to keep contributing :). I just need to clean up my code from comments and should be able to submit a PR this weekend. And I'll make new issues where I think it makes sense from the other things I have found.

[przet]

@CandyFace

The old and known BMP format has never had a transparency channel. it's a new thing that's been added via the BMPV5Header and It doesn't look like QT uses that header yet.

Okay, cool; thanks for that. I had come across people posting conflicting things about bmp transparency in general, so good to know this.

[przet]

@scribblemaniac

TIFF export is working fine for me on the nightly build. Can you export with a nightly build first to see if this is caused by some change you've made. It may be possible you have a Qt installation without the TIFF codec. Check to see what QImageWriter::supportedImageFormats() outputs.

Yep, I did not have the TIFF codec installed (and it wasn't in the supportedImageFormats() output). I am thinking it might be useful to have a warning pop up if the user selects tiff (or any other format) that isn't installed, with instructions on how to install it - I think I did a pretty standard install of Qt on my (Linux) system, but still didn't get the TIFF codec. I might make this an issue (or add it to a set of subissues).

Looking into it more, it seems that the transparency checkbox should only be enabled for png (so disabled for jpg, bmp, and tiff).

I am getting TIFF transparency...I will check when I move over to Windows to test all my fixes (even though Qt is cross-platform I still feel its best to check to make sure)

[Jose-Moreno]

@przet @scribblemaniac Would we need to bundle the TIFF codec though for the end user? using graphics software like Krita or GIMP has never prompted me to install a codec for TIFF import or export. Or is this an issue with the QT installation only?

Also thank you for all the hard work Philippe! :relaxed:

[scribblemaniac]

I am getting TIFF transparency...I will check when I move over to Windows to test all my fixes (even though Qt is cross-platform I still feel its best to check to make sure)

I just saw this bug which said that "Qt doesn't save TIFFs with alpha". If it works though then that must be outdated/misinformed.

Would we need to bundle the TIFF codec though for the end user? using graphics software like Krita or GIMP has never prompted me to install a codec for TIFF import or export. Or is this an issue with the QT installation only?

The TIFF codec should be deployed with the application with no extra work to be done by the end user. If the TIFF codec is missing, it is either an issue with the Qt installation or the app deployment.

I am thinking it might be useful to have a warning pop up if the user selects tiff (or any other format) that isn't installed, with instructions on how to install it

I don't think a warning would be all that useful because if a user encounters that dialog they don't have any easy way to fix it. What I do think could be improved is adding instructions for this in the build instructions. The program should also check to make sure the TIFF function is supported before adding it as an option to the import/export dialogs. This should be done in an easily extensible manner because we might add support for the other bundled image codecs eventually (full list of possible 3rd party codecs can be found here) Finally, we should verify that all of our nightly builds are producing applications that export TIFFs properly.

[przet]

Thanks @Jose-Moreno :slightly_smiling_face:

[Jose-Moreno]

Thank you everyone for your contributions to this issue, apologies for taking so long to close it! Special thanks to @przet for addressing this issue and @scribblemaniac & @CandyFace for reviewing it so thoroughly.