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

Minor image export bugs #1090

Closed
scribblemaniac opened this Issue Oct 4, 2018 · 21 comments

Comments

Projects
None yet
5 participants
@scribblemaniac
Copy link
Member

scribblemaniac commented Oct 4, 2018

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):

  • Transparency can be checked off even for image formats that do not support transparency. The checkbox should instead be disabled when jpg is selected.
  • 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):

  • Again the transparency checkbox should be disabled for jpg.
  • Bmp has the same issue here, so the same solution should be used here as with the Image Exporter.
  • 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.
  • 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

This comment has been minimized.

Copy link
Contributor

przet commented Oct 24, 2018

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

@przet

This comment has been minimized.

Copy link
Contributor

przet commented Oct 26, 2018

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

This comment has been minimized.

Copy link
Contributor

przet commented Oct 26, 2018

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

This comment has been minimized.

Copy link
Member

Jose-Moreno commented Oct 26, 2018

@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! 🙂

@scribblemaniac

This comment has been minimized.

Copy link
Member Author

scribblemaniac commented Oct 26, 2018

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

This comment has been minimized.

Copy link
Contributor

przet commented Oct 27, 2018

@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

This comment has been minimized.

Copy link
Contributor

przet commented Oct 27, 2018

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

This comment has been minimized.

Copy link
Contributor

przet commented Oct 27, 2018

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:
image
This will happen on load too, because(I think) of the getLastSavePath(...) in void importExportDialog::init().

Create a seperate issue for this too?

@przet

This comment has been minimized.

Copy link
Contributor

przet commented Oct 27, 2018

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

This comment has been minimized.

Copy link
Contributor

przet commented Oct 27, 2018

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

This comment has been minimized.

Copy link
Member

CandyFace commented Oct 27, 2018

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

This comment has been minimized.

Copy link
Member Author

scribblemaniac commented Oct 30, 2018

@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 😉

@Jose-Moreno

This comment has been minimized.

Copy link
Member

Jose-Moreno commented Oct 30, 2018

@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 🙂

@scribblemaniac

This comment has been minimized.

Copy link
Member Author

scribblemaniac commented Oct 30, 2018

@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

This comment has been minimized.

Copy link
Contributor

przet commented Nov 1, 2018

@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

This comment has been minimized.

Copy link
Contributor

przet commented Nov 1, 2018

@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

This comment has been minimized.

Copy link
Contributor

przet commented Nov 3, 2018

@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)

@przet przet referenced this issue Nov 3, 2018

Merged

Fix/minor export issues #1123

6 of 6 tasks complete
@Jose-Moreno

This comment has been minimized.

Copy link
Member

Jose-Moreno commented Nov 3, 2018

@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! ☺️

@scribblemaniac

This comment has been minimized.

Copy link
Member Author

scribblemaniac commented Nov 3, 2018

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

This comment has been minimized.

Copy link
Contributor

przet commented Nov 12, 2018

Thanks @Jose-Moreno 🙂

@chchwy chchwy added the bug label Feb 21, 2019

@Jose-Moreno

This comment has been minimized.

Copy link
Member

Jose-Moreno commented Mar 16, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.