-
Notifications
You must be signed in to change notification settings - Fork 149
Implement scaling for FlxTilemapExt
#384
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
Conversation
FYI, for future changes, know that having the functional changes mixed in with the formatting changes makes this much more tedious to review, either keep them in separate commits or preferably in separate PRs. This is a small 20-50 line, high-priority change, made into a 250 line change for things I consider low priority, I could have probably tested the functional changes and had this approved in a half hour, now it'll likely not get done for a couple of days |
Just really want to restate what I said in HaxeFlixel/flixel#2763:
Just because you make a change in a file, that does not give you license to completely rewrite and/or reformat the entire file. Only edit the methods you are ALREADY changing for functional reasons if you want to reformat an entire file, make a separate PR for that change. I would not consider FlxTilemap.hx to be small. I meant like 4 or 5 methods, max |
I could undo the latter two commits, if you'd like. I won't do it unless you say I should, though, because you may be already looking through the changes right now, and I don't want to make you confused and make you need to start over. |
Nah Same question as flxweapons, do you have a small test file i can use? I assume its quench, so ill make my own test when i have a chance. |
Yeah, it was Quench. It should be pretty easy to make a test, though. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hold of on changes I'm gonna try some things, I have questions, tho
For everything what you questioned, I copied the code from If it can be simplified, though, that'd be great; it would mean that second TODO comment at the top of the file could be removed. |
Cool, I'll just make similar changes to FlxTilemap in another PR. next time please explain changes like this in the PR description otherwise I'l assume they're some style preference you have |
I'm done making changes, anything you want to add? |
Where exactly is |
excellent point, lol. 1 more edit coming |
sorry, I was sick for these last few days. any more comments or concerns, @DalekCraft2 ? |
None right now. I can always bring them up in some other PR if I think of any. |
Thanks! Also note of similar changes being made to FlxTilemap in https://github.com/HaxeFlixel/flixel/pull/2734/files |
* add FlxTypedTilemap * similar changes to HaxeFlixel/flixel-addons#384 * remove dead code * improve docs * make FlxBaseTilemap an abstract class * create bitmap instead of loading one * revert abstract classes * doc formatting + var case
FlxTilemapExt
works withFlxTilemap
'sscale
field properly now.I have not made many changes to
FlxTilemapExt
's doc comments, but I probably will make more changes to them if I am given permission.