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

accessing tileset firstgid property from python missing? #2191

Closed
sverx opened this issue Sep 4, 2019 · 15 comments

Comments

@sverx
Copy link

@sverx sverx commented Sep 4, 2019

In a python exporter I'm writing, I'd like to access the tileset's firstgid property but I can't find any way to do that. Is that missing?
(the point is that tileLayer.cellAt(x, y).tile().id() returns the id of the tile, not his gid, so if I'm using more than one tileset I can't really tell tiles apart...)
Any suggestion?

@bjorn

This comment has been minimized.

Copy link
Owner

@bjorn bjorn commented Sep 4, 2019

Tilesets don't inherently have a firstgid property. They are assigned and used by the JSON and TMX formats to map references to possibly multiple tilesets into a single range of numbers. But any other system could be used, like for example storing each tile as a tileset index + tile id pair.

If you want to use the same mechanism in your Python plugin, the assigning of firstgids is quite straight-forward since it starts with 1 for the first tileset and then goes up by the value of the highest local tile ID + 1 for each tileset.

For figuring out the highest local tile ID + 1, you could rely on the tileCount() function, but I just realized that this doesn't cover image collection tilesets, which can have gaps in the range of local tile IDs when tiles have been removed from the collection. If you need to handle that case, you'd need to increment the ID passed to findTile(id) until it has returned non-null tileCount() times.

@bjorn bjorn closed this Sep 4, 2019
@sverx

This comment has been minimized.

Copy link
Author

@sverx sverx commented Sep 9, 2019

I see no way to get the tileset index directly from a tileset.
Also, using a for in tilesetCount range, I can't check if the tilesetAt(t) is the expected one, that does never become true:

for t in range(tileMap.tilesetCount()):
    if tileMap.tilesetAt(t).data() is tileLayer.cellAt(x, y).tileset():
     # this is never true

any hint?

@bjorn

This comment has been minimized.

Copy link
Owner

@bjorn bjorn commented Sep 9, 2019

@sverx I can't really explain this, apart from that they are apparently not the same Python objects while still holding the same Tileset* value. Possibly something related to how the generated bindings work. I'm not sure if there is another way to compare them, but I would suggest comparing by tileset name instead.

@sverx

This comment has been minimized.

Copy link
Author

@sverx sverx commented Sep 10, 2019

using:
if tileMap.tilesetAt(t).data().name() == tileLayer.cellAt(x, y).tileset().name():
works.

I'd say I can live with it, even if probably having an id() method in tileset class would be better (hint...)

Thanks!

@bjorn

This comment has been minimized.

Copy link
Owner

@bjorn bjorn commented Sep 10, 2019

I'd say I can live with it, even if probably having an id() method in tileset class would be better (hint...)

Right, tilesets don't currently have an ID. I have been considering to add globally unique IDs to each asset, but actually the file name should already be uniquely identifying the asset. The only problem with file names is that relative paths can break when you move files around and having globally unique IDs would provide a way to fix up these references, at least within one project. One problem with globally unique IDs is of course that if you copy a file you end up with two files that have the same "unique" IDs.

In any case, ideally, the direct Tileset* comparison should have just worked here... I'm not sure why it doesn't, but if it's related to the way the bindings work then maybe switching to a different mechanism would resolve that issue (see #2190).

@sverx

This comment has been minimized.

Copy link
Author

@sverx sverx commented Sep 10, 2019

I actually think some method (index()?) that would return 't' from a tileset so that
tileMap.tilesetAt(t).data()
would point to the tileset itself would be sufficient...

@bjorn

This comment has been minimized.

Copy link
Owner

@bjorn bjorn commented Sep 10, 2019

@sverx The tileset could be used by multiple maps, and it could have a different index in each map. But, I could imagine to add a function to the map to get the index of the tileset, so tileMap.indexOfTileset(tileset).

@sverx

This comment has been minimized.

Copy link
Author

@sverx sverx commented Sep 10, 2019

... so that you'll get the index that the specified tileset has in the map, which means you'll then be able to scan all the tilesets that are 'before' this one to calculate the needed tile's GID adding up the previous tileset's tileCount() - if I got that correctly.
I would say it should work!

@bjorn

This comment has been minimized.

Copy link
Owner

@bjorn bjorn commented Sep 10, 2019

Hrm, I just noticed that indexOfTileset is already there:

cls_map.add_method('indexOfTileset', 'int',
[param('const SharedTileset &','tileset')])

However, it takes a SharedTileset and not a Tileset*, so it can't be used with the tileset returned from the tile unless we expose Tileset::sharedPointer() in the Python API.

bjorn added a commit that referenced this issue Sep 11, 2019
To get the shared pointer from a Tileset* value, since this is needed to
pass it as an arguement to Map.indexOfTileset().

Also added trimming of trailing whitespace, which was causing empty
lines in the Issues view (affecting Python script output).

See issue #2191
@bjorn

This comment has been minimized.

Copy link
Owner

@bjorn bjorn commented Sep 11, 2019

The above commit adds the needed function, which should enable you to get the index of the tileset. It will be available in the next development snapshot (or if you compile Tiled yourself from master of course).

... so that you'll get the index that the specified tileset has in the map, which means you'll then be able to scan all the tilesets that are 'before' this one to calculate the needed tile's GID adding up the previous tileset's tileCount() - if I got that correctly.

Actually it's a bit easier, the steps I imagined were:

  • Iterate all the tilesets in the map, calculating for each of them the "firstgid" and storing this in an array (my previous note about finding the highest local tile ID applies here, unless you don't need to support collections, then tileCount is fine).
  • Iterate the cells of a tile layer, and for each tile, use tileMap.indexOfTileset to get the index of its tileset (passing the returned value from tileset.sharedPointer()).
  • Use that index to look up the "firstgid" from the array, which you can then add to the local tile ID to get the global ID.

Hope that helps!

@sverx

This comment has been minimized.

Copy link
Author

@sverx sverx commented Sep 11, 2019

Got it! I'll try it when the next development snapshot will be out.
Thanks!

@sverx

This comment has been minimized.

Copy link
Author

@sverx sverx commented Oct 16, 2019

OK I tried with latest development snapshot to use the new sharedPointer() instead of comparing the names of the tilesets.

# if tileMap.tilesetAt(t).data().name() == tileLayer.cellAt(x, y).tileset().name():
if tileMap.tilesetAt(t).data().sharedPointer() is tileLayer.cellAt(x, y).tileset().sharedPointer():

now it won't work, even using == instead of is

I'll try a different approach, as you suggested, but I'm quite puzzled about why the above wouldn't work.

@sverx

This comment has been minimized.

Copy link
Author

@sverx sverx commented Oct 16, 2019

this seems to work, instead:

for t in range(tileMap.indexOfTileset(tileLayer.cellAt(x, y).tileset().sharedPointer())):  
    tile_id+=tileMap.tilesetAt(t).data().tileCount()
@bjorn

This comment has been minimized.

Copy link
Owner

@bjorn bjorn commented Oct 16, 2019

I'll try a different approach, as you suggested, but I'm quite puzzled about why the above wouldn't work.

In one situation, the pointer comparison happens on the C++ side, where raw tileset pointers are compared and it will say whether they are the same tileset instance. In your situation, the comparison happens by Python and who knows what its comparison boils down to. At least we know, that it's comparing something else than tileset pointers. The Python API exposed for Tiled plugins may be creating different Python objects to wrap around the same tileset pointers, which could throw off that comparison.

Your last snippet should indeed work in general, but doing that for each tile it should be faster to set up a small array of "first global tile ID" first, then you could trivially add it for a given tileset index instead of having to do a for loop.

@sverx

This comment has been minimized.

Copy link
Author

@sverx sverx commented Oct 16, 2019

I see, so we really shouldn't trust pointer comparisons.

As for your hint, that's surely a possibile optimization. As in my current scenario I have only two tilesets and tiles in the second one are very rarely used, I think I don't even need that :)

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.