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

FlxTilemap#loadMap(): support Array<Array<Int>> instead of Array<Int>, closes #1290, closes #1324 #1292

Merged
merged 6 commits into from
Oct 12, 2014

Conversation

Gama11
Copy link
Member

@Gama11 Gama11 commented Sep 10, 2014

Supporting both 2D and 1D-arrays here is suprisingly difficult due to the use of Dynamic and target-specfic differences, so I decided to drop support for the latter. A 2D array is much more natural representation of map data anyway. Loading maps with array is very inconvenient / awkward as is, as widthInTiles and heightInTiles have to be set in advance.

@Gama11
Copy link
Member Author

Gama11 commented Sep 21, 2014

@gamedevsam You think this is an acceptable breaking change?

@gamedevsam
Copy link
Contributor

Does this break compatibility with maps loaded from Tiled Editor?

@Gama11
Copy link
Member Author

Gama11 commented Sep 21, 2014

Not sure, I think they use csv for loading?

@gamedevsam
Copy link
Contributor

No, they use in array that can be compressed with gzip or json.

@Gama11
Copy link
Member Author

Gama11 commented Sep 28, 2014

Perhaps we should split this up into several functions? loadMap(), loadMapFromArray() and loadMapFrom2DArray()? Would also get rid of the Dynamic usage.

@gamedevsam
Copy link
Contributor

Perhaps, but I don't like the idea of duplicating code...

@Gama11
Copy link
Member Author

Gama11 commented Sep 29, 2014

If done right, this shouldn't introduce any redundancies.

@gamedevsam
Copy link
Contributor

Seems like a good change then, although it does complicate the API slightly. Also LoadMap should be refactored to be LoadMapFromCSV()

Let's make it all very explicit.

@Gama11
Copy link
Member Author

Gama11 commented Sep 29, 2014

If only haxe supported overloading. :)

I don't think this complicates the API actually, it might make it simpler to understand actually.

@Gama11
Copy link
Member Author

Gama11 commented Oct 5, 2014

Hm... Since the render refactor, there's a loadMap() and a new loadMapFrames(), which kinda conflicts with this... Thoughts @Beeblerox?

@Beeblerox
Copy link
Member

@Gama11 i don't think that there is big problem with updating pull request. I can do it myself if you want to.
loadMapFrames() does almost everything as loadMap, but takes exisiting FlxTileFrames as an argument instead of FlxGraphicAsset. which allows you to load graphic from atlas.

@Gama11
Copy link
Member Author

Gama11 commented Oct 5, 2014

Yes, that's exactly the issue. I wanted to split loadMap() into loadMapFromCsv(), loadMapFromArray() and loadMapFrom2DArray() - if each one of those needs a Frames-version as well, we'd end up with 6 functions.

@Beeblerox
Copy link
Member

maybe we could add some sort of helper method, which will convert 1d-array to 2d-array to avoid this?

@Gama11
Copy link
Member Author

Gama11 commented Oct 5, 2014

That would work, but it seems pretty inefficient - convert 1d array to 2d and then back again to 1d (since that's the format that FlxTilemap ends up using).

@Beeblerox
Copy link
Member

let me some time to think about it

@Beeblerox
Copy link
Member

i've removed loadMapFrame() here: 9a84e25997dd
loadMap() can accept tile frames (added couple casting operations, but i think that tilemap loading isn't frequent operation, so it acceptable)
so you could split loadMap() into loadMapFromCsv(), loadMapFromArray() and loadMapFrom2DArray() as you wanted

Conflicts:
	flixel/system/FlxAssets.hx
@Gama11
Copy link
Member Author

Gama11 commented Oct 10, 2014

Updated.

// Figure out the map dimensions based on the data string
_data = new Array<Int>();
var columns:Array<String>;
var rows:Array<String> = StringTools.trim(cast (MapData, String)).split("\n");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why casting here?

@Beeblerox
Copy link
Member

i think this pull request can be merged, but we'll need to update TiledEditor, HeatmapPathfinding, Tilemap, GridMovement and FlipRotationAnimationTiles demoes, since it makes widthInTiles and heightInTiles read-only

@Beeblerox
Copy link
Member

and other demoes where tilemaps are used

@Gama11 Gama11 changed the title FlxTilemap#loadMap(): support Array<Array<Int>> instead of Array<Int>, closes #1290 FlxTilemap#loadMap(): support Array<Array<Int>> instead of Array<Int>, closes #1290, closes #1324 Oct 12, 2014
Gama11 added a commit that referenced this pull request Oct 12, 2014
FlxTilemap#loadMap(): support Array<Array<Int>> instead of Array<Int>, closes #1290, closes #1324
@Gama11 Gama11 merged commit 077eab8 into HaxeFlixel:dev Oct 12, 2014
Gama11 added a commit to HaxeFlixel/flixel-addons that referenced this pull request Oct 12, 2014
Gama11 added a commit to HaxeFlixel/flixel-demos that referenced this pull request Oct 12, 2014
@Gama11 Gama11 deleted the tilemapLoadMap2Darray branch May 11, 2016 14:43
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.

3 participants