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

[Tiled] Support for hexagonal map orientation #143

Closed
craftworkgames opened this issue Mar 16, 2016 · 12 comments
Closed

[Tiled] Support for hexagonal map orientation #143

craftworkgames opened this issue Mar 16, 2016 · 12 comments

Comments

@craftworkgames
Copy link
Collaborator

Andy_E noticed that we currently don't support "hexagonal" map oriendation with Tiled added in version 0.11

One question though, my current C# Tiled classes are based on Tiled 0.15.1 which I need for Hex TMX maps. What version does MonoGame.Extended support?

I don't think this would be too difficult to add.

@lithiumtoast
Copy link
Contributor

Do we have a renderer for hexagonal or isometric Tiled maps?

@craftworkgames
Copy link
Collaborator Author

We currently support Orthogonal and Isometric.

We don't currently have Staggered or Hexagonal.

@lithiumtoast
Copy link
Contributor

Whoa. I didn't know we had an isometric renderer. Could definitely use a starter kit / demo example for each type of map orientation.

@craftworkgames
Copy link
Collaborator Author

Yeah totally. Our demos have improved but we've still got a number of things to add. I'm really excited about having more demos and starter kits.

We should probably raise a new issue about starter kits to explain what they are and how they are going to work.

@AndyE0
Copy link

AndyE0 commented Mar 19, 2016

I've begun looking through the source for adding in Hex support. I've got as far as TiledMapWriter. I have a query

var map = value.Map;
....
writer.Write(ConvertRenderOrder(map.RenderOrder).ToString());
....
....
writer.Write(Convert.ToInt32(map.Orientation));

Why the different treatment for these enumerations? I've already added "Hexagonal" to both TiledMapOrientation and TmxOrientation but I have a couple of new enums for StaggerAxis and StaggerIndex to add to the writer class.

@craftworkgames
Copy link
Collaborator Author

Yeah, that is a rather odd implementation. I suspect the only reason these are inconsistent is because they where implemented by different programmers. Either approach should work just fine, but I think I prefer the Convert.ToInt32 approach because it's simpler.

In C# an enum can be thought of as a named number anyway so converting it to a string and back again seems rather odd. If we look at the implementations of these particular enums it's even more clear that converting them to int's makes a lot of sense since they are explicitly defined that way.

public enum TmxOrientation
{
    [XmlEnum(Name = "orthogonal")] Orthogonal = 1,
    [XmlEnum(Name = "isometric")]  Isometric = 2,
    [XmlEnum(Name = "staggered")]  Staggered = 3
}

public enum TmxRenderOrder
{
    [XmlEnum(Name = "right-down")] RightDown = 1,
    [XmlEnum(Name = "right-up")]   RightUp = 2,
    [XmlEnum(Name = "left-down")]  LeftDown = 3,
    [XmlEnum(Name = "left-up")]    LeftUp = 4
}

Of course, the above enums are used when deserializing the XML and they have a one to one mapping with the below enums defined in the core library.

public enum TiledMapOrientation
{
    Orthogonal = 1,
    Isometric = 2,
    Staggered = 3
}

public enum TiledRenderOrder
{
    RightDown,
    RightUp,
    LeftDown,
    LeftUp
}

Given they are supposed to be a one to one mapping, the TiledRenderOrder enum should really have explicitly defined values like the TiledMapOrientation enum to make sure the convert code works properly.

One could argue that defining these numbers in two places is a code smell / maintenance pain but I'm not sure that argument holds much weight. Once these values are defined and tested there's really no reason to ever change them, so having them defined on both ends doesn't really matter. You just have to be careful when adding a new one.

So yeah, I'm happy if you want to go ahead and clean up this code to be more consistent. I have a feeling this code could be made even simpler with some thought. If you come up with a completely new way of doing it that's cool too. As long as it doesn't involve moving the XML serialization code into the core library. I don't think that would be a good idea.

@craftworkgames
Copy link
Collaborator Author

@AndyE0 Btw.. where are you working on these changes. I noticed you don't have a fork of MonoGame.Extended on your github profile.

@AndyE0
Copy link

AndyE0 commented Mar 19, 2016

Yes - I preferred the Convert.ToInt32 approach as well - the reason for my question as I couldn't see the point of other one and thought I was missing something obvious. I also don't have a problem with the dual definition approach because as you say it keeps the core library cleaner.

Regarding my on-going work my GitHub profile is the same age as my post above :-) I didn't have one until then. I've not used git much - I'm a Subversion user at work and I use it at home as well. Initially, all I'd done was download a zip of the source and build it but you know how it is..... I was looking at the source code this morning and before I knew what was happening code was being typed and compiled.

I'm guessing to fork it, I just click on the Fork button :-)

One further query, although there are classes for ObjectGroups and Objects there is no code for Import/Export yet - is that correct?

@AndyE0
Copy link

AndyE0 commented Mar 19, 2016

Consider it "forked".....

@AndyE0
Copy link

AndyE0 commented Mar 19, 2016

Ignore my query about ObjectGroups and Objects - it looks like the first zip download I made was from master and there are some follow-up changes on develop.

@lithiumtoast
Copy link
Contributor

@AndyE0 Where are you at with this?

@craftworkgames
Copy link
Collaborator Author

Closing this for now. We can always reopen it if @AndyE0 returns.

It would be nice to get this feature done but there hasn't been a lot of people asking for it so it's not really a top priority.

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

No branches or pull requests

2 participants