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

Fix automap for infinite maps #1692

Merged
merged 8 commits into from Aug 27, 2017

Conversation

Projects
None yet
2 participants
@ketanhwr
Contributor

ketanhwr commented Aug 20, 2017

This patch aims to fix the automap issue in infinite-maps.

@bjorn

See inline comments.

Does this also make infinite maps work for automapping rules?

Show outdated Hide outdated src/tiled/automapper.cpp Outdated
Show outdated Hide outdated src/libtiled/tilelayer.cpp Outdated
@ketanhwr

This comment has been minimized.

Show comment
Hide comment
@ketanhwr

ketanhwr Aug 21, 2017

Contributor

No, it doesn't work if the automap rules are infinite as well, right now.

Contributor

ketanhwr commented Aug 21, 2017

No, it doesn't work if the automap rules are infinite as well, right now.

Show outdated Hide outdated src/tiled/automapper.cpp Outdated
@bjorn

This comment has been minimized.

Show comment
Hide comment
@bjorn

bjorn Aug 24, 2017

Owner

Reminder to self, or when you have time: need to check this change against issue #1224 (since that is where this special code we have now removed was introduced).

Owner

bjorn commented Aug 24, 2017

Reminder to self, or when you have time: need to check this change against issue #1224 (since that is where this special code we have now removed was introduced).

@bjorn

This comment has been minimized.

Show comment
Hide comment
@bjorn

bjorn Aug 24, 2017

Owner

Alright, I checked this against issue #1224 and unfortunately it re-introduces that issue. However, I think the consistency is more important than getting that particular example to work. The example in issue #1224 could be adjusted to have separate input and output regions in order to make the rule work at the border.

However, I found an issue with the automapping in that even after this patch it doesn't work with infinite maps outside of the already used bounds. Please open this example.zip and try to place a rock somewhere. You should see that the first time it works, but then when you paint rocks in many places, it only places the flowers within the chunks that were allocated the first time the automapping was activated.

Owner

bjorn commented Aug 24, 2017

Alright, I checked this against issue #1224 and unfortunately it re-introduces that issue. However, I think the consistency is more important than getting that particular example to work. The example in issue #1224 could be adjusted to have separate input and output regions in order to make the rule work at the border.

However, I found an issue with the automapping in that even after this patch it doesn't work with infinite maps outside of the already used bounds. Please open this example.zip and try to place a rock somewhere. You should see that the first time it works, but then when you paint rocks in many places, it only places the flowers within the chunks that were allocated the first time the automapping was activated.

@ketanhwr

This comment has been minimized.

Show comment
Hide comment
@ketanhwr

ketanhwr Aug 25, 2017

Contributor

Oh 😕

I'll try to fix this.

Contributor

ketanhwr commented Aug 25, 2017

Oh 😕

I'll try to fix this.

@ketanhwr

This comment has been minimized.

Show comment
Hide comment
@ketanhwr

ketanhwr Aug 26, 2017

Contributor

What I've observed is when we apply automap and then paint outside the bounds and try automap again it doesn't work. Then if we undo the first automap and then paint outside the bounds and then try automap again, it works. What exactly is stored after calling 'Automap' for the first time ?

Contributor

ketanhwr commented Aug 26, 2017

What I've observed is when we apply automap and then paint outside the bounds and try automap again it doesn't work. Then if we undo the first automap and then paint outside the bounds and then try automap again, it works. What exactly is stored after calling 'Automap' for the first time ?

@ketanhwr

This comment has been minimized.

Show comment
Hide comment
@ketanhwr

ketanhwr Aug 26, 2017

Contributor

Okay, so after spending the past 3 hours into this, I can say that I've wasted the whole time 🙁

So actually, automapping works even if we paint out-of-bounds. If you test after my previous commit, you cannot visually see the automapped tile layer. If you save it and then restart Tiled, the automapped tile layer is clearly visible 🤦‍♂️

I have no idea why this is happening. Here's a gif of the same.

peek 2017-08-26 20-20

😢

Contributor

ketanhwr commented Aug 26, 2017

Okay, so after spending the past 3 hours into this, I can say that I've wasted the whole time 🙁

So actually, automapping works even if we paint out-of-bounds. If you test after my previous commit, you cannot visually see the automapped tile layer. If you save it and then restart Tiled, the automapped tile layer is clearly visible 🤦‍♂️

I have no idea why this is happening. Here's a gif of the same.

peek 2017-08-26 20-20

😢

@bjorn

bjorn requested changes Aug 26, 2017 edited

Okay, so after spending the past 3 hours into this, I can say that I've wasted the whole time

Actually it's not wasted, because you made me realize that it's probably to do with the bounds changed signal not being emitted. You know the thing you added to the TileLayerChangeWatcher. Can you verify this and look into a fix if that's the reason?

Your change to TileLayer::copy looks like a sensible code cleanup, so please keep that part.

Show outdated Hide outdated src/libtiled/tilelayer.cpp Outdated
@ketanhwr

This comment has been minimized.

Show comment
Hide comment
@ketanhwr

ketanhwr Aug 27, 2017

Contributor

Okay, automap is fixed now. But still some problem persists. When we undo, we need to zoom in/out or move around to see the effect of undo. This is because when we undo, the bounds of Tile Layer are not changed. But I don't think it's that big of a problem.

Contributor

ketanhwr commented Aug 27, 2017

Okay, automap is fixed now. But still some problem persists. When we undo, we need to zoom in/out or move around to see the effect of undo. This is because when we undo, the bounds of Tile Layer are not changed. But I don't think it's that big of a problem.

@bjorn

This comment has been minimized.

Show comment
Hide comment
@bjorn

bjorn Aug 27, 2017

Owner

Right, so this signal should actually be made part of the undo command. Would that be hard to do?

Owner

bjorn commented Aug 27, 2017

Right, so this signal should actually be made part of the undo command. Would that be hard to do?

@ketanhwr

This comment has been minimized.

Show comment
Hide comment
@ketanhwr

ketanhwr Aug 27, 2017

Contributor

This is because when we undo, the bounds of Tile Layer are not changed.

This is primarily the reason, the signal cannot emit. Since the bounds do not change after removing cells, we cannot actually check if it is necessary to emit this signal.

Contributor

ketanhwr commented Aug 27, 2017

This is because when we undo, the bounds of Tile Layer are not changed.

This is primarily the reason, the signal cannot emit. Since the bounds do not change after removing cells, we cannot actually check if it is necessary to emit this signal.

@bjorn

This comment has been minimized.

Show comment
Hide comment
@bjorn

bjorn Aug 27, 2017

Owner

This is primarily the reason, the signal cannot emit. Since the bounds do not change after removing cells, we cannot actually check if it is necessary to emit this signal.

If the bounds do not change, it is not necessary to emit the signal. Then there must be some other thing missing to trigger an update.

Owner

bjorn commented Aug 27, 2017

This is primarily the reason, the signal cannot emit. Since the bounds do not change after removing cells, we cannot actually check if it is necessary to emit this signal.

If the bounds do not change, it is not necessary to emit the signal. Then there must be some other thing missing to trigger an update.

ketanhwr added some commits Aug 27, 2017

@ketanhwr

This comment has been minimized.

Show comment
Hide comment
@ketanhwr

ketanhwr Aug 27, 2017

Contributor

Done! 🙂

Contributor

ketanhwr commented Aug 27, 2017

Done! 🙂

@bjorn bjorn merged commit e493426 into bjorn:master Aug 27, 2017

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bjorn

This comment has been minimized.

Show comment
Hide comment
@bjorn

bjorn Aug 27, 2017

Owner

Nice, thanks!

Owner

bjorn commented Aug 27, 2017

Nice, thanks!

@bjorn bjorn changed the title from Fix automap to Fix automap for infinite maps Dec 31, 2017

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