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

Add tileset tile x/y draw offset. #3488

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

Chase-san
Copy link

@Chase-san Chase-san commented Oct 5, 2022

This is a modern(ish) version of #1323. I just want to do the minimum needed to get this merged in, whatever that minimum might be. I want to avoid making the PR overly complex, so it is very bare bones at the moment.

This functionality, or something like it is important for a game I am working on.

@Chase-san
Copy link
Author

Chase-san commented Oct 5, 2022

The main issue I can see with this is the lack of tooltip, translations and some way to know if a block has been offset in some way inside the tileset display.

Additionally, not all map formats will support this new functionality, but if you know of any that do, I can update the exporters as well.

@Chase-san
Copy link
Author

Would it be better to use the "tilesetTileOffsetChanged" rather than the "tileImageSourceChanged", neither are perfect of course, but I am not sure if should be adding new signals.

@Chase-san
Copy link
Author

@bjorn Is there anything else needed for this?

@bjorn
Copy link
Member

bjorn commented Nov 3, 2022

@Chase-san I appreciate that you're picking up this change again! I will have a look at this tomorrow, but at a quick glance I could not see much difference from the changes at #1323 so it'd probably be fair to look into merging that PR instead. Is there anything in this PR that the older one didn't cover?

@Chase-san
Copy link
Author

Chase-san commented Nov 3, 2022

@bjorn This one is up to date with the codebase and provides a more robust map save/load functionality.

It differs slightly in how it applies the offset.

If you would like more to be changed for this functionality to be better integrated, please let me know.

@bjorn bjorn linked an issue Nov 4, 2022 that may be closed by this pull request
I think this should be more intuitive, especially when we later make this
property visually editable.
@Chase-san
Copy link
Author

Chase-san commented Nov 4, 2022

The enums are not saved or encoded into a file at any point right? Seen that be an issue before, which is why I put the new values at the end.

If a tile only had its origin customized, it would still fail to get
written out.
@bjorn
Copy link
Member

bjorn commented Nov 4, 2022

The enums are not saved or encoded into a file at any point right? Seen that be an issue before, which is why I put the new values at the end.

Right, these enums are only used within the application. :-)

What remains to be fixed before this can be merged, is fixing the rendering artifacts that can appear due to the tile not occupying the area expected by for example the brush preview. We may also need a more specific change signal than tileImageSourceChanged.

@Chase-san
Copy link
Author

Do you have an example of the artifacts?

@bjorn
Copy link
Member

bjorn commented Nov 11, 2022

@Chase-san Sorry, I ran out of time and today again I ran out of time, but regarding the artifacts, the steps I can reproduce it with are:

  • Take one of the example maps, for example desert.tmx
  • Open its tileset
  • Change the origin of one of the tiles such that the tile shifts to the top-left (say, set origin to 10,10)
  • Go back to the map
  • Select the tile with the changed origin
  • Hover the map with the brush preview
  • Notice that the preview leaves artifacts of the tile when it changes position

There is already code to avoid such artifacts, which takes into account the tile offset set on a tileset. This will need to be adjusted, so that also individual origins on tiles are taken into account.

@Chase-san
Copy link
Author

Sorry I've been pretty busy with other things, I'll take a look at this this weekend.

@bjorn
Copy link
Member

bjorn commented Feb 23, 2023

@Chase-san No worries, I'm currently looking into fixing this up.

The tile origin changes where the tiles are rendered, so it needs to be
taken into account when calculating the draw margins. These are used
when determining which area to repaint, and which tiles to render in a
certain area.

The draw margins are cached per map and now also per tileset. They are
invalidated when the "tile offset" or a tileset is changed as well as
when the "origin" of a tile is changed.
@bjorn
Copy link
Member

bjorn commented Feb 23, 2023

Still to do:

  • Support writing and reading tile origin for the JSON format (maptovariantconverter.cpp and varianttomapconverter.cpp)
  • Support writing out tile origin in Lua export (luaplugin.cpp)
  • Support tile origin in GameMaker 2 export (yyplugin.cpp)

@bjorn
Copy link
Member

bjorn commented Feb 24, 2023

I've decided to de-schedule this change from Tiled 1.10 since it will need more time. As an individual "drawing offset" setting it wasn't very problematic, since we already supported that on the tileset level. However, I've changed this property to "origin", which I thought was just a negation and simple rename, but it actually affects the interaction with alignment for both tile objects and tiles placed on a tile layer, which are not behaving intuitive in its current state due to the default bottom-alignment in both cases.

This issue remains high priority, but we need to take the time to do it right, preferably simplifying things rather than making things more complicated.

@bjorn bjorn removed this from the Tiled 1.10 milestone Feb 24, 2023
@Favouris
Copy link

Favouris commented Nov 8, 2023

How usable is this commit at this juncture? Per-tile offset / origin is important to me too so if the progress of this feature is on hold, I'll be happy just using something minimal instead.

@bjorn
Copy link
Member

bjorn commented Nov 11, 2023

How usable is this commit at this juncture? Per-tile offset / origin is important to me too so if the progress of this feature is on hold, I'll be happy just using something minimal instead.

Unfortunately I had to take a rather long (over 2 months) break from Tiled development recently, and it's a bit hard to tell for me which state this was in. There were some issues raised (see my previous comment), but it's been a while and I need to get back into it, play around with the current state and then decide what way to go forward.

@Favouris
Copy link

From what I could grok, there seems to be an alignment issue with previews? Maybe it would help if we could have a list of things to be fixed (and where to find them in the code) for anyone unfamiliar with the codebase who wants to jump in and help with the PR?

@Chase-san
Copy link
Author

From what I could grok, there seems to be an alignment issue with previews? Maybe it would help if we could have a list of things to be fixed (and where to find them in the code) for anyone unfamiliar with the codebase who wants to jump in and help with the PR?

While that may sound reasonable on the surface. If that information was readily know they would simply fix it.

@Favouris
Copy link

From what I could grok, there seems to be an alignment issue with previews? Maybe it would help if we could have a list of things to be fixed (and where to find them in the code) for anyone unfamiliar with the codebase who wants to jump in and help with the PR?

While that may sound reasonable on the surface. If that information was readily know they would simply fix it.

Very well. That's kind of unfortunate...

On a side note, were you able to use this commit to any success for your game?

@bjorn
Copy link
Member

bjorn commented Nov 16, 2023

@Favouris Since the previous builds had expired, I've merged master which triggered new builds, so you could try out the feature. Just be aware that it's not finished and subject to change, but feedback is welcome.

@RomainMazB
Copy link

This is the missing feature, first asked 9 years ago 😢.

Hope this will we merged soon.

@stoner-jack
Copy link

Thanks for this.

  • when exporting, the origin properties are lost in tsj.
  • the properties are reserved in lua and tsx

Will keep the notes here as I test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Draw offset for individual tiles
5 participants