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

Export of tilesets to Lua (#1213) #1835

Merged
merged 20 commits into from Dec 20, 2017
Merged

Export of tilesets to Lua (#1213) #1835

merged 20 commits into from Dec 20, 2017

Conversation

radman0x
Copy link
Contributor

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.

Rough tracer implementation of exporting a tileset directly to a .lua
file. Needs refinement before being finalised.
@@ -199,14 +228,20 @@ static bool includeTile(const Tile *tile)
}

void LuaPlugin::writeTileset(LuaTableWriter &writer, const Tileset *tileset,
unsigned firstGid)
unsigned firstGid, bool standalone)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've just realised that I've got the semantics of this bool backwards. Please disregard as this will certainly be rectified if these changes move forward.

Copy link
Member

@bjorn bjorn left a comment

Choose a reason for hiding this comment

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

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!

@@ -130,6 +131,7 @@ class TILEDSHARED_EXPORT MapFormat : public FileFormat
* occurred. The error can be retrieved by errorString().
*/
virtual bool write(const Map *map, const QString &fileName) = 0;
virtual bool writeAsTileset(const Tiled::Tileset *tileset, const QString &fileName) { return false; }
Copy link
Member

Choose a reason for hiding this comment

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

Here is I think the main problem with the patch as-is. The MapFormat class implements a map format, not a tileset format. For that we have TilesetFormat. So, if the Lua plugin wants to support exporting of tilesets, it should add an instance of a class derived from TilesetFormat to the PluginManager.

I realize now that the LuaPlugin is using the old (but still supported) approach of directly implementing the MapFormat interface. Now, it will need to work similar to the JsonPlugin, in that it should implement the Tiled::Plugin interface and add objects in its initialize method (see JsonPlugin::initialize). I think it may be good to do a smaller PR first that only changes the LuaPlugin in this way, before continuing to add a LuaTilesetFormat class and enabling the Export action for tilesets.

Alternatively, it may be possible to derive the LuaPlugin from both MapFormat and TilesetFormat. I think the only problem here is the read function, which has the same signature but different return values. This could be fixed by renaming it to readMap and readTileset respectively. I'd be fine with such a change as well, and I think it may be easier to implement.

@@ -130,6 +138,9 @@ class Document : public QObject
Object *mCurrentObject; /**< Current properties object. */

bool mIgnoreBrokenLinks;

QString mLastExportFileName;
MapFormat* mExportFormat;
Copy link
Member

Choose a reason for hiding this comment

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

This should be QPointer<FileFormat>. The QPointer is needed because the format could get deleted (when the user disables the plugin), and it should be FileFormat because it needs to store either a MapFormat or TilesetFormat.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was one of the hacks that I made to just get it working. I copy pasted directly from MapDocument to Document which is why it's a MapFormat type. I'll look at making it a FileFormat as suggested to make it fit with the major design change.

I did have an issue here that you might be able to assist with. It looks like QPointer doesn't work with forward declared types, which is why I made it a raw pointer as a hack. Pretty sure there is a circular dependency somewhere between mapformat.h and here so including it wasn't an option. Any ideas on this front?

Copy link
Member

Choose a reason for hiding this comment

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

Regarding the moving from MapDocument to Document, that makes total sense so it's not much of a hack, as long as you do change the type to the more generic one. :-)

I think including mapformat.h in document.cpp should be enough to fix your compile issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll give that a go, that might just do it :)

QString lastExportFileName() const;
void setLastExportFileName(const QString &fileName);

MapFormat* exportFormat() const;
Copy link
Member

Choose a reason for hiding this comment

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

Coding style: please put the * against the method name. This is just for consistency with variables.

} else {
chosenFormat = format;
if (mapDocument)
{
Copy link
Member

Choose a reason for hiding this comment

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

Coding style: please put the { on the same line.

mSettings.setValue(QLatin1String("lastUsedExportFilter"), selectedFilter);

auto exportResult = false;
if ( auto definitelyMap = qobject_cast<MapDocument*>(mDocument) )
Copy link
Member

Choose a reason for hiding this comment

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

Coding style: please don't put spaces on the inside of parentheses.

auto exportResult = false;
if ( auto definitelyMap = qobject_cast<MapDocument*>(mDocument) )
{
exportResult = chosenFormat->write(definitelyMap->map(), fileName);
Copy link
Member

Choose a reason for hiding this comment

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

Coding style: indentation should be 4 spaces.

exportResult = chosenFormat->writeAsTileset(certainlyTileset->tileset().data(), fileName);
}

if ( ! exportResult )
Copy link
Member

Choose a reason for hiding this comment

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

Coding style: please put the ! against the variable name.

@radman0x
Copy link
Contributor Author

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

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
Copy link
Contributor Author

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

Copy link
Member

@bjorn bjorn left a comment

Choose a reason for hiding this comment

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

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.

@@ -74,7 +74,7 @@ class MapWriterPrivate
void writeMap(const Map *map, QIODevice *device,
const QString &path);

void writeTileset(const Tileset &tileset, QIODevice *device,
void writeTileset(const Tileset *tileset, QIODevice *device,
Copy link
Member

Choose a reason for hiding this comment

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

Why change this? You shouldn't need to change this file at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

The reason I changed this was to make it consistent with writeMap just above. It was an unnecessary change, just something I did while I was thinking about something else.

@@ -83,7 +83,7 @@ class TILEDSHARED_EXPORT MapWriter
* Error checking will need to be done on the \a device after calling this
* function.
*/
void writeTileset(const Tileset &tileset, QIODevice *device,
void writeTileset(const Tileset *tileset, QIODevice *device,
Copy link
Member

Choose a reason for hiding this comment

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

Please just let it be a reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -79,5 +102,3 @@ TILEDSHARED_EXPORT SharedTileset readTileset(const QString &fileName,
TILEDSHARED_EXPORT TilesetFormat *findSupportingTilesetFormat(const QString &fileName);

} // namespace Tiled

Q_DECLARE_INTERFACE(Tiled::TilesetFormat, "org.mapeditor.TilesetFormat")
Copy link
Member

Choose a reason for hiding this comment

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

Why remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like I put it on line 71 above as WritableTilesetFormat is a new addition and I was keeping it in the same spot relative to the TilesetFormat class. I can move this down if desired?


/**
* Reads the map and returns a new Map instance, or 0 if reading failed.
*/
virtual Map *read(const QString &fileName) = 0;
virtual Map *readMap(const QString &fileName) = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Since indeed multiple inheritance won't work (sorry I forgot about the QObject part), please don't still rename this function. There is no reason to change it anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

};


class LUASHARED_EXPORT LuaMapFormat : public Tiled::WritableMapFormat, public LuaUtilWriter
Copy link
Member

Choose a reason for hiding this comment

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

Please instantiate the LuaUtilWriter on the stack instead of inheriting from it. At least currently I don't see the reason for inheritance. Then you also avoid naming conflicts to you can drop the "Lua" from "writeMapLua" and "writeTilesetLua".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Inheritance was just a quick way of sharing the implementation across the two classes. It felt natural given that the methods were all pulled directly from the pevious objects private implementation. Composition works just as well though so I've done as requested.

// Remember export parameters, so subsequent exports can be done faster
mapDocument->setLastExportFileName(fileName);
mapDocument->setExportFormat(chosenFormat);
else if (auto tilesetDocument = qobject_cast<TilesetDocument*>(mDocument)) {
Copy link
Member

Choose a reason for hiding this comment

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

Coding style: else on the same line as the }.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

// 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);
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

, mFileName(fileName)
{}

bool valid() { return static_cast<bool>(mFormat); }
Copy link
Member

Choose a reason for hiding this comment

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

Naming style: please call this isValid. For implementation I prefer return mFormat != nullptr;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Format* mFormat;
QString mFileName;

ExportDetails() {}
Copy link
Member

Choose a reason for hiding this comment

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

Should initialize mFormat to nullptr.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

this);
if (!exportDetails.valid()) {
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

Coding style: for single-line bodies, leave out the brackets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@radman0x
Copy link
Contributor Author

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

{
return true;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bjorn, can you please provide some direction on how implement this. I could copy as best I can from other places but it'd be good to not waste my time doing the wrong thing if possible ;)

Copy link
Member

Choose a reason for hiding this comment

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

This format only implements writing so it doesn't "support" any file and should always return false. You can just remove this override though, since that's what WritableMapFormat already does.

@radman0x
Copy link
Contributor Author

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)

Copy link
Member

@bjorn bjorn left a comment

Choose a reason for hiding this comment

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

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.

{
return true;
Copy link
Member

Choose a reason for hiding this comment

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

This format only implements writing so it doesn't "support" any file and should always return false. You can just remove this override though, since that's what WritableMapFormat already does.

// 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);
Copy link
Member

Choose a reason for hiding this comment

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

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

@@ -63,6 +62,9 @@ class TilesetDocument : public Document
FileFormat *writerFormat() const override;
void setWriterFormat(TilesetFormat *format);

TilesetFormat* exportFormat() const override;
Copy link
Member

Choose a reason for hiding this comment

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

Coding style: please put * against the function name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -22,13 +22,12 @@

#include "document.h"
#include "tileset.h"
#include "tilesetformat.h"
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same reasoning as described above for mapdocument.h

MapFormat *readerFormat() const;
void setReaderFormat(MapFormat *format);

FileFormat *writerFormat() const override;
void setWriterFormat(MapFormat *format);

MapFormat *exportFormat() const;
void setExportFormat(MapFormat *format);
MapFormat* exportFormat() const override;
Copy link
Member

Choose a reason for hiding this comment

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

Coding style: please put * against the function name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

writer.writeValue(color.alpha());
writer.writeEndTable();
writer.setSuppressNewlines(false);
}
Copy link
Member

Choose a reason for hiding this comment

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

I see no reason for moving this function. Please just keep it where it was as static.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

} // namespace Tiled

Q_DECLARE_INTERFACE(Tiled::TilesetFormat, "org.mapeditor.TilesetFormat")
Q_DECLARE_OPERATORS_FOR_FLAGS(Tiled::FileFormat::Capabilities)
Copy link
Member

Choose a reason for hiding this comment

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

Please move this operators macro to fileformat.h.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

QString errorString() const override;

protected:
LuaUtilWriter mLuaWriter;
Copy link
Member

Choose a reason for hiding this comment

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

I suggested to allocate the LuaUtilWriter on the stack. Was there a reason it should be a member instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has mMapDir and is used across calls e.g. writeProperties which seems to use the member assuming it has been set previously. I could be wrong, please feel free to correct me.

Copy link
Member

Choose a reason for hiding this comment

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

I've pushed a change which puts it on the stack, since I wanted to make sure anyway that it would work. Now the mMapDir thing works even better I think, since by requiring it in the constructor we make sure that it is set appropriately. Only the static methods could be called without initializing it (though they aren't).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks great :)

@@ -45,23 +47,12 @@ class LuaTableWriter;
/**
* This plugin allows exporting maps as Lua files.
*/
class LUASHARED_EXPORT LuaPlugin : public Tiled::WritableMapFormat
class LuaUtilWriter
Copy link
Member

Choose a reason for hiding this comment

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

I would rename this class to just LuaWriter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

writer.writeKeyAndValue("name", tileset.name());
if (standalone) {
writer.writeKeyAndValue("firstgid", firstGid);
}
Copy link
Member

Choose a reason for hiding this comment

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

Coding style: in case of single-line body, please leave out the brackets (also above).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@radman0x
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

* Rename of standalone to embedded for semantic correctness
* Moved writeColor to a static member
* Misc code style adjustments
@bjorn
Copy link
Member

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.

Also made a few more methods static and renamed mMapDir to mDir since it
can also be the tileset directory.
@radman0x
Copy link
Contributor Author

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

@bjorn bjorn merged commit 33c3b8b into mapeditor:master Dec 20, 2017
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