Add object reference property type

[Phlosioneer]

This PR is based heavily on @simlrh's PR of the same name, #1408. It addresses all of @bjorn's comments on that PR.

This commit is a partial solution to #707. It allows a new property type for object ids. It also adds a dialog box to choose an object from a filterable, sortable list.

[Phlosioneer]

I don't understand what's going on with the build

For clang on linux, it never compiles the new files.
Actually, it never runs QBS at all.
Instead, it checks if a makefile exists, and if so, it skips generating a makefile via qbs:

cd src/ && ( test -e Makefile || /opt/qt56/bin/qmake /home/travis/build/bjorn/tiled/src/src.pro CONFIG+=tiled_zstd -o Makefile ) && make -f Makefile 
make[1]: Entering directory '/home/travis/build/bjorn/tiled/src'

And the makefile appears to be pulled from the master branch's cache?

$ export PATH=/usr/lib/ccache:$PATH
cache.1
Setting up build cache
$ export CASHER_DIR=${TRAVIS_HOME}/.casher
0.28s$ Installing caching utilities
0.00s2.11sattempting to download cache archive
fetching PR.2712/cache--linux-xenial-5970a08b8df6e4e591443a1970293bdf0335a0475604854da0f8fed2260226d8--compiler-clang.tgz
fetching PR.2712/cache-linux-xenial-5970a08b8df6e4e591443a1970293bdf0335a0475604854da0f8fed2260226d8--compiler-clang.tgz
fetching PR.2712/cache--compiler-clang.tgz
fetching master/cache--linux-xenial-5970a08b8df6e4e591443a1970293bdf0335a0475604854da0f8fed2260226d8--compiler-clang.tgz
found cache
cache.ccache
adding /home/travis/.ccache to cache

qmake is called, but it only runs for 0.01 seconds.

The other builds always run qbs.

[Phlosioneer]

I think I found the issue. That one build process is using .pro files instead of .qbs files. I pushed an updated .pro file.

Why is it the only one using .pro files...?

Could we use qbs config defaultProfile clang instead? It's also our slowest check.

[bjorn]

@Phlosioneer As long as we're still using the .pro files we need to make sure they are working, which that one build helps to ensure. The other build that is still using qmake is the snap built on build.snapcraft.io (which of course should also be moved to Qbs).

[bjorn]

Thanks for picking this one up again! I've commented a few quick observations.

[bjorn]

@Phlosioneer Please be aware that I've pushed some changes that already addressed a few of my comments. For now though I'm out of time this week and will continue Tuesday next week, so feel free to work on remaining tasks like trying to use MapObjectModel and introducing a lookup table for objects by ID.

It would also be nice to use the icon associated with objects in the Objects view, instead of the "missing image" icon which appears to be used right now:

[Phlosioneer]

Alright, thanks for the comments. I'll continue working on this.

[Phlosioneer]

Okay, assuming that passes the tests, that should address every point except the iterating-all-objects one, and the objects view image thing.

[Phlosioneer]

This is a lot more work than I expected for a seemingly simple feature, but it turns out to be quite tricky. I'm learning a ton about Qt in the process and I'm getting very familiar with Tiled's codebase, though.

It looks like another few commits' worth of work still to go. Trying to keep the commits small and descriptive to make reviewing this PR easier.

[bjorn]

This is a lot more work than I expected for a seemingly simple feature, but it turns out to be quite tricky. I'm learning a ton about Qt in the process and I'm getting very familiar with Tiled's codebase, though.

That's great. I'm itching to help but only have a bit of time in the evenings until Tuesday. Indeed this feature is more work than I had expected as well, but it's also what you get when you want to make the best of it. After all the initial patch already got the job done.

The object selection dialog is especially more tricky than I had anticipated. Yes, we already have a MapObjectModel, but it has editing features (visibility, renaming) and allows layers to be selected (we should disable that by not setting the selectable flag btw, instead of doing a workaround in ObjectsTreeView::selectionChanged). And then of course it doesn't work for tile collision shapes, but actually I hope we can rely on the dummy map document used by the tile collision editor.

Btw, we don't need to use the MapObjectModel instance from the MapDocument. We can instantiate another one and set some options on it, like setImmutable and setLayersSelectable, then we don't need the ImmutableRoleModel.

It looks like another few commits' worth of work still to go. Trying to keep the commits small and descriptive to make reviewing this PR easier.

That's working out great, thanks! At the end we can either squash it all or make a cleaned up set of commits for merging.

[Phlosioneer]

With those pushes, I'm almost done. I've just gotta address your comment just now, then I'm happy with it and you can review it on teusday.

Btw, we don't need to use the MapObjectModel instance from the MapDocument. We can instantiate another one and set some options on it, like setImmutable and setLayersSelectable, then we don't need the ImmutableRoleModel.

Wasn't the point of the refactoring to avoid copying all the values? Creating a new instance of MapObjectModel every time we edit an objectRef property seems quite counter-productive.

And then of course it doesn't work for tile collision shapes, but actually I hope we can rely on the dummy map document used by the tile collision editor.

With the route I went, I wasn't able to leverage them. I opted to have all objects on the tileset selectable, and we only have a dummy map document for the currently selected tile if I read the code correctly.

Ideally, we can refactor the code in this PR and the dummy map models into one tileset map model that understands how tiles can have objects? The most recent pushes were quite messy because I couldn't use an existing abstractdatamodel.

(we should disable that by not setting the selectable flag btw...)

I didn't know there were selectable flags. I'll go change that.

[Phlosioneer]

I also want to say thank you for putting so much work into this. I always leave half-finished projects online as a starting point for the next person with the same use case - but I think you're the first person to continue something I started instead of yelling at me for not finishing

😄

and now the dialog displays the objects of all tiles, which is bad for usability since there will likely be no good way to distinguish them.

The dialog should have the objects grouped by tile ID, in a tree...? It even displays the user-provided "type" field for the tiles.

And I don't really see a use-case for it. I think we should keep object references local to the map and/or tile for now.
It came as a consequence of adding Tile* to the object, which was required when I removed the check for the currently selected tile.

If we have a Tile* reference on it, then that reference either needs to be a cache value (set it when we need it), or it needs to be always-correct. Making it always-correct complicates map loading a lot. Property loading code now needs to know about the current tileset's pointer. And if you say "let's just leave it as a nullptr until after loading", we need to iterate over every property in every tile and every property of every object on those tiles to set the pointers after loading.

So I went with the "Tile* is a cached value" route.

I think that if we want to build anything on top of this feature in the future (like scripting), we need a less ambiguous identifier. Currently, code looking at just properties without respect for the object that owns the property (like propertybrowser) need to use static_cast<TilesetDocument*>(DocumentManager::instance()->document())->selectedTiles()[0] and all the assumptions built into that. And assuming it's the currently selected tile is already wrong sometimes, in the case where there
are multiple selected tiles.

It was also causing lots of corner cases when a property type (object ref) couldn't be placed on some things (tileset, wang set), and also couldn't be edited on multiple tiles simultaneously (if you select multiple tiles).
Edit: Another corner case I was struggling with is when properties are copy/pasted, because apparently that's something Tiled supports.

[bjorn]

I think ObjectRef can have a Tile* and a Map* (or Document* + tileId). They'd both be unset by default and the TMX reader would not need to worry about setting those members. It's only when we want to display the ObjectRef value in the Properties view that we need to set the pointer (we could also consider using a separate struct for this), and that's just because the widget displaying the value otherwise does not know its context (but the PropertyBrowser does).

In scripts, getting an ObjectRef property value should probably just return its EditableMapObject instance and setting it should take EditableMapObject parameter.

I guess when you copy/paste an object reference, eventually we may want to include a file name and tile ID, so that references to objects in other files could be made, but for now I think it's fine to just copy the object ID only.

[Phlosioneer]

Document* + tileId is ruled out, because the ObjectRef type is present in libtiled.

I still think treating the disambiguating values as "cached" is going to hamper other features in the future. For example, scripts calling MapObject.properties need to know the context of the objectRef as well; and if we ever want to add a "draw lines between connected objects" feature (as mentioned multiple times in the original issue), it'll be much harder to ensure those cached values are correct. For propertybrowser I could hook into the addProperty to ensure that the values were correct, but other UI elements might not be so easy.

I think we have disagreeing opinions on this that aren't going to be settled, though. My priority right now is to have this feature in tiled. So if the above doesn't change your mind, I'll do it your way.

[bjorn]

Are you open for a video chat about this tomorrow? I think we may just have a misunderstanding rather than a disagreement. I try to clarify a little here:

Document* + tileId is ruled out, because the ObjectRef type is present in libtiled.

That's right, but not if we introduce a separate struct for UI purposes.

For example, scripts calling MapObject.properties need to know the context of the objectRef as well

The value of those properties would be immediately mapped EditableMapObject instances, which should be possible in EditableObject::properties because we know the context.

[Phlosioneer]

The value of those properties would be immediately mapped EditableMapObject instances, which should be possible in EditableObject::properties because we know the context.

As is, we currently just return the underlying QVariantMap.

Are you open for a video chat about this tomorrow? I think we may just have a misunderstanding rather than a disagreement.

Sure. I'm available 12pm-9pm EST.

[Phlosioneer]

Actually, even the current implementation fails when object refs are accessed through a property - Qt has no way to turn QVariant(Tiled::ObjectRef) into a js value.

Edit: It also fails with File properties, spitting out a QVariant(Tiled::FilePath)

[bjorn]

Sure. I'm available 12pm-9pm EST.

Hmm, could you be available a little earlier perhaps? I am working on Tiled until about 11am EST (17:00 CET). Though we could also try chatting in my evening, starting between 2-3pm your time.

As is, we currently just return the underlying QVariantMap.

That's true, custom types like FilePath and ObjectRef won't work out of the box. I've added support for FilePath in change d13afac now. We could support ObjectRef in similar fashion, though I imagine automatic conversion to/from EditableMapObject would be preferable.

[bjorn]

@Phlosioneer I'll try to be around this evening. For this change, I think the main remaining change to do is the adjust the documentation. I hope you're also fine with these changes, but let me know if you have any concerns.

[Phlosioneer]

Immutable map objects model is now even more immutable

XD

I hope you're also fine with these changes, but let me know if you have any concerns.

If we're doing it this way, there are some design challenges we still need to solve. ObjectRefs are not allowed on tilesets, terrains, wang sets, wang colors, or object templates. Here's a list of the corner cases:

• Some objects can't have ObjectRef properties added to them. So addPropertyDialog needs to know whether the current object is allowed to have an ObjectRef property. The soultion is easy: add a boolean objectRefAllowed to its constructor, and create a new function on libtiled's object class called canHaveObjectRefProperty(). This leads to...
• The properties doc spawns AddPropertiesDialog, so it needs a reference to the object it's editing. Using DocumentManager::instance() to find the currently selected item doesn't work; you can edit the tileset's properties while a tile is selected. So the properties doc needs a new member variable and a function to set it, and the mapEditor and tilesetEditor need to keep that updated.
• The ObjectTypesEditor is in the same situation as the properties doc: it needs to spawn an AddPropertiesDialog. DocumentManager::instance() again doesn't work. So it needs a new member variable and a function to set it. But it's owned directly by MainWindow. There is no global location for the property browser's target, as far as I can tell, so it's going to need to go through MainWindow and through MapEditor and TilesetEditor to ask them what the current object is.
• The scripting interface also needs to prevent scripts from adding objectRef properties to the above objects.
• The map readers also need to know that tilesets and such can't have ObjectRef properties. If we load a map with an ObjectRef property on a tileset, the current code crashes, without debug message.
  • So we either need to sanitize properties after they're loaded, removing them or throwing errors as appropriate; or we need to tell our property-reading code whether it's allowed to read objectRefs.
• When a script sets an objectRef property on a tile or tile collision object, we need to sanitize that to ensure it references an object on the same tile.
• When a script detaches an object from one tile and attaches it to another, we need to sanitize that the same way.

On an unrelated note, I thought of a new corner case that both approaches will need to handle:

• when a script detaches an object with objectRefs, but then re-attaches it to another document, we need to remove those object refs.

[Phlosioneer]

This feels like a property MapObject should have.

[bjorn]

Yeah, I guess there are lots of places where we want to get the map of an object already.

[Phlosioneer]

Do we want to support the case where there's a more complex detached layer tree? Currently, this just looks at the immediate parent of the map object. If a user does this:

root = new GroupLayer();
child1 = new ObjectLayer();
child2 = new ObjectLayer();
root.addLayer(child1);
root.addLayer(child2);
obj1 = new MapObject();
obj2 = new MapObject();
child1.add(obj1);
child2.add(obj2);

// What happens here?
obj1.setProperty("test", obj2);
// Or here?
root.setProperty("test2", obj2);

I think we should fallback on just returning the id as an integer. The user can do any combination of detaching layers, attaching objects, and setting properties.

We should make a best-effort to locate the correct object by id; then return just the id if we can't find the object. This would only ever happen on detached layers / detached objects. JS is dynamically typed, so the user can check Number.isInteger() when working with it.
When we add a detached layer or object to a map (or tile), we should do something like (python-pseudocode):

for object in added layers:
  for property in object.properties:
    if property.type == ObjectRef:
      # NOTE: Do this after adding the new layers, so that it can find id's inside the newly added layers.
      if object.asset() != asset() or not findObjectById(property.value.toInt()): 
        property.value = 0 # Unset

[Phlosioneer]

I guess we'd have to add something like object.originalAsset() though.
And if object.originalAsset() is null, we should assume the object refs are intentional and try to resolve them.

[bjorn]

I think I only had tile object group in mind for this code, but indeed it also covers detached object groups. Sure we can make a best effort to handle detached hierarchies, and I like the idea of falling back to returning the ObjectRef itself (not the ID, because we need to keep the type of the value intact).

I don't think we should be actively invalidating object references though. As it is, you can also copy/paste them and as long as there is an object with a matching ID, it will refer to it. That behavior is somewhat predictable and I think preferable over the IDs getting set to 0. We just need to make sure it doesn't crash.

[Phlosioneer]

That's reasonable.

What do you mean by returning the ObjectRef, not the ID? The object ref can't be manipulated by JS at all. Perhaps we need an EditableObjectRef class.

[bjorn]

We just need to do something similar to what I did with FilePath in change d13afac.

[Phlosioneer]

Another corner case I forgot about:

• If multiple tiles are selected, we need to disable editing any ObjectRef properties.

[bjorn]

Not sure about this being only a C++11 problem. The code is compiled as C++14 everywhere and compiled fine with both GCC and clang (are you on Visual Studio?). That said, we can't just remove this, since we need to initialize somehow. Let's just have a constructor then.

[Phlosioneer]

https://en.cppreference.com/w/cpp/language/aggregate_initialization

no default member initializers (since C++11, until C++14)

So I guess that includes C++14?

Also, it's supposed to be zero-initialized by the aggregate constructor:

If the number of initializer clauses is less than the number of members or initializer list is completely empty, the remaining members are value-initialized. If a member of a reference type is one of these remaining members, the program is ill-formed.

And on https://en.cppreference.com/w/cpp/language/value_initialization

1) if T is a class type with no default constructor...
2) if T is a class type with a default constructor that is neither user-provided nor deleted (that is, it may be a class with an implicitly-defined or defaulted default constructor), the object is zero-initialized and then it is default-initialized if it has a non-trivial default constructor;
3) if T is an array type...
4) otherwise, the object is zero-initialized.

So we don't need to provide a constructor to ensure id is set to 0.

[Phlosioneer]

Also, no, it didn't compile fine on MINGW. One of the automated tests failed with that error.

[bjorn]

Also, no, it didn't compile fine on MINGW. One of the automated tests failed with that error.

Whoops, I guess I checked the CI builds after they had already run with your fix...

Thanks for pointing out the value-initialization for initializer lists! That does mean that without this explicit initialization, we need to avoid writing ObjectRef() (currently see three occurrences).

[bjorn]

That does mean that without this explicit initialization, we need to avoid writing ObjectRef() (currently see three occurrences).

So, actually according to https://en.cppreference.com/w/cpp/language/value_initialization, the form T() also does value-initialization, so there is no need to change those occurrences.

[bjorn]

• Some objects can't have ObjectRef properties added to them. So addPropertyDialog needs to know whether the current object is allowed to have an ObjectRef property. The soultion is easy: add a boolean objectRefAllowed to its constructor, and create a new function on libtiled's object class called canHaveObjectRefProperty(). This leads to...

Let's try to keep things simple. We can add a property of type 'object' anywhere. It just won't be possible to set it to a valid reference in all contexts. It's not a big deal, but trying to disallowing it is a pain and can even be an inconvenience to the user.

Take the example of the tile properties or the object types editor. You'd say on tiles you can't refer to objects, and in the object types editor you can't either. But in both cases it can make sense to add a property of that type, so that once you instantiate a tile object, it will have already that property and there you can set a valid value on it.

In the end, the ObjectRef is just an integer, like people have already been using them until now, but typed such that it is easier to select which object the number refers to and such that it can be understood and visualized. We don't need the script API or the map reader/writer to be checking if the references are valid (I did insert a check for same-asset in the scripting API, because I thought it may be helpful, but I think it might needlessly break some scripts just because they end up using a detached object to set a value for example).

So, there's probably some places where we need to avoid crashing and we could probably help the user realize he can't actually set a valid value in some contexts by disabling the edit widget, but I don't see big design challenges there.

[Phlosioneer]

We still need to jump through some hoops though:

• On those places, we still need to know to prevent editing. Particularly for the Tileset properties, if we just naively look at static_cast<...>(DocumentManager::instance())->selectedTile, the dialog box will open and we'll end up with strange results.