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

Fix automap for infinite maps #1692

Merged
merged 8 commits into from Aug 27, 2017
Merged

Fix automap for infinite maps #1692

merged 8 commits into from Aug 27, 2017

Conversation

@ketanhwr
Copy link
Contributor

@ketanhwr ketanhwr commented Aug 20, 2017

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

Copy link
Member

@bjorn bjorn left a comment

See inline comments.

Does this also make infinite maps work for automapping rules?

int endY = dstY + height;

if (!mMapWork->infinite()) {
startX = qMax(dstX, startX);

This comment has been minimized.

@bjorn

bjorn Aug 21, 2017
Member

dstX and startX are the same at this point. You meant 0 instead of dstX.

r &= QRect(dx, dy, other->width(), other->height());
} else {
r = rect;
}

This comment has been minimized.

@bjorn

bjorn Aug 21, 2017
Member

Why do you need the rect parameter? As far as I can see, r just needs to cover both layers and it should be in local coordinates, so this should do it:

const QRect r = bounds().united(other->bounds()).translated(-position());
@ketanhwr
Copy link
Contributor Author

@ketanhwr ketanhwr commented Aug 21, 2017

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

@@ -691,7 +691,7 @@ static bool compareLayerTo(const TileLayer *setLayer,
bool matchListNo = false;


if (!setLayer->contains(x + offset.x(), y + offset.y())) {
if (!(setLayer->contains(x + offset.x(), y + offset.y()) || setLayer->map()->infinite())) {

This comment has been minimized.

@bjorn

bjorn Aug 21, 2017
Member

Intuitively, this check and the special handling inside should actually not be necessary at all. If the setLayer does not contain the given location, then c1 will just be an empty cell, which I think should be treated the same as a cell outside of the layer boundaries. Can you check this?

This comment has been minimized.

@ketanhwr

ketanhwr Aug 21, 2017
Author Contributor

No no, it needs to have that check. I found it strange too, but it doesn't work if this check is removed.

This comment has been minimized.

@bjorn

bjorn Aug 21, 2017
Member

Can you give an example of where it goes wrong?

This comment has been minimized.

@ketanhwr

ketanhwr Aug 21, 2017
Author Contributor

This is what happens when I remove the check:
screenshot from 2017-08-21 17-10-38
This is what happens when I keep the check (this obviously is the expected behaviour):
screenshot from 2017-08-21 17-11-05

This comment has been minimized.

@bjorn

bjorn Aug 21, 2017
Member

What part did you try removing exactly? I suggested removing this entire bit of code:

                if (!(setLayer->contains(x + offset.x(), y + offset.y()) || setLayer->map()->infinite())) {
                    foreach (const TileLayer *comparedTileLayer, listYes) {
                        if (!comparedTileLayer->contains(x, y))
                            return false;

                        const Cell &c2 = comparedTileLayer->cellAt(x, y);
                        if (!c2.isEmpty())
                            return false;
                    }
                    continue;
                }

It's not clear to me what is happening from those screenshots.

This comment has been minimized.

@ketanhwr

ketanhwr Aug 21, 2017
Author Contributor

Aah, I thought you meant setLayer->map()->infinite()

@bjorn
Copy link
Member

@bjorn 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
Copy link
Member

@bjorn 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
Copy link
Contributor Author

@ketanhwr ketanhwr commented Aug 25, 2017

Oh 😕

I'll try to fix this.

@ketanhwr
Copy link
Contributor Author

@ketanhwr 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
Copy link
Contributor Author

@ketanhwr 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

😢

Copy link
Member

@bjorn bjorn left a comment

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.

@@ -236,7 +233,10 @@ void TileLayer::setCells(int x, int y, TileLayer *layer,
const QRegion &mask)
{
// Determine the overlapping area
QRegion area = QRect(x, y, layer->width(), layer->height());
int width = qMax(layer->width(), layer->bounds().width());
int height = qMax(layer->height(), layer->bounds().height());

This comment has been minimized.

@bjorn

bjorn Aug 26, 2017
Member

I think this makes little sense, please revert it back to what it was.

This comment has been minimized.

@ketanhwr

ketanhwr Aug 27, 2017
Author Contributor

But why? Suppose a layer was created with size (1, 1) and then some cells were set out-of-bounds. If this layer is then used in setCells for some other layer, then technically, these 'out-of-bounds' cells should also be set in this layer. Don't you think?

This comment has been minimized.

@bjorn

bjorn Aug 27, 2017
Member

Well, the part that does not make sense, is to take the width/height of the bounds but still copy starting from 0,0.

This comment has been minimized.

@ketanhwr

ketanhwr Aug 27, 2017
Author Contributor

So I guess we need to unite QRect(0, 0, layer->width(), layer->height() with layer->bounds().

This comment has been minimized.

@ketanhwr

ketanhwr Aug 27, 2017
Author Contributor

And adjust the offset accordingly.

This comment has been minimized.

@bjorn

bjorn Aug 27, 2017
Member

No, if you use the bounds then the rect() of the layer is completely irrelevant.

This comment has been minimized.

@ketanhwr

ketanhwr Aug 27, 2017
Author Contributor

I got confused by something else 😕

@ketanhwr
Copy link
Contributor Author

@ketanhwr 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
Copy link
Member

@bjorn 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
Copy link
Contributor Author

@ketanhwr 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
Copy link
Member

@bjorn 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 3 commits Aug 27, 2017
@ketanhwr
Copy link
Contributor Author

@ketanhwr ketanhwr commented Aug 27, 2017

Done! 🙂

@bjorn bjorn merged commit e493426 into mapeditor:master Aug 27, 2017
1 of 2 checks passed
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
Copy link
Member

@bjorn bjorn commented Aug 27, 2017

Nice, thanks!

@bjorn bjorn changed the title Fix automap 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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.