Add object reference property type #1408

simlrh · 2016-12-08T15:02:18Z

Adds an "object" property type; really just another integer but with some UI for easily setting this as the ID of an object in the map.

Similar to the text editor, it's a spinbox for if you already know the ID, or click the button to open a dialog where you can do a text search over all the objects.

Addresses the use case in #707, although only the object ref, not the fancy connecting lines etc!

simlrh · 2016-12-08T15:48:06Z

Hmm. I think there is propbably a simple solution to these build errors, but could someone familiar with Qt point me toward the problem?

bjorn · 2016-12-08T21:10:52Z

Hmm. I think there is probably a simple solution to these build errors, but could someone familiar with Qt point me toward the problem?

The Travis CI build started failing for some other reason, I'm not sure yet what to do about it. The AppVeyor build is failing because you added the new files only to the qmake project, but AppVeyor does the builds using Qbs.

bjorn · 2016-12-08T21:17:31Z

Just having looked quickly over the code, I think this is a really great start! I'll be sure to try it out soon (probably Monday or Tuesday) and do a more detailed review of the code.

src/tiled/objectiddialog.cpp

bjorn

Overal a nice change and I can see you paid a lot of attention to detail!

I've added some comments, some of which are small tasks but I also asked about a bigger change (using MapObjectModel in the ObjectIdDialog). Just let me know what you'd like to do.

bjorn · 2016-12-12T11:02:44Z

src/libtiled/properties.h

@@ -41,6 +41,10 @@ struct FilePath {
    QString absolutePath;
 };

+struct ObjectId {
+    int id;


This could use an initializing constructor, setting id to 0.

bjorn · 2016-12-12T11:07:17Z

src/tiled/objectiddialog.cpp

+    Utils::restoreGeometry(this);
+
+    if (MapDocument *document = DocumentManager::instance()->currentDocument()) {
+        QTableWidget *tableWidget = mUi->tableWidget;


Rather than using a QTabeWidget and copying all objects into it, did you try using a QTreeView and re-using the MapObjectModel? When combined with a QSortFilterProxyModel, you can still have sorting and filtering work. Also, you could add an ID column to MapObjectModel, but hide it in the ObjectsView (in objectsdock.cpp).

Thanks for this suggestion, this was the first Qt project I've looked at so this was just the first way I figured out to display the items. I'll change this and the QLineEdit.

bjorn · 2016-12-12T11:09:29Z

src/tiled/objectiddialog.cpp

+        foreach (QTableWidgetItem *item, items) {
+            if (item->column() == 0) {
+                tableWidget->selectRow(item->row());
+                lineEdit->setText(QStringLiteral(""));


Please use QString() to make an empty string. However, in this case, you should use lineEdit->clear().

bjorn · 2016-12-12T11:16:11Z

src/tiled/objectiddialog.cpp

+    connect(mUi->lineEdit, &QLineEdit::textChanged, this, &ObjectIdDialog::onTextChanged);
+    connect(mUi->tableWidget, &QTableWidget::itemSelectionChanged, this, &ObjectIdDialog::onItemSelectionChanged);
+    connect(mUi->tableWidget, &QTableWidget::itemDoubleClicked, this, &ObjectIdDialog::onItemDoubleClicked);
+    connect(mUi->pushButton, &QPushButton::clicked, this, &ObjectIdDialog::onButtonClicked);


Rather than having a separate "Clear" button, please use the functionality built into QLineEdit: http://doc.qt.io/qt-5/qlineedit.html#clearButtonEnabled-prop. I think it's not needed that clicking this button also clears the selection in the list.

bjorn · 2016-12-12T11:21:57Z

src/tiled/tiled.pro

-    reversingproxymodel.cpp
+    reversingproxymodel.cpp \
+    objectiddialog.cpp \
+    objectidedit.cpp


I know it's annoying that it doesn't do it automatically, but please keep these lists sorted. It helps avoid merge conflicts.

bjorn · 2016-12-12T11:39:31Z

src/tiled/variantpropertymanager.cpp

+        if (typeId == objectIdTypeId()) {
+            ObjectId id = value.value<ObjectId>();
+
+            if (MapDocument *document = DocumentManager::instance()->currentDocument()) {


Hmm, this may be a little problematic. There is no guarantee that the current document is the document to which the given ObjectId value is referring. Most of the time it will be true, but for example in the Tile Collision Editor, the objects have their own ID space (though it's a little hacky). I think the only real solution there would be to make the ObjectId contain also a Map*, but actually that won't work for the tile collision editor case since it uses a bare ObjectGroup instance. And if we add an ObjectGroup* to the ObjectId then we'd have to go and keep that up to date, which would be annoying as well.

What are you thoughts on this?

How about a Map* and an int referring to tile id for tile collision objects, or 0 for other objects? Does that uniquely identify all objects to your knowledge?

Hmm, no, you'd need a Tileset* as well. I think adding a Map* and a Tile* would work.

As long as this can be exported into the tmx as tilesets don't have ids. Perhaps in the file it could refer to the tile gid.

In the TMX file there should always just be the ID. The Map* and Tile* are only needed for the VariantPropertyManager to have enough context information to display the name of the object. In the TMX file, this context is clear.

I don't think I follow; I understand the Map* can be omitted, but say there is a property referring to object id 1, and there is an object id 1 in an object layer and another object id 1 in a tile collission object group, how are they differentiated?

They are differentiated by context. An object on a tile collision layer can't ever refer to an object on a tile collision layer from another tile, nor can it refer to an object on a map. So when the Tile* is set instead of the Map*, we need to find the referred object in the ObjectGroup associated with that tile.

bjorn · 2016-12-12T11:41:05Z

src/tiled/variantpropertymanager.cpp

+                            if (object->name().length())
+                                label.append(object->name());
+                            else
+                                label.append(QStringLiteral("Unnamed object"));


Please use tr("Unnamed object"), so that it can be translated.

bjorn · 2016-12-12T11:43:54Z

src/tiled/variantpropertymanager.cpp

+                    }
+                }
+            }
+            return QStringLiteral("%1: Object not found").arg(QString::number(id.id));


This should be tr("%1: Object not found").arg...

Also, I think a special text would be good for when the id is 0, which can never refer to a valid object. Maybe just "Unset".

This commit was based heavily on simlrh's PR of the same name, mapeditor#1408. Addresses some parts of issue mapeditor#707.

bjorn · 2020-01-08T19:38:14Z

Closing this now that it is superseded by #2712.

Add object reference property type

c5fc4cc

Fix qbs build and warnings

8e3a8bd

bjorn reviewed Dec 10, 2016

View reviewed changes

src/tiled/objectiddialog.cpp Outdated Show resolved Hide resolved

Remove redundant check

991d9fb

bjorn reviewed Dec 12, 2016

View reviewed changes

Phlosioneer added a commit to Phlosioneer/tiled that referenced this pull request Dec 22, 2019

Add object reference property type

a905e92

This commit was based heavily on simlrh's PR of the same name, mapeditor#1408. Addresses some parts of issue mapeditor#707.

Phlosioneer mentioned this pull request Dec 22, 2019

Add object reference property type #2712

Merged

bjorn closed this Jan 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add object reference property type #1408

Add object reference property type #1408

simlrh commented Dec 8, 2016

simlrh commented Dec 8, 2016

bjorn commented Dec 8, 2016

bjorn commented Dec 8, 2016

bjorn left a comment

bjorn Dec 12, 2016

bjorn Dec 12, 2016

simlrh Dec 12, 2016

bjorn Dec 12, 2016

bjorn Dec 12, 2016

bjorn Dec 12, 2016

bjorn Dec 12, 2016

simlrh Dec 12, 2016

bjorn Dec 12, 2016

simlrh Dec 12, 2016

bjorn Dec 12, 2016

simlrh Dec 12, 2016

bjorn Dec 13, 2016

bjorn Dec 12, 2016

bjorn Dec 12, 2016

bjorn commented Jan 8, 2020

Add object reference property type #1408

Add object reference property type #1408

Conversation

simlrh commented Dec 8, 2016

simlrh commented Dec 8, 2016

bjorn commented Dec 8, 2016

bjorn commented Dec 8, 2016

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

Choose a reason for hiding this comment

bjorn commented Jan 8, 2020