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

Automapping fix #1224 #1276

Merged
merged 2 commits into from Apr 3, 2017

Conversation

Projects
None yet
2 participants
@stefanbeller
Copy link
Contributor

commented May 14, 2016

see #1224 for an explanation.

It has been a long time of no tiled programming. It took me a while to remember the details as well as getting it to build (I still had qt4 configured ...)

stefanbeller added some commits May 14, 2016

.mailmap: Add Stefans corporate email address
While Google encourages to continue using an existing identity when having
contributed already, I chose to switch my email addresses. This makes it
easy later on to identify the copyright holder of the respective patch.

Signed-off-by: Stefan Beller <sbeller@google.com>
Automapping: Better map boundary handling
When comparing the automapping rules against the set layer, any out of
bounds condition would abort the checking.

We'd want to be careful however: Assume there are a empty tiles outside
the map boundary and carry on with the comparison.

Fixes 1224.
Signed-off-by: Stefan Beller <sbeller@google.com>

@stefanbeller stefanbeller force-pushed the stefanbeller:automappingfix1224 branch from bd60012 to e5ca653 May 14, 2016

@bjorn bjorn changed the title Automapping fix 1224 Automapping fix #1224 May 14, 2016

@bjorn

This comment has been minimized.

Copy link
Owner

commented May 24, 2016

Hey @stefanbeller, thanks for coming back to Tiled development! I'm sorry that I didn't provide any feedback on this change yet, but it's because I'd like to understand it and this needs a bit of time. :-)

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

This comment has been minimized.

Copy link
@bjorn

bjorn May 24, 2016

Owner

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?

This comment has been minimized.

Copy link
@stefanbeller

stefanbeller May 27, 2016

Author Contributor

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.

This comment has been minimized.

Copy link
@bjorn

bjorn May 30, 2016

Owner

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?

This comment has been minimized.

Copy link
@bjorn

bjorn May 30, 2016

Owner

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. :-/

This comment has been minimized.

Copy link
@stefanbeller

stefanbeller May 30, 2016

Author Contributor

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.

This comment has been minimized.

Copy link
@bjorn

bjorn May 30, 2016

Owner

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

@bjorn bjorn merged commit bbbe0ac into bjorn:master Apr 3, 2017

1 of 2 checks passed

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

This comment has been minimized.

Copy link
Owner

commented Apr 3, 2017

I've merged this even though some questions about the behavior of the code are still open, since it does fix the reported issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.