Creating template instances #1652

thabetx · 2017-07-14T18:55:08Z

This pull request is part of the Reusable Object Templates project.

Added a template instance creation tool similar to the insert tile object tool. Also enabled dragging and dropping from the template dock into the map.

Right now, the functionality of the project can be tested without the need of manually editing files.

bjorn

Some review feedback.

bjorn · 2017-07-17T10:04:08Z

src/tiled/objecttemplatemodel.h

@@ -53,6 +53,11 @@ class ObjectTemplateModel : public QAbstractItemModel
    bool addNewDocument(TemplateGroupDocument *document);
    bool addTemplateGroup(TemplateGroup *templateGroup);
    bool saveObjectToDocument(MapObject *object, QString name, int documentIndex);
+    ObjectTemplate *toObjectTemplate(const QModelIndex &index) const;
+
+    Qt::ItemFlags flags(const QModelIndex &index) const;


Should be marked as override.

bjorn · 2017-07-17T10:05:47Z

src/tiled/mapscene.cpp

 void MapScene::dragEnterEvent(QGraphicsSceneDragDropEvent *event)
 {
-    event->ignore();
+    Q_UNUSED(event);


This function will still need to explicitly ignore the event when it does not contain the TEMPLATES_MIMETYPE, to make sure that maps and tilesets dropped on Tiled will get opened (also when dropped on the map view). I guess the comment you removed could have been more explicit about this. :-)

bjorn · 2017-07-17T10:25:59Z

src/tiled/mapscene.cpp

+    stream >> groupFileName >> templateId;
+
+    TemplateGroup *group = templateManager->findTemplateGroup(groupFileName);
+    const ObjectTemplate *objectTemplate = group->findTemplate(templateId);;


Please check both the group and the objectTemplate pointers, because you can't assume they will be valid. The dropped data could potentially come from another application, or just another instance of Tiled that hasn't got the same template groups loaded.

bjorn · 2017-07-17T10:35:59Z

src/tiled/templatesdock.cpp

+
+    setSelectionMode(QAbstractItemView::SingleSelection);
+    setDragEnabled(true);
+    setDropIndicatorShown(true);


That seems a little pointless for a drag-only view, but I guess it could be enabled when we would later enable dragging templates from one group to another (which I think could be a good reason to try using UUIDs for template references).

bjorn · 2017-07-17T10:38:06Z

src/tiled/templatesdock.cpp

 TemplatesView::TemplatesView(QWidget *parent)
    : QTreeView(parent)
 {
    setUniformRowHeights(true);
    setHeaderHidden(true);

+    connect(this, SIGNAL(pressed(QModelIndex)), SLOT(onPressed(QModelIndex)));


For new connections, please prefer the modern connect syntax:

connect(this, &QAbstractItemView::pressed, this, &TemplatesView::onPressed);

Qt Creator has a fixup for doing the conversion when you put your cursor on it and press Ctrl+Enter (or right-click and choose "Refactor").

bjorn · 2017-07-17T10:40:21Z

src/tiled/templatesdock.h

@@ -43,6 +45,8 @@ class TemplatesDock : public QDockWidget
    TemplatesDock(QWidget *parent = nullptr);
    ~TemplatesDock();

+    TemplatesView *templatesView() const;


Instead of exposing the templates view, I would suggest to add the currentTemplateChanged signal also to the dock.

In that case the view emits the change then the dock emits it again, right?

Yes, you can directly connect one signal to another signal to achieve this.

bjorn · 2017-07-17T10:53:21Z

src/tiled/createtemplatetool.cpp

+{
+    const MapRenderer *renderer = mapDocument()->renderer();
+
+    const QSize imgSize = mNewMapObjectItem->mapObject()->cell().tile()->size();


This crashed for me on the first try, because it assumes the object is a tile object and that its tile is loaded. I think it does make sense to center the object on the mouse, but doing this in the general case will be a little more complicated. You can try starting with using MapRenderer::boundingRect, which should do the trick for all non-rotated objects on non-isometric maps.

Centering correctly on isometric maps will unfortunately require handling tile objects and other objects differently.

I have a tendency to test only tile objects and forget about the others.

I tried using the bounding rectangle dimensions to calculate the shift instead of the tile, but when clicking, each type has a different starting point, the tile objects uses the bottom left, ellipses, rectangles and text objects will use the top left, while a polyline or a polygon will use their first point.

The easy solution would be just use pos without any shifting. but I think we can handle them to always be centered using the bounding rectangle but with different shift for each type. Drag and drop uses the default starting point so it might be changed as well to be centered.

I think it will be fine for now to not bother with the centering at all. On the upside, it will give a clear indication to the user as to where the object is actually positioned, which will be beneficial also when this anchor point eventually becomes configurable.

bjorn · 2017-07-17T11:00:00Z

src/libtiled/map.cpp

+
+void Map::removeTemplateGroupReference(const TemplateGroup *templateGroup)
+{
+//    mTemplateGroupReferences[templateGroup]--;


Did this not work for some reason?

Btw, while it makes sense to automatically remove template group references, this is only relevant when saving the map. So rather than keeping reference counts, the map writer could also just iterate over all the objects to determine the list of template group references that need to be written. I would prefer that approach.

It worked fine but I wasn't sure which style to use. I will try the suggested approach it should be simpler.

bjorn · 2017-07-17T11:49:19Z

src/tiled/createobjecttool.h

@@ -74,6 +76,7 @@ public slots:
    MapObjectItem *mNewMapObjectItem;
    MapObjectItem *mOverlayPolygonItem;
    Tile *mTile;
+    ObjectTemplate *mObjectTemplate;


Why is this a member of CreateObjectTool rather than CreateTemplateTool?

The same question could be asked about mTile of course. That one is there for historical reasons and can be moved to CreateTileObjectTool since change 33e048b.

Used the icon of GIMP's select foreground tool.

bjorn

Approved!

I've still noticed a few small changes needed, which you can include in your next PR.

In tidmapper.h, TemplateRef is forward-declared with class, though it is declared as struct. Please make sure the forward-declaration is consistent with the actual declaration, to avoid compiler warnings when compiling with Clang or Visual Studio.

I assume you may have already dealt with the warnings about enumeration values not being handled in some switch statements in propertybrowser.cpp?

src/tiled/propertybrowser.cpp:529: warning: enumeration values 'ObjectTemplateType' and 'TemplateGroupType' not handled in switch [-Wswitch]
    switch (mObject->typeId()) {
            ^

bjorn · 2017-07-25T07:48:34Z

src/tiled/objecttemplatemodel.cpp

+{
+    Qt::ItemFlags defaultFlags = QAbstractItemModel::flags(index);
+
+   ObjectTemplate *objectTemplate = toObjectTemplate(index);


Indentation is off by one here.

thabetx · 2017-07-25T19:42:18Z

The warnings were not relevant to the pull request so I left them. I will fix them in the next PR.

thabetx · 2017-07-25T19:52:11Z

I think I will just add them to suppress the warnings without adding any functionality as the new object types are not used in the property browser.

bjorn · 2017-07-25T19:53:34Z

I think I will just add them to suppress the warnings without adding any functionality as the new object types are not used in the property browser.

Sounds reasonable.

bjorn requested changes Jul 17, 2017

View reviewed changes

thabetx added 2 commits July 18, 2017 17:31

Create template tool

9ea0586

Enable drag and drop for creating template instances

01bc2db

thabetx force-pushed the template-creation branch from a466151 to 01bc2db Compare July 18, 2017 15:42

thabetx added 2 commits July 18, 2017 20:02

Fix copying template instances

b13b6e4

Add icon for the template creation tool

d141b87

Used the icon of GIMP's select foreground tool.

bjorn approved these changes Jul 25, 2017

View reviewed changes

bjorn merged commit d141b87 into mapeditor:wip/templates Jul 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Creating template instances #1652

Creating template instances #1652

thabetx commented Jul 14, 2017

bjorn left a comment

bjorn Jul 17, 2017

bjorn Jul 17, 2017

bjorn Jul 17, 2017

bjorn Jul 17, 2017

bjorn Jul 17, 2017

bjorn Jul 17, 2017

thabetx Jul 17, 2017

bjorn Jul 18, 2017

bjorn Jul 17, 2017

thabetx Jul 18, 2017

bjorn Jul 18, 2017

bjorn Jul 17, 2017

thabetx Jul 17, 2017

bjorn Jul 17, 2017

bjorn left a comment

bjorn Jul 25, 2017

thabetx commented Jul 25, 2017

thabetx commented Jul 25, 2017

bjorn commented Jul 25, 2017

Creating template instances #1652

Creating template instances #1652

Conversation

thabetx commented Jul 14, 2017

bjorn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjorn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thabetx commented Jul 25, 2017

thabetx commented Jul 25, 2017

bjorn commented Jul 25, 2017