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.

Sign up for GitHub

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

Jump to bottom

Automapping fix #1224 #1276

Merged
merged 2 commits into from
Apr 3, 2017
Merged

Automapping fix #1224 #1276

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
274da53
.mailmap: Add Stefans corporate email address
stefanbeller May 14, 2016
e5ca653
Automapping: Better map boundary handling
stefanbeller May 14, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .mailmap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,6 @@ Samuli Tuomola <samuli.tuomola@gmail.com> stt <samuli.tuomola@gmail.com>
Alex Vega (semtiko) <semtiko@gmail.com> Alex Vega <semtiko@gmail.com>
Dennis Honeyman <arcticuno@gmail.com> Dennis Honeyman <dennis@dennis-laptop.(none)>
Yohann Ferreira <yohann.ferreira@orange.fr> Yohann Ferreira <yohann_dot_ferreira_at_orange_dot_efer>
Stefan Beller <stefanbeller@gmail.com> <stefanbeller@googlemail.com>
Stefan Beller <stefanbeller@gmail.com> <sbeller@google.com>

14 changes: 12 additions & 2 deletions src/tiled/automapper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -712,8 +712,18 @@ static bool compareLayerTo(const TileLayer *setLayer,
bool matchListYes = false;
bool matchListNo = false;

if (!setLayer->contains(x + offset.x(), y + offset.y()))
return false;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that rather than discarding the whole match just because the output layer does not contain every position in the rule region, you're instead taking a shortcut in only checking the listYes, since the listNo is irrelevant for determining an out-of-range match?

What I don't understand yet, is why when a layer in listYes does not contain this rule region position, should the match fail? Shouldn't that be treated like the cell being empty, thus doing continue?

In general, wouldn't it make sense to make sure the layers in listYes and listNo are all the same size, and all big enough to contain the ruleRegion? Then we can drop all the contains checks on them. Or is there some reason why this is not the case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that rather than discarding the whole match just because the output layer does not contain every position in the rule region,

right.

in the rule region, you're instead taking a shortcut in only checking the listYes, since the listNo is irrelevant for determining an out-of-range match?

right.

The assumption I made here is 'treat any position outside the map as if it contains no tile, i.e. the nullptr tile in the input region, but it is part of the region to inspect'.

What I don't understand yet, is why when a layer in listYes does not contain this rule region position, should the match fail? Shouldn't that be treated like the cell being empty, thus doing continue?

Looking at #1224, the first solution I came up with in code here looked like
1224_wrong

which comes from 'the stone may be just outside the map', and we don't want to have that IMHO.

So when we assume there are only nulls outside, we don't have to check any of listNo, as there is nothing to exclude (because we assume nulls).

If there is a position in listYes though, that tile is asked to be unequal to null, i.e. we want to check if there is that tile is not null. I'll look into that again if we could come up with a saner logic for that example case presented in #1224.

In general, wouldn't it make sense to make sure the layers in listYes and listNo are all the same size,

I think it was intentional to allow arbitrary sizes.

and all big enough to contain the ruleRegion? Then we can drop all the contains checks on them. Or is there some reason why this is not the case?

oh. I'd have to think about that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which comes from 'the stone may be just outside the map', and we don't want to have that IMHO.

Right, but I was asking why the match failed when a layer in listYes does not contain this rule region position, whereas in the case of the stone, the listYes does contain the position but the setLayer doesn't, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried a different fix: simply assuming that tiles outside the set layer are empty, by doing:

Cell c1;
if (setLayer->contains(x + offset.x(), y + offset.y()))
    c1 = setLayer->cellAt(x + offset.x(), y + offset.y());

However, it did not work, and I have no idea why. Can you explain this? I still find all this rather hard to understand. :-/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, it did not work, and I have no idea why.

I also find this hard to understand, so I started making a KV map of all boolean conditions.
However the answer came up when looking at the rule, which states:

  • Examine a 3x3 region (as instructed by the regions layer)
  • in the middle there must be a rock matching in the Ground layer (as there is a rock placed in the middle in the input_Ground layer)
  • in the outer tiles there must be something other than (rock tile, empty tile).

The last condition seems to be a little confusing.
The rock is excluded because of the exception mentioned in the comment above the function. Essentially to deal with situations like two adjacent rocks and then not putting flowers over each other. The empty tile is excluded because we need to have 'something' there for matching.

When using your suggestion (3 times, for c1 as well as for c2 in the loops below), you can archive the corner cases in another simple different way by adding an 'inputnot_Ground' layer to the rule and put rocks in the outer tiles.
Then you remove the condition for having 'something' around.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the outer tiles there must be something other than (rock tile, empty tile).

Hmm, what I still don't understand, is why then does it work for the rocks that are not on the edge? Because they too have empty tiles around them, just like rocks on the edge when you treat out-of-bounds as empty tiles, right?

Also, actually I don't understand why the empty tile is in that exclude list. :S


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

const Cell &c1 = setLayer->cellAt(x + offset.x(),
y + offset.y());
Expand Down