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

Tilesize by Layer #1422

Closed
wants to merge 31 commits into from
Closed

Tilesize by Layer #1422

wants to merge 31 commits into from

Conversation

Lucrecious
Copy link

This pull request is give layers ownership of their own tile sizes instead of having the map own the global tile size. The idea is to allow the user to have multiple tile sizes for their map.

Build

No change, follow instructions on main repo.

How it works:

There are four new layer properties (regardless of layer type):

  • Tile Height and Tile Width (can be edited)
  • Height and Width (cannot be edited)

The height and width properties are given in tiles, not pixels. This is only for convenience so the user can see how many tiles the map is given a tile size.

Editing a layer's tile size changes the grid both visually and storage wise. The width and height of the grid in pixels does not change. The only way to change the size of your map is by resizing the map, or changing the tile sizes through the map properties.

For Tile layer types this is useful because you can use tiles of any size per layer.
For any other layer type, this seems to just be a visually aid (and snapping I believe).

When adding layers into the layer dock, the default tile sizes mirror the map tile sizes.

Approach

This section will talk about the approach I decided to take, things that I would like to change and my reasoning. It's also here for others to understand what I did more easily.

Layers

Layer objects now require a tile width and tile height in their constructors, there is no default layer tile size

Maps

Map objects now have an active layer, which mirrors the active layer on the MapDocument. This is kind of sketchy because Map objects having an active layer doesn't really make sense class wise, so I'd like to find a better solution.

As a result of the former, map objects have overloaded width, height, tile width and tile height functions. The reason for this is because the map's width and height in tiles depends on its "focused" layer.

width(bool b) for example, returns the "focused" width of the map if b is true. If the map has a layer in "focus", then it returns the width of the layer. Otherwise, this function returns the width of the map. If b is false, this function only returns the width of the map.
There are similar functions for height, tileHeight and tileWidth.

In the future I would like to completely get rid of the the overloaded function and have the map just own it's own width, height and tile sizes. The problem right now is that grid sizes, brushes and (basically) everything relies on the map object in some way, so to switch from map-based to layer-based requires a much bigger of a revamp than I had originally thought. The good thing about having the map object own an active layer is that it's very easy to allow map dependant classes to use a map's focused sizes, which take into account the active layer's sizes.

In other words, this solution is more on the lazy side because it required less work to add in active layer functionality to a map than to restructure an entire infrastructure.

I'm not totally sure if this is a good approach, but it was quick(ish) and didn't require a major restructure.

Known bugs:

  • Selection between layers with different tile sizes is undefined
  • Tiles may disappear when tile sizes change, a workaround is to move the layer up once and down once)
  • Undo/Redo is messed up on layers with different tile sizes

Things to do

  • Fix up broken plugins
  • Fix up variant map converter
  • Fix up python bindings
  • Clean up

User Notes

This section is for users of this branch.

Integration with .tmx readers

All current .tmx readers don't recognize that layers have tile width or tile height now, but since all readers read custom properties I temporarily create two custom properties before a save: a tilewidth and a tileheight. With any .tmx readers you can access a layer's custom properties and access the tile width and height by querying for tilewidth or tileheight.

Automapping

In case you're wondering, for automapping, all layer tile width, tile height, width and height must match that of the map's, otherwise an error will pop up on an automap (telling you exactly that).

End Remarks

Hopefully I'm not missing anything.

I'll be working on this periodically, but keep in mind that the functionality I was looking for already has been implemented. As a result, the updates on this will be a little slow. I think for many users, what I have now with this branch should be enough. Although, I'd be careful since things are still pretty buggy.

Thanks!

Confirm that read/write of tile sizes work
Undo/Redo unimplemented
Width and height of grid do not change
@bjorn
Copy link
Member

bjorn commented Jan 3, 2017

First of all thanks for getting this started! As you say, the change is already enough for your purposes, but I think there is a lot of work remaining before this can be merged. I'll try to help with that.

There are a few things in the implementation that I consider problematic:

  • Keeping track of focused layer in the Map class doesn't fit the design and duplicates functionality in MapDocument. You've already realized this, and we'll just need to find a less lazy solution.

  • tileWidth and tileHeight should only be used for TileLayer, not for the other layer types. For object snapping, it should be fine to use the map grid instead (until a more flexible grid system is added).

  • Why was the static MapRenderer::MapSize introduced?

  • Resizing the layer because of changes to its tile size. This can easily lead to a large amount of memory allocation when tile size is changed repeatedly, and it may simply not be what the user wants. I think we'll need to have a "Resize Layer" dialog, which works similar to the already available "Resize Map" dialog (though it will require changes as well).

  • We can't have tilewidth and tileheight be written out as custom properties. Readers will just need to be updated.

  • Undo is broken for layers that are synced because of changing map tile size (data loss). As when changing layer tile size, I do not think this operation should implicitly resize layers.

In addition there are of course the known bugs and things to do that you've already mentioned. I think this will be a long project, but it's great to finally start to tackle all these issues.

I do have some reservations about the design in general. In some way, it would seam more reasonable to me to introduce the concept of a "scene", which could contain multiple tile maps, each with their own tile size. At the same time, a tile map would only consist of tile layers and not also contain objects. Those would then go into the scene directly. Such a change of course represents a big departure from the current structure and is hence probably something for a "Tiled 2" kind of project. In the meantime, we can see if this change can be finished while keeping everything working and intuitive.

@Lucrecious
Copy link
Author

Hey Bjorn.

I've been busy with my actual project, and I don't know when I'll get to doing these things, for now I'm leaving it as it is, and whenever I get bored of my current project, I'll come back and update this.

Basically, I agree with 100% of what you're saying. I hope you acknowledge that a lot of what I did was only to get something working for my own project, and I wasn't really expecting to actually push it to master haha

The only reason I have a temporary custom property tilewidth and tileheight, is just for ease of use for my own project!

The static function for MapRenderer was introduced because I thought creating an entirely new object to call one inexpensive function, was kind of wasteful. Although, in the grand scheme of things it doesn't really matter I guess.

The concept of "scene" is very interesting and easy to wrap your head around (coding wise). I'm willing to take on the challenge, but at a later time :)

Of course, if anyone wants to do the reimplementation of this, I would be happy to see it done better!

I'll be back and thanks for taking the time to look at my additions!

@Lucrecious
Copy link
Author

Lucrecious commented Aug 11, 2017

Hey @bjorn, I might start working on this again soon and I had a few concerns.

You mentioned you had some reserves on the overall design, and you mentioned scenes each with their own maps, and each with their own tile-size.

I still think it's better to have a tile sizes per layer, and not per map. To me it makes more since that a map can have tiles of multiple sizes than over complicating the overall design with scenes (which seems like just a way of organizing "map layers").

I've also noticed that in your newest version of Tiled, you have added groups, so scenes on top of that seems like it would be overkill, don't you think?

Do you have any reserves on how multiple tile sizes should be implemented (per map, per layer)? I want to start from scratch again, and this time with a design we can both agree on.

If you could give me pointers on where to start, that'd be great too.

Edit:
Reviewing my code from months ago, I notice that the only big issue I'm having with the tile sizes are with the renders. The MapRenderers are cool because they're generic enough to use in many places, but a problem with them is that since Maps don't have a concept of a "selected layer", the MapRenderers themselves don't have that concept either. We can't pass in MapDocuments because the MapRenderers don't render MapDocuments and sometimes you render maps that are unattached to MapDocuments.

The main problem I'm having is figuring out what grid size to use when rendering. In all other cases, I have access to the current tile map (for brushes, stamps, etc). The outlier here only being the MapRenderers. Even with the concept of scenes, there's no way of accessing the current focused map.

Here's a suggestion (obviously, if you have any better ideas, I'm all ears), why not make the Map inherit the tile size from the layers? i.e. Selecting a layer changes the tile size of the Map? At the same time, Maps come with a "default" tile size that is used when layers are created. In other words, a Map tile size is dynamic, and doesn't have a concept of a focused layer, it has its own tile size which is changed whenever a layer is selected, added, deleted, etc. This makes some intuitive sense to me.

Let me know what you think!

@bjorn
Copy link
Member

bjorn commented Aug 11, 2017

I still think it's better to have a tile sizes per layer, and not per map. To me it makes more sence that a map can have tiles of multiple sizes than over complicating the overall design with scenes (which seems like just a way of organizing "map layers").

Introducing a scene was not meant to make things more complicated. It is mainly because I think the "map" has way too many attributes (especially tile-layer specific ones) and I'd prefer to have a less encumbered top-level container type. Actually, moving the grid size to the tile layer is a step into that direction. I think it was a bit confusing that I wrote about multiple maps in a scene, because I didn't mean the map as it is now but rather a trimmed down "tile map" version that only contains tile layers sharing the same attributes.

I've also noticed that in your newest version of Tiled, you have added groups, so scenes on top of that seems like it would be overkill, don't you think?

The elements of a scene would definitely still have the same grouping concept. Maybe "level" would be a better term than "scene", btw.

Do you have any reserves on how multiple tile sizes should be implemented (per map, per layer)? I want to start from scratch again, and this time with a design we can both agree on.

I would say the attributes should go the TileLayer, to not change the whole structure too much for now.

If you could give me pointers on where to start, that'd be great too.

Unfortunately I'm totally swamped at least until the GSoC 2017 is over, but after that I should be able to help out.

@Lucrecious
Copy link
Author

Good luck with GSoC 2017! :)

PS: I've also edited my previous post with a suggestion.

@bjorn
Copy link
Member

bjorn commented Aug 12, 2017

Good luck with GSoC 2017! :)

Thanks! The students are doing well so far, but there's actually less than three weeks remaining!

PS: I've also edited my previous post with a suggestion.

I'm afraid I do not quite understand it. You talk about either introducing "current layer" to the Map class or changing the grid size of the map based on the current layer. But why is this needed? When the MapRenderer is asked to render a tile layer, it should just get the grid size from that layer. More difficult will be the rendering of tile selection and tile grid, but in both cases again the caller will need to pass in required information, like a tile layer pointer (or a "QSize gridSize", or a "GridParameters", which could include size and color and in the future possibly other things).

Then, when the MapScene tries to render the grid, it can pass in the current tile layer or parameters based on it. If the current layer is a group layer or object layer, the grid size could fall back to that of the map.

Let me know if I'm missing something!

@Lucrecious
Copy link
Author

Lucrecious commented Aug 12, 2017

I got it! I just misunderstood how the code flowed.

I thought drawTileLayer was called iteratively (i.e. it was called on all tile layers) in which case I wouldn't know which tile layer was selected. Looking more into it, I figured out that's not how it works.

I'll see if I can get something by the end of the month :)

Thanks!

I'll probably start a new pull request, so I'll close this before I start another.

@justgook
Copy link

justgook commented Jul 4, 2019

any chance get it someday or it will be not touched any more ?

@bjorn
Copy link
Member

bjorn commented Jul 4, 2019

TLDR; Maybe some day, but not the way it was done in this PR.

In terms of enabling multiple layers within the same map to have different tile sizes, that is definitely something I'd like to enable. However, it will be very tricky to do this due to the many places in the code that work based on the assumption that tile layers use the same tile size.

Since this pull request was opened, it became even more difficult due to the introduction of multi-layer selection and editing in Tiled 1.2, which is another feature that will get more complicated once each layer can also have its own tile size.

So this is where I think it makes sense to introduce another container type above the map, I called it the "scene" which is a common term for it. Then, if you wanted a collision layer or a background layer with a different tile size from your other layers, it would be a matter of placing multiple tile maps within a single scene. This approach would avoid most of the problems with existing features. It'd still be a big change, but hopefully it would not introduce the same level of complexity.

I'll close this PR now given that it can't be merged.

@bjorn bjorn closed this Jul 4, 2019
@PeachieDude
Copy link

Sad news, even more due the compatibility with unity..
I have both 32x32 and 16x16 tileset and wanted to have both working together (w/o having to scale 16x16) and just discovered that i will need to find another way to..
Tiled is a very amazing editor but..
Maybe, if is there a work around, if not scale, id be glad to know.
I have the 32x32 version of the 16x16 but the 16x looks way better
anyway, maybe one day..
Cheers for the try

@bjorn
Copy link
Member

bjorn commented May 18, 2022

@PeachieDude Possibly workarounds are:

  • placing your 32x32 tiles on a 16x16 grid. In this case you have to be careful with placement because tiles right next to each other will overlap.
  • using a 32x32 grid, but placing your 16x16 tiles on an object layer as tile objects. You can snap them to a 16x16 grid by enabling "Snap to Fine Grid" and setting "Fine grid divisions" in Preferences > Interface to 2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants