FlxTilemap refactoring : Separation of logic/display #1101
Conversation
| private var _data:Array<Int>; | ||
|
|
||
| /** | ||
| * Virtual methods, must be implemented in each renderers |
There was a problem hiding this comment.
Could this be solved by using an abstract?
There was a problem hiding this comment.
What part ? List of virtual methods ? I thought about it but couldn't find any doc about how to do that, the keyword abstract is not making classes virtual
There was a problem hiding this comment.
I can build a macro though to reproduce the behavior of an abstract class
There was a problem hiding this comment.
Don't think that'd be worth it.. seems to require quite a bit of extra code:
https://groups.google.com/forum/#!topic/haxelang/WzeI-N1XbIg
https://gist.github.com/andyli/5011520
There was a problem hiding this comment.
That's why I did not do that on purpose
There was a problem hiding this comment.
👍
You might wanna remove the comment about macros then.
|
Seems like the naming of "base classes" is a bit of a mess right now:
I feel like we should consistently put |
Masadow
changed the title
|
Imo, at the end, it's better for completion, however, at the start, it looks more natural. I'm not even sure if it's good for completion, since you are most likely to never import one of the base classes within your game ? |
| totalTiles = _data.length; | ||
| } | ||
|
|
||
| private function doAutoTile(DrawIndex : Int, CollideIndex : Int):Void |
There was a problem hiding this comment.
The spaces before and after the : don't fit the coding style of flixel. Also not a huge fan of function names that start with do().
There was a problem hiding this comment.
What name would you reckon ?
There was a problem hiding this comment.
I think applyAutoTiling() and applyCustomRemapping() might be more fitting.
|
I agree, putting the base first sounds more natural. Completion probably doesn't matter much. |
|
Indeed, but it is a typo in HaxeFlixel context isn't it ? |
|
I guess you could see it that way. :) |
|
Anything that helps speed up workflow and pave the way for more features is a good change in my book. Agreed Base should go at the end. This looks good to me 👍 |
|
To the End ? Cause we both agreed it would be better right after Flx |
|
Lol (missread), I don't really mind it either way, I just like consistency :) |
|
Cool, also think after 'Flx' is better. |
|
Are there any more concerns about this pull request? Will it get merged? |
|
@Gama11 surely is in charge of it |
|
I'd prefer if these function names were adjusted: #1101 (comment) But yeah, this is absolutely going to be merged. |
|
My bad, forgot to push ... has been fixed while ago |
FlxTilemap refactoring : Separation of logic/display
|
There are certainly maintenance and extensibility issues in FlxTilemap. FlxTilemap has so much in common with FlxSprite. If graphics were separated into a component it would be far easier for a lot of the graphics code to be shared. FlxTilemap would shrink massively. Furthermore, the autotiling stuff could be separated into an extensible FlxAutoTile class, with an general interface for mapping one tilemap into another. This way it would be possible to make extensions for multitile objects, multiple tileset autotiling, autotiling with adjacent tilemaps, non-linear randomly distributed tiles etc., all while maintaining the same interface, and without ever having to touch the Tilemap class. With such changes, I think there would be no need for both BaseTilemap and Tilemap. |
In order to implement easier our isometric tilemap (because the tilemap class change too often, we lost a lot of time merging it again all the time). I submit this refactor.
It fixes a bug in overlapsAt method
It adds the following new methods
public function getTileIndexByCoords(Coord:FlxPoint):Intpublic function getTileCoordsByIndex(Index:Int, Midpoint:Bool = true):FlxPoint