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

TileData not perfectly aligned to grid will be lost when exporting to .tbin format #1985

Closed
krisives opened this Issue Jul 26, 2018 · 5 comments

Comments

2 participants
@krisives

krisives commented Jul 26, 2018

When saving a map in the .tbin format you have to be very careful to have perfectly aligned objects called TileData otherwise they will be deleted upon export without any warning. This can be irritating if you don't notice they aren't perfectly aligned.

@krisives

This comment has been minimized.

krisives commented Jul 26, 2018

Appears this happens in this code:

if (obj->size().width() != tiles->tileSize.x || obj->size().height() != tiles->tileSize.y ||
obj->position().x() / tiles->tileSize.x != std::floor(obj->position().x() / tiles->tileSize.x) ||
obj->position().y() / tiles->tileSize.y != std::floor(obj->position().y() / tiles->tileSize.y))
continue;

It would be nice if either a.) it produced a warning or b.) saved to the closest aligned tile.

@bjorn

This comment has been minimized.

Owner

bjorn commented Jul 26, 2018

Yeah, the manual makes an explicit warning about this, but I agree it is not ideal and I fully support your suggestions for improvement.

A warning could be shown using a modal QMessageBox, as is already done by some other plugins. Maybe a warning like "%n objects did not align with the tile grid, they have been automatically aligned to the nearest position".

@krisives

This comment has been minimized.

krisives commented Jul 26, 2018

@bjorn I tried to find another plugin using QMessageBox but couldn't find any. Is it safe to call from within the plugin code or do I need to get a reference to some other GUI object? Sorry, I'm not a Qt developer by trade.

@krisives

This comment has been minimized.

krisives commented Jul 27, 2018

I checked out how errorString() and various plugins use mError to propagate the error up to the user with QMessageBox and was able to modify the TbinMapFormat to show an error when TileData objects were automatically aligned. However, this stops the file from being marked as saved and tells the user an error has occurred, when in reality the file was saved and it's a warning.

I tried modifying the code to have a warningString() virtual method, but there are too many layers of abstraction to find and update since this behavior is from the FileFormat object which is used by a lot of pieces of the codebase. Waiting a few minutes for make to fail over and over isn't fun either, so I think I will give up on this and just use my hacked version for the time being since it solves my problem.

Feel free to close this issue.

@bjorn

This comment has been minimized.

Owner

bjorn commented Aug 30, 2018

Sorry, it seems you're right that no QMessageBox was actually used yet by any plugins. It should be safe to call from plugin code though. And indeed raising an error instead is not an option in this case.

When adding a warningString() virtual method, you could have provided a default implementation returning an empty string, in which case you'd only have to adjust the tBIN plugin to use it. However, I'm not sure whether this is the best way to raise a warning. I'd probably rather find a convenient way to use the Debug Console for this, probably with something added to the status bar to make warnings more noticeable.

I'll keep this issue open since I'd still like to improve the behavior at least a bit before the Tiled 1.2 release.

@bjorn bjorn added this to Tiled 1.2 (next feature release) in Roadmap Aug 30, 2018

@bjorn bjorn closed this in e1888bf Sep 2, 2018

Roadmap automation moved this from Tiled 1.2 (next feature release) to Recently Completed Sep 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment