Fixed TiledMap.GetSourceRect #86
Conversation
TiledMap.GetSourceRect wasn't returning the correct frame with tile spacing in mind. TODO: figure out what margins do and update this method again
|
Hey Thanks for the fix! I would like to test it out myself as well and see what I can do with the margin after the merge. Can you share your tileset with me? I never had a tileset with margins or spacing before. And to be honest, I still do not understand why some artists do that. lol With kind regards, |
|
Giving spacing ( and I think margins too ) to tilesets is kinda helps you differ between sort of similar tiles by adding a gaps of pixels that are skipped over and not rendered in between each tile:
The Tiled tileset editor when being prompted for an image also asks you if you margined or spaced out your tiles, this way it can render your tiles correctly by skipping over 1 pixel every frame:
If I were to remove the spacing from the tileset properties but keep it in the sprite my tiles when rendered would look something like this:
This is also what a rendered map that uses TiledMap.GetSourceRect to get each tile's frame would look like if I tried to render tiles with spacing. By adding the tileset's spacing multiplied by its frame onto the frame's x and y values you'd be skipping over the spacing that was inputted allowing the tiles to be rendered properly. |
|
Hey Sorry for the late reply. Thanks a lot for the explanation! That does make sense indeed. I do have one question though. Do you think you can still share one of your test maps with me, please? That way I can test it properly and round things up. With kind regards, |
|
Hey @Frankfires I still have not heard back from you. Do you mind to share your tileset with me so I can properly test things? Thanks! With kind regards, |
|
If it helps at all, here is another texture which requires Spacing=1 and Margin=1... |
| @@ -707,8 +707,8 @@ public TiledSourceRect GetSourceRect(TiledMapTileset mapTileset, TiledTileset ti | |||
| if (i == gid - mapTileset.firstgid) | |||
| { | |||
| var result = new TiledSourceRect(); | |||
| result.x = tileHor * tileset.TileWidth; | |||
| result.y = tileVert * tileset.TileHeight; | |||
| result.x = tileHor * tileset.TileWidth + tileset.Spacing * tileHor; | |||
There was a problem hiding this comment.
Could just put parathesis around (tileset.TileWidth + tileset.Spacing) so that only one multiply is performed vs 2. This would before both of these lines (but height for the second obviously)
There was a problem hiding this comment.
Hey, I was not aware others could review too. Was a bit surprised about that to be honest. But thanks for taking your time to take a look at the PR!
Could you elaborate on why the parentheses need to be added? Did you got some incorrect results? As you may have noticed I have not spent much time in this repository due to my focus shifted to other projects so it has been a while since I took a look at this PR.
There was a problem hiding this comment.
Technically they don't "need" to be added. It was just a suggestion for (premature optimization) I suppose.
But my comment on the ticket says that PR #94 actually has the same fix you have here, but also has a fix for Margin, so it should be probably be used instead of your fix since it fixes 2 different things.
I didn't mean to indicate your suggestion was "wrong", just wanted to see if I could help spark a conversation that gets some of these PR's merged in.
There was a problem hiding this comment.
Well, I am not the author of this PR so it was not my suggestion. But I agree it could improve readability. Mathematically it works as it should but I get where you're coming from. Will have a look at both PR and compare them.
There was a problem hiding this comment.
Oh my, that's what I get for replying while watching Hulu. Sorry i thought you were the requestor. Haha yes I also thought it was odd I could review things on your repository.
There was a problem hiding this comment.
Oh fuck me.. haha I found only one setting under moderation that I thought would have done the trick. But apparently GitHub decided otherwise. lol
There was a problem hiding this comment.
Yeah this article makes it sound like it's open to public??? Weird? Now I gotta go check my public repos lol
There was a problem hiding this comment.
Perhaps I can approve but I can't merge?
There was a problem hiding this comment.
Yea I found the same page. Already consulted the GitHub community on Reddit cause I am a bit at loss here now. I also can only draw the same conclusion that apparently anyone can fuck with your pull requests when your repo is public. Not really a desired situation imho.
I wish to keep control over my code and it feels like I barely have control over how anyone can interact with my repository. First the branching regulations and now this crap. lol
Let's rest it for now. I am glad I figured it out this way rather than the nasty way. I will wait out the response on Reddit and see if I can contact GitHub support to ask the same question. Other than that, I will have to deal with the consequences as long as this repo is public I guess. Thank you for taking your time by the way, in all hurries forgot to even properly say thanks. So hereby, thanks!
There was a problem hiding this comment.
Yeah it appears github at some point (perhaps always?) made public repos open.
You'd have to lock the branch down with a branch protection option in settings.
Settings > code and automation > branches
Then down under branch protection click add rule and see if those settings work.
|
Did a quick review, Not knowing much about the properties available on the class, assuming that the Spacing is a valid property on tileSet, then I think the code could be merged. However, may want to adjust the math to do the addition first, and the multiply second, so only 1 multiply is performed vs 2 per line. |
|
It appears #94 has an added fix that also includes margins in the position calculation. May want to use that one instead of the PR from this one. |
TiledMap.GetSourceRect wasn't returning the correct frame with tile spacing in mind.
TODO: figure out what margins do and update this method again