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

Export of tilesets to Lua (#1213) #1835

Merged
merged 20 commits into from Dec 20, 2017

Conversation

Projects
None yet
2 participants
@radman0x
Contributor

radman0x commented Dec 14, 2017

These changes were prompted following my forum post regarding this. The closest issue to what I raised is #1213, but the scope of that appears to be far wider that what I would like / am attempting. Simply I just want to be able to work on a tileset in isolation and then export that to a .lua file. The discussion in #1213 deals with a lot of issues which may impact this but are outside the driving reason for these particular changes.

With that in mind the changes I have here go a very small way toward addressing #1213. The changes simply enable the 'Export As' option for tilesets and I've implemented / modified output to .lua files (the others will just error currently).

These changes are intentionally rough and I'd like to get feedback from @bjorn regarding their suitability from an architecture standpoint etc.

WIP export of tileset to Lua (#1213)
Rough tracer implementation of exporting a tileset directly to a .lua
file. Needs refinement before being finalised.
Show outdated Hide outdated src/plugins/lua/luaplugin.cpp Outdated
@bjorn

Hey there, thanks for your work on this issue!

I've commented about a design issue we're having with the file format interfaces, and I made several comments regarding coding style (these are non-exhaustive, I try to comment on each issue only once). Let me know what you think!

Show outdated Hide outdated src/libtiled/mapformat.h Outdated
Show outdated Hide outdated src/tiled/document.h Outdated
Show outdated Hide outdated src/tiled/document.h Outdated
Show outdated Hide outdated src/tiled/mainwindow.cpp Outdated
Show outdated Hide outdated src/tiled/mainwindow.cpp Outdated
Show outdated Hide outdated src/tiled/mainwindow.cpp Outdated
Show outdated Hide outdated src/tiled/mainwindow.cpp Outdated
@radman0x

This comment has been minimized.

Show comment
Hide comment
@radman0x

radman0x Dec 14, 2017

Contributor

@bjorn, as far as the style changes go I'm happy to fix all of them up. Mainly they're the product of my IDE being set up contrary, 2 space indent, *& attached to type rather than var name etc. My next round of changes will be less hack and I'll try to adhere everywhere, if I miss any please point it out as my eyes are ingrained to looking for different formatting.

Putting this here so as not to make the same response on each issue raised.

Contributor

radman0x commented Dec 14, 2017

@bjorn, as far as the style changes go I'm happy to fix all of them up. Mainly they're the product of my IDE being set up contrary, 2 space indent, *& attached to type rather than var name etc. My next round of changes will be less hack and I'll try to adhere everywhere, if I miss any please point it out as my eyes are ingrained to looking for different formatting.

Putting this here so as not to make the same response on each issue raised.

Updated LuaPlugin style (#1213)
Working but code is VERY rough, needs tidy.

* LuaPlugin now uses Tiled::Plugin
* FileFormat put in it's own file fileformat.h
* FormatHelper now works for Tilesetformat and MapFormat
@radman0x

This comment has been minimized.

Show comment
Hide comment
@radman0x

radman0x Dec 14, 2017

Contributor

@bjorn, I've followed through on the design change.

Had to abandon the multiple inheritance route for LuaPlugin, QObject hard disallows multiple inheritance so inheriting from FileFormat is impossible.

Updated LuaPlugin to be like JsonPlugin and exporting works.

Code needs to some significant tidy up, I've just pushed it across the line and I've spent no time to review. I think with these changes exports to other types for Tilesets should just work too but I haven't checked.

Let me know what you think. Probably won't be able to make anymore changes for a couple of days though!

Contributor

radman0x commented Dec 14, 2017

@bjorn, I've followed through on the design change.

Had to abandon the multiple inheritance route for LuaPlugin, QObject hard disallows multiple inheritance so inheriting from FileFormat is impossible.

Updated LuaPlugin to be like JsonPlugin and exporting works.

Code needs to some significant tidy up, I've just pushed it across the line and I've spent no time to review. I think with these changes exports to other types for Tilesets should just work too but I haven't checked.

Let me know what you think. Probably won't be able to make anymore changes for a couple of days though!

@bjorn

I think design-wise it's looking much better now. I've had a closer look at the code which resulted in many small comments.

Show outdated Hide outdated src/libtiled/mapwriter.cpp Outdated
Show outdated Hide outdated src/libtiled/mapwriter.h Outdated
Show outdated Hide outdated src/libtiled/tilesetformat.h Outdated
Show outdated Hide outdated src/libtiled/mapformat.h Outdated
Show outdated Hide outdated src/plugins/lua/luaplugin.h Outdated
Show outdated Hide outdated src/tiled/mainwindow.cpp Outdated
// Check if writer will overwrite existing files here because some writers
// could save to multiple files at the same time. For example CSV saves
// each layer into a separate file.
QStringList outputFiles = exportDetails.mFormat->outputFiles(mapDocument->map(), exportDetails.mFileName);

This comment has been minimized.

@bjorn

bjorn Dec 19, 2017

Owner

This outputFiles call and related overwrite check should technically also by applied when exporting tilesets, even though right now we know that there are no tileset formats that export multiple files. The code could just be moved into chooseExportDetails, right?

@bjorn

bjorn Dec 19, 2017

Owner

This outputFiles call and related overwrite check should technically also by applied when exporting tilesets, even though right now we know that there are no tileset formats that export multiple files. The code could just be moved into chooseExportDetails, right?

This comment has been minimized.

@radman0x

radman0x Dec 20, 2017

Contributor

This section relies on MapFormat::outputFiles and this isn't currently a part of TilesetFormat which is why I separated it from chooseExportDetails, if it was then it could easily be included.

What would you like done here?

@radman0x

radman0x Dec 20, 2017

Contributor

This section relies on MapFormat::outputFiles and this isn't currently a part of TilesetFormat which is why I separated it from chooseExportDetails, if it was then it could easily be included.

What would you like done here?

This comment has been minimized.

@bjorn

bjorn Dec 20, 2017

Owner

Sorry, I didn't realize outputFiles was only part of the MapFormat. I see in the current code it was part of FileFormat, but that didn't make much sense since it receives a map as parameter. So it's good that you moved it to MapFormat and then you can disregard this comment. :-)

@bjorn

bjorn Dec 20, 2017

Owner

Sorry, I didn't realize outputFiles was only part of the MapFormat. I see in the current code it was part of FileFormat, but that didn't make much sense since it receives a map as parameter. So it's good that you moved it to MapFormat and then you can disregard this comment. :-)

Show outdated Hide outdated src/tiled/mainwindow.cpp Outdated
Show outdated Hide outdated src/tiled/mainwindow.cpp Outdated
Show outdated Hide outdated src/tiled/mainwindow.cpp Outdated
@radman0x

This comment has been minimized.

Show comment
Hide comment
@radman0x

radman0x Dec 19, 2017

Contributor

Thanks for the detailed feedback @bjorn, I'll hopefully be able to integrate all of that and get a final changeset done today.

Contributor

radman0x commented Dec 19, 2017

Thanks for the detailed feedback @bjorn, I'll hopefully be able to integrate all of that and get a final changeset done today.

Show outdated Hide outdated src/plugins/lua/luaplugin.cpp Outdated
@radman0x

This comment has been minimized.

Show comment
Hide comment
@radman0x

radman0x Dec 20, 2017

Contributor

Phew! Finally got those cross compile build errors under control :O Changes should now be tidied and I've integrated all comments, barring those I've flagged with a question. It looks like exporting to json also works now for free and I think that's true for any of the types that tilesets are supported for.

Somewhat related question; Why is one of the Windows compilers MSVC2013 and not something more recent? Also why the need for multiple different compilers: Mingw, MSVC, clang, gcc etc? (I'm sure there's a good reason)

Contributor

radman0x commented Dec 20, 2017

Phew! Finally got those cross compile build errors under control :O Changes should now be tidied and I've integrated all comments, barring those I've flagged with a question. It looks like exporting to json also works now for free and I think that's true for any of the types that tilesets are supported for.

Somewhat related question; Why is one of the Windows compilers MSVC2013 and not something more recent? Also why the need for multiple different compilers: Mingw, MSVC, clang, gcc etc? (I'm sure there's a good reason)

@bjorn

Phew! Finally got those cross compile build errors under control :O Changes should now be tidied and I've integrated all comments, barring those I've flagged with a question. It looks like exporting to json also works now for free and I think that's true for any of the types that tilesets are supported for.

Yeah! What was the problem with the inheriting constructors? I think I never used them before, but since we do use C++11 I thought it might be fine. Just problems with MSVC2013 compatibility?

Somewhat related question; Why is one of the Windows compilers MSVC2013 and not something more recent? Also why the need for multiple different compilers: Mingw, MSVC, clang, gcc etc? (I'm sure there's a good reason)

Using different compilers helps checking the code for portability and errors, since one compiler may show warnings that others do not, and one compiler may compile code because it supports some language extension that others do not. But also:

  • clang is used on macOS because there isn't anything else.
  • Visual Studio is used for Windows 64-bit builds because Qt doesn't ship a version for MinGW 64.

Also, I'd love to move to a more recent version of MSVC, and I've already tried (6988aa3), but it will need adjustments for the installer and I couldn't immediately figure out the right thing to do there (see the build log). Any help in this area would be greatly appreciated!

See all the inline comments for the remaining issues I've found. I hope I've also addressed all questions.

Show outdated Hide outdated src/plugins/lua/luaplugin.cpp Outdated
// Check if writer will overwrite existing files here because some writers
// could save to multiple files at the same time. For example CSV saves
// each layer into a separate file.
QStringList outputFiles = exportDetails.mFormat->outputFiles(mapDocument->map(), exportDetails.mFileName);

This comment has been minimized.

@bjorn

bjorn Dec 20, 2017

Owner

Sorry, I didn't realize outputFiles was only part of the MapFormat. I see in the current code it was part of FileFormat, but that didn't make much sense since it receives a map as parameter. So it's good that you moved it to MapFormat and then you can disregard this comment. :-)

@bjorn

bjorn Dec 20, 2017

Owner

Sorry, I didn't realize outputFiles was only part of the MapFormat. I see in the current code it was part of FileFormat, but that didn't make much sense since it receives a map as parameter. So it's good that you moved it to MapFormat and then you can disregard this comment. :-)

Show outdated Hide outdated src/tiled/tilesetdocument.h Outdated
@@ -22,13 +22,12 @@
#include "document.h"
#include "tileset.h"
#include "tilesetformat.h"

This comment has been minimized.

@bjorn

bjorn Dec 20, 2017

Owner

I believe you shouldn't need this include and can keep relying on the forward declaration.

@bjorn

bjorn Dec 20, 2017

Owner

I believe you shouldn't need this include and can keep relying on the forward declaration.

This comment has been minimized.

@radman0x

radman0x Dec 20, 2017

Contributor

Same reasoning as described above for mapdocument.h

@radman0x

radman0x Dec 20, 2017

Contributor

Same reasoning as described above for mapdocument.h

Show outdated Hide outdated src/tiled/mapdocument.h Outdated
Show outdated Hide outdated src/plugins/lua/luaplugin.cpp Outdated
Show outdated Hide outdated src/libtiled/tilesetformat.h Outdated
Show outdated Hide outdated src/plugins/lua/luaplugin.h Outdated
Show outdated Hide outdated src/plugins/lua/luaplugin.h Outdated
Show outdated Hide outdated src/plugins/lua/luaplugin.cpp Outdated
@radman0x

This comment has been minimized.

Show comment
Hide comment
@radman0x

radman0x Dec 20, 2017

Contributor

Hmmm I took a look at the build log you linked and it looks like your container / build node just doesn't have the newer version of MSVC installed:

C:\projects\tiled\dist\win\installer.wxs(96) : error LGHT0103 : The system cannot find the file 'C:/Program Files (x86)/Microsoft Visual Studio 14.0/VC/bin/amd64/../..\redist\x64\Microsoft.VC120.CRT\msvcp120.dll'.

Not sure how you would get it installed but it looks like everything compiled fine, just it couldn't link...

Actually I think it's trying to link an older version of the dll msvcp120.dll where it should be getting msvcp140.dll or something similar. Chances are this is hardcoded in the Wix file. There's also a mixture of / and \ in the path, but that's "probably" not the problem.

I do have some experience with Wix so if I find myself on Windows I might try to build it and see if I can work it out quickly.

Contributor

radman0x commented Dec 20, 2017

Hmmm I took a look at the build log you linked and it looks like your container / build node just doesn't have the newer version of MSVC installed:

C:\projects\tiled\dist\win\installer.wxs(96) : error LGHT0103 : The system cannot find the file 'C:/Program Files (x86)/Microsoft Visual Studio 14.0/VC/bin/amd64/../..\redist\x64\Microsoft.VC120.CRT\msvcp120.dll'.

Not sure how you would get it installed but it looks like everything compiled fine, just it couldn't link...

Actually I think it's trying to link an older version of the dll msvcp120.dll where it should be getting msvcp140.dll or something similar. Chances are this is hardcoded in the Wix file. There's also a mixture of / and \ in the path, but that's "probably" not the problem.

I do have some experience with Wix so if I find myself on Windows I might try to build it and see if I can work it out quickly.

@bjorn

This comment has been minimized.

Show comment
Hide comment
@bjorn

bjorn Dec 20, 2017

Owner

Actually I think it's trying to link an older version of the dll msvcp120.dll where it should be getting msvcp140.dll or something similar. Chances are this is hardcoded in the Wix file. There's also a mixture of / and \ in the path, but that's "probably" not the problem.

Indeed it's just trying to include the wrong version of the DLL in the installer. I do not know which DLL is the right one to ship for MSVC 2015. If it's just a change of file name, then it would need to be updated in dist/win/installer.wxs and dist/distribute.qbs.

Owner

bjorn commented Dec 20, 2017

Actually I think it's trying to link an older version of the dll msvcp120.dll where it should be getting msvcp140.dll or something similar. Chances are this is hardcoded in the Wix file. There's also a mixture of / and \ in the path, but that's "probably" not the problem.

Indeed it's just trying to include the wrong version of the DLL in the installer. I do not know which DLL is the right one to ship for MSVC 2015. If it's just a change of file name, then it would need to be updated in dist/win/installer.wxs and dist/distribute.qbs.

@radman0x

This comment has been minimized.

Show comment
Hide comment
@radman0x

radman0x Dec 20, 2017

Contributor

The name will be with a 14 replacing 12 where it is currently. Confusingly Visual Studio versioning is separate from MSVC++ versioning. Visual Studio 2017 ships with MSVC++ 14.1 :P

I'd also say that you can probably comfortably jump to an even newer compiler, e.g. Visual Studio 2017. There are unlikely to be any compilation incompatibilities.

Contributor

radman0x commented Dec 20, 2017

The name will be with a 14 replacing 12 where it is currently. Confusingly Visual Studio versioning is separate from MSVC++ versioning. Visual Studio 2017 ships with MSVC++ 14.1 :P

I'd also say that you can probably comfortably jump to an even newer compiler, e.g. Visual Studio 2017. There are unlikely to be any compilation incompatibilities.

@radman0x

This comment has been minimized.

Show comment
Hide comment
@radman0x

radman0x Dec 20, 2017

Contributor

WRT inheriting constructors, It's an addition from C++11 that allows a using declaration(see Inheriting Constructors section) of a constructor that essentially allows the derived class to say, I expose the same constructors as my base. This is convenient in not having to repeat code. MSVC2013 is the only one that had an issue, it really is pretty stone age now as far as the new C++ standards go (C++11, C++14, C++17).

As an aside there are also delegating constructors which let you call a constructor of the same class (just like a base class) to help eliminate code duplication if constructors share a lot of the same initialisation logic.

Contributor

radman0x commented Dec 20, 2017

WRT inheriting constructors, It's an addition from C++11 that allows a using declaration(see Inheriting Constructors section) of a constructor that essentially allows the derived class to say, I expose the same constructors as my base. This is convenient in not having to repeat code. MSVC2013 is the only one that had an issue, it really is pretty stone age now as far as the new C++ standards go (C++11, C++14, C++17).

As an aside there are also delegating constructors which let you call a constructor of the same class (just like a base class) to help eliminate code duplication if constructors share a lot of the same initialisation logic.

Minor touch ups from review (#1213)
* Rename of standalone to embedded for semantic correctness
* Moved writeColor to a static member
* Misc code style adjustments
@bjorn

This comment has been minimized.

Show comment
Hide comment
@bjorn

bjorn Dec 20, 2017

Owner

The name will be with a 14 replacing 12 where it is currently. Confusingly Visual Studio versioning is separate from MSVC++ versioning. Visual Studio 2017 ships with MSVC++ 14.1 :P

Ok, let's see what 33637fa yields.

As an aside there are also delegating constructors which let you call a constructor of the same class (just like a base class) to help eliminate code duplication if constructors share a lot of the same initialisation logic.

Yeah, I've been using those a lot already so VS 2013 seems to support those fine.

Owner

bjorn commented Dec 20, 2017

The name will be with a 14 replacing 12 where it is currently. Confusingly Visual Studio versioning is separate from MSVC++ versioning. Visual Studio 2017 ships with MSVC++ 14.1 :P

Ok, let's see what 33637fa yields.

As an aside there are also delegating constructors which let you call a constructor of the same class (just like a base class) to help eliminate code duplication if constructors share a lot of the same initialisation logic.

Yeah, I've been using those a lot already so VS 2013 seems to support those fine.

Allocate the LuaWriter on the stack
Also made a few more methods static and renamed mMapDir to mDir since it
can also be the tileset directory.
@bjorn

bjorn approved these changes Dec 20, 2017

@radman0x

This comment has been minimized.

Show comment
Hide comment
@radman0x

radman0x Dec 20, 2017

Contributor

Huzzah! I'm AFK for the rest of the night so it's good to hear!

Contributor

radman0x commented Dec 20, 2017

Huzzah! I'm AFK for the rest of the night so it's good to hear!

@bjorn bjorn merged commit 33c3b8b into bjorn:master Dec 20, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment