-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Can't edit external tile sets #242
Comments
This is a technical limitation since Tiled only knows how to keep track of a single document (file) per map. Basically it is currently not possible for the undo system to cope with changes made to external tilesets since they are shared between multiple maps, so the UI stops you from changing them. It is however possible to temporarily import the tileset, make any changes to tile properties, and then export it again. The forced saving of the file on export makes sure that no changes go lost. This way of doing things isn't ideal, but is still much more comfortable than re-creating the tileset. |
Has any progress been made on this since the last comment? I've just encountered it now, and while there is a workaround, it's a little bit frustrating. |
Nope, sorry. |
(from #1213) I agree that adding separate undo/redo buttons for the tilesets panel would be a bad solution, it's essentially trading a UI problem for a different one. As far as I understand, the core of the issue here is a discrepancy between the user's mental model of undoing ("it undoes the last thing I did in this tab") and Qt/Tiled's implementation ("it undoes the last thing the user did in a specific file"). This works fine when it's clear to Tiled which file the user is editing (by having a 1-1 mapping between files and tabs) but this falls apart when the same tab can have multiple files open (eg. map and tilesets). From here I can see two options:
Managing multiple undo stacks in one tabBasics
ExampleHere is a simple example (with one tab):
When users press the redo button, we redo in the undo stack which offers an action stored with the lowest action number. When performing an action after a few undos, all the undo stacks for files in the tab are truncated from their current level to the top. Dealing with concurrent editsSince an external tileset can be edited in different tabs concurrently, we need a way to make sure things behave consistently when trying to undo changes across tabs. My suggestion here is that when an action is performed on a tileset from a tab which is different from the one that currently has actions in the tileset's undo stack, that tileset's undo stack is wiped. This is why I suggested storing which tabs actions originate from. As a result, the only scenario where a undo stack gets wiped is the following:
Note that changes in tab M1 to the map or other tilesets can still be undone/redone. NotesOverall, I think the advantages of this solution are:
The only inconvenient I see is that it is non-trivial work to implement this feature on top of Qt's undo/redo system but it doesn't sound as bad as revamping the whole UI. It is also much less risky and self-contained. As a side note, this could also be implemented by adding custom undo stacks to tabs (with which Ctrl+Z/Ctrl+Y would interface), rather than polluting the existing actions with extra data. I feel like the idea was easier to describe with extra data in the undo actions but it's probably not the cleanest way to implement it. It's definitely tricky to explain so let me know if part of this sounds confusing! |
My idea was to move any editing operations to the full screen tileset view and make the Tilesets view essentially read-only (providing only an "Edit" button, which would open the tileset in a document tab). For embedded tilesets, this would work exactly the same, except that changes made to the tileset would end up on the undo stack of the affected map document. This may be a little confusing as well, but I'm not comfortable with just throwing away undo history when a user edits an external tileset also from another tab. Also, I think it will be quite hacky trying to layer your suggestion on top of the QUndoCommand/QUndoStack/QUndoGround framework. Though there isn't that much code involved in that, anyway. I'd prefer to drop support for embedded tilesets completely, since that would simplify things a lot. I will at least try to make using external tilesets the default way of doing things, and have embedded tilesets as the special case. Embedded tilesets seem nice when you start off, but once you define any additional tileset properties you quickly run into the issue that it's hard to share that information among multiple maps. I think your idea is quite clever to avoid any conflicts, but I'm skeptical whether it is really completely natural to the user. I'd prefer to clearly distinguish in the UI between different files, and associate an undo stack with each file. Removing tiles from collectionsOne thing I'm still troubled with is dealing with changes that affect both the tileset and the maps that use it. I think there is really only one such change though, and it's the removal of tiles from image collection tilesets. When you remove a tile from a tileset, currently Tiled will also remove any usages of this tile from the map. And this is combined into a single undo macro. When the tileset is edited as a separate file, the map could still be adjusted but the macro approach is no longer possible since they have different undo stacks. So the user may then undo the map change and run into the problem that the map is referencing the removed tiles. Another way to get into this situation is to undo the addition of tiles to the tileset, after you've already started using them on the map. I think really the only solution here is to stop referring to tile instances directly using pointers, but rather refer to them by their ID. This avoids Tiled from crashing and would allow it to display special graphics or warnings when the tile layers refers to no longer existing tiles. |
Wouldn't it be confusing in scenarios such as:
I didn't know dropping support for embedded tilesets was something you would consider as it seems like a big breaking change. If that is acceptable, then I am absolutely positive that presenting tilesets as standalone documents (like you suggested) is by far the best option of all. All I can see in defense of embedded tilesets as a feature is:
On the plus side, people's workflows might break but engines which consume maps don't have to, because embedding the tileset data in a map export should still be possible.
I agree with your solution here too. Does sound like a lot of work though. |
Hello guys. Sorry to interrupt your discussion flow, but I wanted to maybe point you both on another direction. |
I don't think projects are a different direction, they're more of a step further. If/when tilesets become first-class citizens (which is what this ticket is about), it could make sense to support "project" files which would store custom object types, references to maps files and references to tileset files. Much like a Visual Studio solution is a collection of code files (stored separately) and settings. ^ Note that my 2 cents aren't worth too much either as I haven't written a single line of Tiled's code (yet?). |
Right, I anticipated this would be a little weird and now that you point this out as well I'm convinced that it would probably be better for tilesets to have their own undo stack, even when embedded. At the same time, the "unsaved changes" state of the MapDocument should probably be based on the clean state of the undo stack of any embedded tilesets, in addition to its own undo stack. And of course saving a map would set all these undo stacks to the "clean" state.
Right, for now I don't think I can entirely remove this feature or even move it to an export-only option. But what I can do is rearrange the UI to move the embedded tilesets feature to the background.
While related, introducing projects does not magically solve the problem of keeping track of changes made to external tilesets separately from a map. It would definitely be the next thing on the radar though. |
After spending two evenings and one full day on reworking the UI to support editing maps and tilesets alongside each other, I've determined that it is a lot more work than I had anticipated. A large amount of things are no longer functional and marked with "todo" entries in the code. Before I continue, I would like to have a better worked out plan of where this is going to be sure it's the way to go. Here's a screenshot of my current Tiled after creating a new tileset: The nice thing is, we could add a tileset-specific tool bar that would have most actions currently below the Tilesets view, and which also allows switching to different modes like 'terrain edit' mode. I'm not sure yet how the collision editing and tile animation editing would best fit into this though. Ideally I'd like to integrate those as well. Overall though, I have to be careful not to make the current workflow needlessly more complicated. One problem I see is the menu bar, which contains largely map-specific actions. Many of them could actually be removed since they are already available through other means, or moved onto a tool bar. It seems most modern applications are doing away with menu bars, but I think they do have their place. But that place is definitely not below the tab bar. Here's a screenshot with a map selected: I've pushed this work to the Any feedback is welcome. |
I've tried this embedded main window on OS X and Windows and didn't like how it looks. That's why I've attempted to implement this using mode switching on a single main window. However, when I got it to sort-of work I already noticed that it wasn't going to work either, mainly because we really can't have the tab bar potentially shift about as a result of switching documents. So I'm back at the embedded main windows and will just try making it look decent. I've already made the File Open action work on both maps and tilesets and will continue solving other problems, including things you suggested. I had actually written a response here earlier while booted into Windows, but apparently it got lost. In any case I basically said that the introduction of "Tileset" and "Command" menus make total sense. But I would not move the actions to add new layers out of the Layer menu, because I think people expect them where they are. I really like the amount of thought you're putting into this, which definitely helps me through this change. Thanks! |
Thanks for the update! I skimmed through the live-coding session to see the two modes in action. I'm really curious to see what the embedded main window setup looks like on Windows (and OS X). Hopefully it's possible to make it look decent, I much prefer it over the single main window. A few notes, mostly trying to explain the reasoning behind some of my earlier suggestions or elaborate on them:
I think that is true at the moment but probably will change after the introduction of a Tileset menu. If the Layer menu had "Add layer" in it, you'd expect the Tileset menu to have an "Add tileset" entry. However, the Tileset menu is all about editing a tileset so we don't want to put "Add tileset" there. In the livestream, I noticed you had the main toolbar between the menus and the tabs. I don't know if that's how you intend to keep it but I think there's a good case to be made for its removal (or hiding by default):
Keep up the good work! |
If you're on Windows, you can look for the latest installer based on the
It sounds reasonable, but consistency with other applications also matters. In GIMP for example, you add layers from the Layer menu, and not from the Image menu. Hence I'm not even entirely convinced that the 'New Tileset' action shouldn't be in the 'Tileset' menu, although your reasoning makes total sense.
Of course I didn't intend to keep it that way and in the followup screencast of today (starting at around 58:30) you can see how I've built a new |
Ah, not easily I think. Besides, that same border sits below the entire tool bar and removing it would look strange above the map view I think. The main thing that bothered me was the gray background on the tab bar, which I think would be better if it blended in with the menu bar. I see that you've got the still shared "History" view open. What do you think about that? Should I instead make two instances of this, one for each embedded main window? On the upside it would allow it to be placed more space-efficiently, but on the downside it would not always be in the same place depending on how you placed it in the different main windows. The same problem exists for the Maps dock. When I instantiate two of those I'd even need to make them synchronize to the same folder. And the Debug Console, for which it really would be nice to avoid multiple instances. |
I agree it would look better but I barely noticed it, other tabs in Tiled (see Terrains/Tilesets or Mini-map/Objects in my screenshot) have similar behavior. I wonder what Qt maintainers think about this, assuming other applications are affected too.
I think would prefer to have an instance of the History panel inside each main window. Having it sit "inside" the tab bar makes it more obvious that it's specific to the file in this tab. Your concerns about the layout being inconsistent different main windows could be alleviated by the layout saving thing I mentioned at the end of my comment which had the long table of suggestions.
When switching from a map to another map, or a tileset to another tileset, layout would stay exactly the same (ie. changes "carry over" to other tabs). When switching between map and tileset, the layout would snap back to the preferred setup for that usage. As for the Maps and Debug Console panels, these aren't connected in any meaningful way to the content of the tab so I would keep them outside of the main windows. Especially considering the "Maps" panel may become some sort of "Project" tab one day. They would also not be part of this dual-Layout-saving feature I mentioned above. |
(from Development Update) I would expect the workflow to be:
I don't think that's an unreasonable number of user inputs for what it does. Things could be a little more user friendly if "Add external tileset" could let users choose from either a file browsing dialog box or a list containing currently open tilesets and tilesets in use by currently open maps. |
Today I've merged the This essentially resolves this issue, but since it's such a huge change I'll leave it open for a bit and am eager to hear your feedback! I'll also have to go over @agersant's suggestions once more to see if there are any changes that still should be done (like putting the History view inside each editor, rather than at the top level). |
Fantastic news! I am very very happy to see this feature landing in |
After going over the feedback once more, I've just made a few remaining actions to the Tilesets menu and I'll close this issue. Changes that could still be made, but are not technically part of this issue:
|
This may be more of a workflow issue (so, documentation), than a bug. But once I export a tile set, I can't set properties on the tiles any longer. That means I have to do it manually, or I have to re-create my whole map over again! (Because I have to delete the tile set and re-create it, but if I do that, all the tiles get removed).
In any case, I am trying to develop a workflow that lets me change the tile properties without destroying the maps.
The text was updated successfully, but these errors were encountered: