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

Add options to customize "MatchOutsideMap" behavior #2141

Merged
merged 6 commits into from
Jun 20, 2019
Merged

Add options to customize "MatchOutsideMap" behavior #2141

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9fcbc6e
Add options "OverflowBorder" and "WrapBorder" to customize "MatchOuts…
JoaoBaptMG Jun 15, 2019
1579d7b
Add forgotten check for infinite map in order not to produce bugs
JoaoBaptMG Jun 15, 2019
4f0898f
Incorporate changes requested by bjorn on the pull request
JoaoBaptMG Jun 15, 2019
9e4fb2d
Turned 'wrap' method static
JoaoBaptMG Jun 16, 2019
6441892
Turned 'wrap' function static
JoaoBaptMG Jun 16, 2019
c96a745
Tweaks
bjorn Jun 20, 2019
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
63 changes: 56 additions & 7 deletions src/tiled/automapper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,11 @@

using namespace Tiled;

// Should i leave this function here?
inline int wrap(int value, int bound) {
Copy link
Member

Choose a reason for hiding this comment

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

The location is fine for now, but please define the function static and put the { on the next line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

inline static or static inline? I know there's no difference, but maybe there is a convention in Tiled's code?

Copy link
Member

Choose a reason for hiding this comment

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

Actually I doubt there is any precedence here since I don't really see the point of defining a function both inline and static.

I actually only use inline when I need to use it. That is, to avoid multiple-definition problems for function definitions in header files that are outside class definitions. In any other case it's at best a hint that the compiler is free to ignore (and probably will, it'll inline only when it thinks it's a good idea anyway).

The static however is appropriate here, since we don't intend the function to be used from another translation unit. Defining it as static makes it inaccessible outside of its translation unit.

Copy link
Member

Choose a reason for hiding this comment

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

Actually I doubt there is any precedence here since I don't really see the point of defining a function both inline and static.

Maybe I should search the code before raising doubts. So, there's precedence for both inline static and static inline, with the latter a clear winner. However, of 9 total occurrences, 8 were inherited from 3rd party code. The one remaining one, I'm probably going to change to just static soon. :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a problem, I will leave it just as static. Commiting the code again.

return (value % bound + bound) % bound;
}

bjorn marked this conversation as resolved.
Show resolved Hide resolved
/*
* About the order of the methods in this file.
* The Automapper class has 3 bigger public functions, that is
Expand Down Expand Up @@ -108,6 +113,16 @@ bool AutoMapper::setupRuleMapProperties()
mOptions.matchOutsideMap = value.toBool();
continue;
}
} else if (name.compare(QLatin1String("OverflowBorder"), Qt::CaseInsensitive) == 0) {
if (value.canConvert(QVariant::Bool)) {
mOptions.overflowBorder = value.toBool();
continue;
}
} else if (name.compare(QLatin1String("WrapBorder"), Qt::CaseInsensitive) == 0) {
if (value.canConvert(QVariant::Bool)) {
mOptions.wrapBorder = value.toBool();
continue;
}
} else if (name.compare(QLatin1String("AutomappingRadius"), Qt::CaseInsensitive) == 0) {
if (value.canConvert(QVariant::Int)) {
mOptions.autoMappingRadius = value.toInt();
Expand All @@ -124,6 +139,17 @@ bool AutoMapper::setupRuleMapProperties()
"Ignoring this property.")
.arg(mRulePath, name, value.toString()) + QLatin1Char('\n');
}

// OverflowBorder and WrapBorder make no sense for infinite maps
if (mMapWork->infinite()) {
mOptions.overflowBorder = false;
mOptions.wrapBorder = false;
}

// Each of the border options imply MatchOutsideMap
if (mOptions.overflowBorder || mOptions.wrapBorder)
mOptions.matchOutsideMap = true;

bjorn marked this conversation as resolved.
Show resolved Hide resolved
return true;
}

Expand Down Expand Up @@ -627,12 +653,24 @@ static bool layerMatchesConditions(const TileLayer &setLayer,
#endif
for (int x = rect.left(); x <= rect.right(); ++x) {
for (int y = rect.top(); y <= rect.bottom(); ++y) {
int xd = x + offset.x();
int yd = y + offset.y();
bjorn marked this conversation as resolved.
Show resolved Hide resolved

if (!options.matchOutsideMap &&
!setLayer.contains(x + offset.x(), y + offset.y()))
!setLayer.contains(xd, yd))
return false;

const Cell &setCell = setLayer.cellAt(x + offset.x(),
y + offset.y());
// Those two options are guaranteed to be false if the map is infinite,
// so no "invalid" width/height accessing here.
if (options.wrapBorder) {
xd = wrap(xd, setLayer.width());
yd = wrap(yd, setLayer.height());
} else if (options.overflowBorder) {
xd = qBound(0, xd, setLayer.width() - 1);
yd = qBound(0, yd, setLayer.height() - 1);
bjorn marked this conversation as resolved.
Show resolved Hide resolved
}

const Cell &setCell = setLayer.cellAt(xd, yd);

// First check listNo. If any tile matches there, we can
// immediately know there is no match.
Expand Down Expand Up @@ -836,11 +874,14 @@ void AutoMapper::copyTileRegion(const TileLayer *srcLayer, int srcX, int srcY,
int endX = dstX + width;
int endY = dstY + height;

if (!mMapWork->infinite()) {
int dwidth = dstLayer->width();
int dheight = dstLayer->height();

if (!mOptions.wrapBorder && !mMapWork->infinite()) {
startX = qMax(0, startX);
startY = qMax(0, startY);
endX = qMin(dstLayer->width(), endX);
endY = qMin(dstLayer->height(), endY);
endX = qMin(dwidth, endX);
endY = qMin(dheight, endY);
}

const int offsetX = srcX - dstX;
Expand All @@ -851,7 +892,15 @@ void AutoMapper::copyTileRegion(const TileLayer *srcLayer, int srcX, int srcY,
const Cell &cell = srcLayer->cellAt(x + offsetX, y + offsetY);
if (!cell.isEmpty()) {
// this is without graphics update, it's done afterwards for all
dstLayer->setCell(x, y, cell);
int xd = x;
int yd = y;

// WrapBorder is only true on finite maps
if (mOptions.wrapBorder) {
xd = wrap(xd, dwidth);
yd = wrap(yd, dheight);
}
dstLayer->setCell(xd, yd, cell);
}
}
}
Expand Down
12 changes: 12 additions & 0 deletions src/tiled/automapper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,18 @@ class AutoMapper : public QObject
*/
bool matchOutsideMap = true;

/**
* If "matchOutsideMap" is true, treat the out-of-bounds tiles as if they
* were the nearest inbound tile possible
*/
bool overflowBorder = false;

/**
* If "matchOutsideMap" is true, wrap the map in the edges to apply the
* automapping rules
*/
bool wrapBorder = false;

/**
* Determines if a rule is allowed to overlap itself.
*/
Expand Down