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

Template instances can't override the flipped states into the gid of the template #1895

Closed
ghost opened this issue Feb 26, 2018 · 6 comments
Closed
Labels
bug Broken behavior.

Comments

@ghost
Copy link

ghost commented Feb 26, 2018

When using templates of tile object, the flipped states can only be set in the template and cannot be changed in the object.

You can reproduce it by creating a template and trying to modify a instance's flipped states.

I`m not sure of a way to solve this, maybe keeping a gid attribute with only the flipped states in the object, but when reading you'll need to pay attention and use the tile from the template and not the nonexistent tile 0 (zero) from the object. (which is fine for me)

@bjorn
Copy link
Member

bjorn commented Mar 2, 2018

You can reproduce it by creating a template and trying to modify a instance's flipped states.

This is not the full story, right? I tried to reproduce this but it seemed to work fine. When I tried saving and loading the map though, I noticed the flipped state was not getting saved.

This is probably because an override flag is not getting set. That flag does affect the tile + flipped states though, so when you flip the tile of a template object, it will also override the tile.

I`m not sure of a way to solve this, maybe keeping a gid attribute with only the flipped states in the object, but when reading you'll need to pay attention and use the tile from the template and not the nonexistent tile 0 (zero) from the object. (which is fine for me)

Unless you really need to override only the flipping bits, I would suggest not making this more complicated than necessary for now. I do agree it's rather unintuitive, so we could consider changing this... the approach you're suggesting doesn't sound bad.

@bjorn
Copy link
Member

bjorn commented Mar 2, 2018

Hmm, after fixing the setting of the changed flag, it still didn't work (would write out gid="0" to the TMX file). That is because the tileset used by the template is not necessarily part of the map, and as such it has no global ID assigned.

In my case the tileset was part of the map as well though, which probably indicates another bug...

@bjorn
Copy link
Member

bjorn commented Mar 2, 2018

In my case the tileset was part of the map as well though, which probably indicates another bug...

Ok, that is because tilesets that are part of the map do not get added to the tileset manager until after the map is loaded, so once it encounters the same tileset while loading a template it will not find it there and it will load it again. This is getting rather tricky... probably the reference counting of tilesets should be moved from MapDocument to the Map class.

bjorn added a commit that referenced this issue Mar 2, 2018
FIXME: The flag should not be set when ChangeMapObjectCells is used in
AdjustTileIndexes.

Issue #1895
@ghost
Copy link
Author

ghost commented Mar 2, 2018

This is not the full story, right? I tried to reproduce this but it seemed to work fine. When I tried saving and loading the map though, I noticed the flipped state was not getting saved.

Yes, sorry for not being totally clear here, indeed it does not get saved and loses it states when closing and loading the map again.

Hmm, after fixing the setting of the changed flag, it still didn't work (would write out gid="0" to the TMX file). That is because the tileset used by the template is not necessarily part of the map, and as such it has no global ID assigned.

It's a bad thing the gid="0"? If keeping only the flipped states in the object's gid, the global ID isn't really needed.

@ghost
Copy link
Author

ghost commented Mar 2, 2018

It's a bad thing the gid="0"? If keeping only the flipped states in the object's gid, the global ID isn't really needed.

Oh, I see, it will always override the flipped states with false values.

It really is tricky, maybe omitting the gid attribute when there's no change in the flipped states. And when there's the gid, get it and override the states bits.

@bjorn bjorn added the bug Broken behavior. label Mar 4, 2018
bjorn added a commit that referenced this issue Mar 4, 2018
Related to issue #1895. However, it does not fix the whole problem,
since in fact a "gid" can't be written for template objects that use a
tile from a tileset that isn't also part of the map.

In addition, there is a caching issue causing tilesets used by both
templates and the map to get loaded twice. This is why it currently
won't even work for loaded maps, even if the tileset used by the
template is part of the map as well.
bjorn added a commit that referenced this issue Mar 4, 2018
Rely on the QSharedPointer for the lifetime and add/remove tilesets
instances from the TilesetManager directly from the Tileset class.

This fixes issues with tilesets getting loaded twice when they are used
by both a map and a template object, which happened as a result of the
manual adding of references being too late.

In general this approach should also be more reliable and prevent double
loading of tilesets in some cases.

Helps with issue #1895 when the tileset used by the template is also
part of the map.
@bjorn
Copy link
Member

bjorn commented Mar 4, 2018

I've made some changes (see above) that will be included with Tiled 1.1.3 which fix this issue in case the tileset used by the template is also part of the map. Though the whole gid will be overridden in this case, not just the flipping flags.

I think changing the behavior such that you could override only the flipping bits is outside the scope of a patch release, but I'll leave this issue open since we could consider this for Tiled 1.2.

@ghost ghost closed this as not planned Won't fix, can't repro, duplicate, stale Aug 2, 2023
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Broken behavior.
Projects
None yet
Development

No branches or pull requests

1 participant