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

Add options to customize "MatchOutsideMap" behavior #2141

Merged
merged 6 commits into from Jun 20, 2019

Conversation

Projects
None yet
3 participants
@JoaoBaptMG
Copy link
Contributor

commented Jun 15, 2019

I decided to edit Tiled's source code after viewing this discussion about the behavior of automapping near the map's edges and I was not satified with the state of the art of automapping near the borders.

Normally, when you try to automap near the edges, the edges are seen as empty tiles, failing to apply, for example, Remex's auto-generated rules (the ones originated from RPG Maker):
image

This pull request add two options (as map properties) to customize the behavior of automapping near the edges.

  • OverflowBorder: this option makes the automapper treats the out-of-bounds areas of the map as if they were prolongations of the border tiles, thus applying the rules along the borders as if they used the same tiles as the nearest inbound tile. This is actually the default behavior of every RPG Maker since XP.
    image
  • WrapBorder: this option makes the automapper wrap the map around the edges, effectively applying the rules also to the other side of the border; this can be useful to create looped maps, to make it seamless around the borders:
    image

I coded those two features so that if both OverflowBorder and WrapBorder are true, WrapBorder takes precedence. And, of course, it will not work with infinite maps, since the notion of "border" in them has no sense.

@bjorn
Copy link
Owner

left a comment

Thank you so much for this improvement! I think it will indeed help a lot dealing with these "edge cases".

I've left some inline comments. In addition, I wondered about the following:

  • Would it make sense to have the OverflowBorder and WrapBorder options imply MatchOutsideMap = true? It feels superfluous to require the user to specify both.

  • Maybe instead of passing in whether the map is infinite, we can make sure these options are never enabled for infinite maps.

xd = setLayer.size().width() - 1;
if (yd < 0) yd = 0;
else if (yd >= setLayer.size().height())
yd = setLayer.size().height() - 1;

This comment has been minimized.

Copy link
@bjorn

bjorn Jun 15, 2019

Owner

Please shorten this to:

xd = qBound(0, xd, setLayer.width() - 1);
yd = qBound(0, yd, setLayer.height() - 1);

This comment has been minimized.

Copy link
@JoaoBaptMG

JoaoBaptMG Jun 15, 2019

Author Contributor

Thanks! I have never written a line of Qt code before, so I didn't know of that function. That could be very useful, indeed!

if (!isMapInfinite) {
if (options.wrapBorder) {
int width = setLayer.size().width();
int height = setLayer.size().height();

This comment has been minimized.

Copy link
@bjorn

bjorn Jun 15, 2019

Owner

Please use setLayer.width() and setLayer.height(). :-)

Show resolved Hide resolved src/tiled/automapper.cpp
@JoaoBaptMG

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2019

* Would it make sense to have the OverflowBorder and WrapBorder options imply MatchOutsideMap = true? It feels superfluous to require the user to specify both.

I will incorporate this into my code as such, but I think it might be useful if the user disables MatchOutsideMap but forgets to do so with OverflowBorder/WrapBorder (well, that doesn't make any sense, but I guess it's a possible scenario?)

* Maybe instead of passing in whether the map is infinite, we can make sure these options are never enabled for infinite maps.

That could also be possible, and will remove excessive parameter passing.

@JoaoBaptMG
Copy link
Contributor Author

left a comment

I incorporated the changes you requested, so I guess it's a bit more "canonical" now. I hope those features are useful to solve the automapping quirks I pointed out.

Show resolved Hide resolved src/tiled/automapper.cpp
Show resolved Hide resolved src/tiled/automapper.cpp
Show resolved Hide resolved src/tiled/automapper.cpp
@bjorn
Copy link
Owner

left a comment

Looking good, I just had one additional request. :-)

@@ -42,6 +42,11 @@

using namespace Tiled;

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

This comment has been minimized.

Copy link
@bjorn

bjorn Jun 16, 2019

Owner

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

This comment has been minimized.

Copy link
@JoaoBaptMG

JoaoBaptMG Jun 16, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@bjorn

bjorn Jun 16, 2019

Owner

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.

This comment has been minimized.

Copy link
@bjorn

bjorn Jun 16, 2019

Owner

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

This comment has been minimized.

Copy link
@JoaoBaptMG

JoaoBaptMG Jun 16, 2019

Author Contributor

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

@JoaoBaptMG
Copy link
Contributor Author

left a comment

Done all changes needed to the code!

@JoaoBaptMG
Copy link
Contributor Author

left a comment

Removed the inline, since probably the compiler will not complain about its abscence (and maybe it would inline the code with optimizations anyway).

Show resolved Hide resolved src/tiled/automapper.cpp
@bjorn

bjorn approved these changes Jun 17, 2019

@bjorn bjorn merged commit 063fe88 into bjorn:master Jun 20, 2019

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@bjorn

This comment has been minimized.

Copy link
Owner

commented Jun 20, 2019

@JoaoBaptMG Thanks again for this great improvement!

Would you like to have a go at updating the documentation for these attributes as well?

@JoaoBaptMG

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

Yeah, I can do that. However, since the PR is already merged, should I open another PR for it or can I commit it to my fork?

@bjorn

This comment has been minimized.

Copy link
Owner

commented Jun 20, 2019

@JoaoBaptMG Yes, just open a new PR! :-)

@the-simian

This comment has been minimized.

Copy link

commented Jun 20, 2019

I'm late to the party here, but let me say this is awesome as can be, and I am so happy to see this work @JoaoBaptMG , @bjorn

So very cool! WhenI work on my project create-phaser-app I will include this change and make that how the levels work

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.