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

Infinite Layer Support with Chunk Loading #26

Closed
wants to merge 2 commits into from

Conversation

Bryan-Legend
Copy link

Replaces #25. Sorry, new to git.

…er chunk merging

OffsetX & OffsetY changed to doubles & added support for infinite layer chunk merging. The infinite layer support is not well and could be buggy, but should work.
@TheBoneJarmer
Copy link
Owner

Hi @Bryan-Legend

I am sorry for the late reply. I have had it very busy last two weeks, a funeral and a upcoming wedding to plan. I will pull your changes today and review that. I do have one question though, do you happen to have a tiledmap and tileset I could use to test with?

I can use my own of course but the fact is that I sometimes not be able to recreate the issue you encountered and sent in a pull request for. Especially with yours here. I do not have a map that big or simply infinite. And while your changes look good, I also like to check out the performance.

With kind regards,
TheBoneJarmer

@TheBoneJarmer
Copy link
Owner

Hi @Bryan-Legend

One more thing, just a very small piece of advice since you mentioned you are new to git. You are trying to merge in from the main branch of your fork to my develop branch, which is not bad per se. But it is fine because TiledCS is not such a big project.

But for future projects I highly recommend using the same source branch as target branch. Mainly because the develop branch in other projects may differ like 100s of commits from the main or master branch because in case of big projects, releases do not happen that often and if done, very carefully and thoroughly tested. And in worse case, it may cause conflicts or your changes may not work anymore.

For now it is fine really since the main branch of this repo is at the same head as the develop branch. And the project is not that big so merges happen rather fast.

With kind regards,
TheBoneJarmer

@TheBoneJarmer TheBoneJarmer self-assigned this Nov 24, 2021
@TheBoneJarmer
Copy link
Owner

Hi @Bryan-Legend

Finally got some time to look at this. But I am not sure where to start and how to say this politely. But I think your code is a mess. It was rather hard to read and on top of that, you seemed to have replaced existing functionality. While this works for you, which is perfectly fine, I cannot merge this into develop.

That said, I got to ask. How long have you been coding in C#? Because I am happy to give you some feedback on your code here because I think it will help you with the overall code quality in your projects. And obviously I will still add support for infinite layers since that is what you are trying to achieve. It is not like I reject this PR and leave it be. No worries about that.

With kind regards,
TheBoneJarmer

@Bryan-Legend
Copy link
Author

LOL! I've been coding C# full time since the first beta of C# 1 back in 2000. :)

I agree the code is a mess. I honestly didn't even expect a response to the pull request and wasn't going to invest much time in it.

This code was used a week or two ago in a 48 hour game jam. https://itch.io/jam/utah-indie-game-jam-2021/rate/1277167

During the jam we discovered a major problem with my code. The chunk positions can be negative causing index out of bounds exception. We averted this crisis by unchecking the infinite map option in Tiled and re-saving the map.

@TheBoneJarmer
Copy link
Owner

During the jam we discovered a major problem with my code. The chunk positions can be negative causing index out of bounds exception. We averted this crisis by unchecking the infinite map option in Tiled and re-saving the map.

Ok, that is good to know. Makes sense actually considering the fact the map is infinite. Also glad to hear you agree about the state of the code. Was worried I insulted you some way. :D

That said, amazing that you used TiledCS for your submission in the game jam! Just gave it a look. Out of 17 your game ended up being number 5. Well done!

@Bryan-Legend
Copy link
Author

Bryan-Legend commented Nov 24, 2021 via email

@TheBoneJarmer
Copy link
Owner

I agree. And to tell you the truth, I try to mimic the structure of the TMX file as much as possible. Hence, I would have kept the chunks separated. And I will do so as well. Resulting in a new property called chunks on the TiledLayer class, which is of type TiledLayerChunck[] and will in fact represent all the chunks of data. That is approach I have always used up till now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants