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

Add QT GUI #159

Merged
merged 26 commits into from
Aug 30, 2017
Merged

Add QT GUI #159

merged 26 commits into from
Aug 30, 2017

Conversation

hryniuk
Copy link
Member

@hryniuk hryniuk commented Jul 19, 2017

Includes #17, eventually providing a tileset preview and options editor in a window.

@hryniuk
Copy link
Member Author

hryniuk commented Jul 19, 2017

Things to do

  • write CMakeLists.txt to include QT libraries into building process
  • add a button for tileset generation
  • add a preview area for displaying generated tileset
  • add options editor (at least some basics):
    • add a selector for options file
    • add a selector for output directory
    • alpha calculator mode
  • disable GUI building in CI or install and set up there QT libraries
  • display all cliques
  • split generation and saving
  • add option for selecting directory with images and checkboxes for tiles (not possible because of a need of generating default options basing on filenames)
  • display selected options file's path
  • fix build scripts - fail of WangscapeGui target doesn't impact result of Travis' OS X/Linux build (will be moved to a separate issue)

@hryniuk
Copy link
Member Author

hryniuk commented Jul 19, 2017

@serin-delaunay could you for now only check, whether this branch compiles and shows a window on your machine? Path to options directory is temporarily hard-coded, so you have to change it in code and recompile to generate a tileset using button.

Note that in CMakeLists.txt I've just included wangscape-main library. I wasn't sure, whether it's a good idea to split Wangscape into small libraries, one for each directory at that time; now we can just include different main.cpp file and create a GUI using this that lib 😃.

# Use the Widgets module from Qt 5.
target_link_libraries(WangscapeGui wangscape-main)

target_link_libraries(WangscapeGui Qt5::Widgets)
Copy link
Member Author

@hryniuk hryniuk Jul 19, 2017

Choose a reason for hiding this comment

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

It should try to find a Qt5 library, instead of assuming that it's installed (I mean it should use if clause here, cause find_package(Qt5Widgets) is already at line 8).

Separate issue. Qt5 it required anyway.


#include <boost/program_options/options_description.hpp>
#include <boost/program_options/parsers.hpp>
#include <boost/program_options/variables_map.hpp>
Copy link
Member Author

Choose a reason for hiding this comment

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

It's not needed, cause it doesn't use program_options.

{
logging::addAppender(std::make_unique<logging::ConsoleAppender>("console", logging::Level::Debug));

std::string filename = "/home/hryniuk/projekty/Wangscape/doc/examples/example2/example_options.json";
Copy link
Member Author

Choose a reason for hiding this comment

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

It have to be read form a text field.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we get the filename from a standard file selector dialogue?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, of course, I've only meant that it will be read from GUI.


void MainWindow::clickPushButton()
{
logging::addAppender(std::make_unique<logging::ConsoleAppender>("console", logging::Level::Debug));
Copy link
Member Author

Choose a reason for hiding this comment

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

Do we need to use a logger? We could think about notifications using Qt windows.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think while it's in the experimental stage we should use a logger. Log output should be viewable through something like a log widget or nonmodal window before public release.


tilegen::TilesetGenerator tg(options, std::move(tp));

std::cout << "generating tiles..." << std::endl;
Copy link
Member Author

Choose a reason for hiding this comment

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

Remove this debug print.


#include <iostream>

#include <iostream>
Copy link
Collaborator

Choose a reason for hiding this comment

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

The second #include <iostream> can be removed.

@hryniuk
Copy link
Member Author

hryniuk commented Jul 25, 2017

Screenshot :).

Last update: 2017-08-27.

tmp 1mpmhknbtb

@serin-delaunay
Copy link
Collaborator

serin-delaunay commented Jul 26, 2017

I made a project file for d2794d8, and it compiles and writes tilesets. However, it does use individual invocations of uic and moc for each file, which might not scale well. Screenshot:
wangscape-gui

I've pushed the changes to a side-branch: serin-delaunay/gui-vs.

@serin-delaunay
Copy link
Collaborator

(Also, this is awesome!)

@hryniuk hryniuk changed the title (WIP) Add QT GUI Add QT GUI Aug 17, 2017
@hryniuk
Copy link
Member Author

hryniuk commented Aug 17, 2017

Build on Linux finished successfully, so I'm removing "(WIP)" prefix. Two things left: green osx build and small refactoring (probably).

@hryniuk
Copy link
Member Author

hryniuk commented Aug 18, 2017

I've just noticed that Options can be easily copied and MainWindow can work on the copy by recreating TilePartitioner and TilesetGenerator after each change of e.g. alpha options. One of the problems I've seen was that MetaOutput::createOutputDirectory was called with options loaded from a file, but the directory can be created independently. I'll add an editor of options I've mentioned above to the GUI.

@hryniuk hryniuk changed the title Add QT GUI (WIP) Add QT GUI Aug 18, 2017
@hryniuk
Copy link
Member Author

hryniuk commented Aug 18, 2017

I've find out, which flag I'm supposed to pass to cmake to build Qt 5 Widgets project 😃, but there's another problem as I've mentioned in checkbox - linking failure make build green anyway, see e.g. https://travis-ci.org/hryniuk/Wangscape/jobs/266115590.

@serin-delaunay
Copy link
Collaborator

We have a similar problem on Appveyor (#150). I think it's to do with how CI interprets program return values, see eg. travis-ci/travis-ci#6768 .

@hryniuk
Copy link
Member Author

hryniuk commented Aug 18, 2017

@serin-delaunay yes, it should be easy to fix on Linux and OS X - just by saving return value and adding one if.

@hryniuk
Copy link
Member Author

hryniuk commented Aug 18, 2017

I've moved buttons to menu, it looks better. I plan to add another action in menu, opening a window with options editor, one per tab. First tab: Alpha Calculator Mode.

@hryniuk
Copy link
Member Author

hryniuk commented Aug 27, 2017

There's a problem with OSX build connected to stdlib. I thought that it should be fixed with -DCMAKE_CXX_FLAGS="-stdlib=libc++" flag, but it failed. I'll try to find a solution tomorrow.

See:
https://stackoverflow.com/questions/10991274/initializer-list-in-clang
https://stackoverflow.com/questions/14790354/is-initializer-list-like-this-legal-in-c11
and, of course, Travis build:
https://travis-ci.org/Wangscape/Wangscape/jobs/268994710

@hryniuk
Copy link
Member Author

hryniuk commented Aug 28, 2017

I've just move constructing the map to a function, resigning of initializer_list for now. same problem :S

@@ -13,5 +15,7 @@ enum class CalculatorMode
Dither
};

extern const std::map<std::string, CalculatorMode> calculatorModeByName;
Copy link
Collaborator

Choose a reason for hiding this comment

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

You need #include <string>. Clang's error messages for this are terribly unhelpful :(

Copy link
Member Author

Choose a reason for hiding this comment

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

Eh... I was misled by a fact it builds on gcc. Headers seemed to be OK.

const sf::Vector2u output_image_size = source_image.getSize();
QImage output_image(output_image_size.x, output_image_size.y, QImage::Format_RGB32);

for (int y = 0; y < output_image_size.y; ++y)
Copy link
Collaborator

Choose a reason for hiding this comment

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

unsigned int y to match output_image_size.y; same for x.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we should build it with -Wall enabled and probably, when all warnings will be fixed, with -Werror.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We used to have a warning from googletest during WangscapeTest compilation, but in the latest master build that error seems to be gone :)

Add CalculatorMode.cpp to Wangscape VS project.
@serin-delaunay
Copy link
Collaborator

serin-delaunay commented Aug 29, 2017

I've adapted the VS project to the new changes, and it builds fine. A few comments on the interface (probably they should be separate issues):

  1. "Select options file" is hidden behind the "File" menu; should be an obvious button.
  2. When loading the options file, a label at the bottom has text set to a file path. If the path is long, the whole window expands to allow it to be displayed. The effect is quite jarring, and very long paths could cause problems, so I think the path should be cut off, or the label should have multiple lines.
  3. The program becomes unresponsive during tileset generation; could that be done in a separate thread?
  4. alphaCalculatorTopTwoPower should be in the Alpha tab of the options editor when TopTwo or Dither is selected.
  5. With "Use default output directory" turned off, if I try to save the tileset I get a folder selector widget named "Select output directory". If I type the name of a nonexistent folder there, an error message is displayed. Instead, it should create the folder and return that path.
  6. When quitting the program (after triggering the above message and saving successfully to a non-default output directory), I get a boost::filesystem::filesystem_error, apparently generated at MainWindow.cpp:95 (boost::filesystem::create_directories(image_dir);), with image_dir=="". This was from a release build, so the error may in fact occur on a different line.
  7. CalculatorMode.cpp should be included in the Wangscape VS project.
  8. Finally, this is really awesome!

I think 6. and 7. should be fixed, and then I'm happy to merge this. If you want to include a QtCreator project or my VS project files (https://github.com/serin-delaunay/Wangscape/tree/gui-vs-2) for WangscapeGui, that's OK too.

@hryniuk
Copy link
Member Author

hryniuk commented Aug 30, 2017

Thanks a lot for testing. I've included your commit and fixed 6. issue. I'll create separate issues for all points you mentioned and my suggestions I've wrote down during implementation.

@serin-delaunay
Copy link
Collaborator

Looks like my VS project isn't working on AppVeyor. It's fixed in 3a0ae18.

This should allow uic and moc to run.
@serin-delaunay serin-delaunay merged commit 741b3a9 into Wangscape:master Aug 30, 2017
@serin-delaunay
Copy link
Collaborator

Merged. Great work! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants