-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/tiled/automapper.cpp
Outdated
xd = setLayer.size().width() - 1; | ||
if (yd < 0) yd = 0; | ||
else if (yd >= setLayer.size().height()) | ||
yd = setLayer.size().height() - 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please shorten this to:
xd = qBound(0, xd, setLayer.width() - 1);
yd = qBound(0, yd, setLayer.height() - 1);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
src/tiled/automapper.cpp
Outdated
if (!isMapInfinite) { | ||
if (options.wrapBorder) { | ||
int width = setLayer.size().width(); | ||
int height = setLayer.size().height(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use setLayer.width()
and setLayer.height()
. :-)
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?)
That could also be possible, and will remove excessive parameter passing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, I just had one additional request. :-)
src/tiled/automapper.cpp
Outdated
@@ -42,6 +42,11 @@ | |||
|
|||
using namespace Tiled; | |||
|
|||
// Should i leave this function here? | |||
inline int wrap(int value, int bound) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
andstatic
.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done all changes needed to the code!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the inline
, since probably the compiler will not complain about its abscence (and maybe it would inline the code with optimizations anyway).
@JoaoBaptMG Thanks again for this great improvement! Would you like to have a go at updating the documentation for these attributes as well? |
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? |
@JoaoBaptMG Yes, just open a new PR! :-) |
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 |
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):
This pull request add two options (as map properties) to customize the behavior of automapping near the edges.
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.