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

Implement scaling for FlxTilemapExt #384

Merged
merged 10 commits into from
Apr 17, 2023

Conversation

DalekCraft2
Copy link
Contributor

@DalekCraft2 DalekCraft2 commented Apr 13, 2023

FlxTilemapExt works with FlxTilemap's scale 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.

@Geokureli
Copy link
Member

Geokureli commented Apr 13, 2023

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

@Geokureli
Copy link
Member

Geokureli commented Apr 13, 2023

Just really want to restate what I said in HaxeFlixel/flixel#2763:

If you make a change to some code to fix a bug or add a feature, you can change the formatting, comments, documentation of that method, if the file is small and isolated, you might want to make similar changes to the other methods.

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

@DalekCraft2
Copy link
Contributor Author

DalekCraft2 commented Apr 13, 2023

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.

@Geokureli
Copy link
Member

Geokureli commented Apr 14, 2023

I could undo the latter two commits, if you'd like.

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.

@DalekCraft2
Copy link
Contributor Author

Yeah, it was Quench. It should be pretty easy to make a test, though.

Copy link
Member

@Geokureli Geokureli left a 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

flixel/addons/tile/FlxTilemapExt.hx Show resolved Hide resolved
flixel/addons/tile/FlxTilemapExt.hx Outdated Show resolved Hide resolved
flixel/addons/tile/FlxTilemapExt.hx Outdated Show resolved Hide resolved
flixel/addons/tile/FlxTilemapExt.hx Outdated Show resolved Hide resolved
flixel/addons/tile/FlxTilemapExt.hx Outdated Show resolved Hide resolved
@DalekCraft2
Copy link
Contributor Author

DalekCraft2 commented Apr 14, 2023

For everything what you questioned, I copied the code from FlxTilemap because I figured it probably had better and more up-to-date code than FlxTilemapExt.

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.

@Geokureli
Copy link
Member

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

@Geokureli
Copy link
Member

I'm done making changes, anything you want to add?

@DalekCraft2
Copy link
Contributor Author

Where exactly is FlxObject.separate() called in the overlapsWithCallback() function? I don't see any usage of it, and I don't know why it would be mentioned if it is not used.

@Geokureli
Copy link
Member

Where exactly is FlxObject.separate() called in the overlapsWithCallback() function? I don't see any usage of it, and I don't know why it would be mentioned if it is not used.

excellent point, lol. 1 more edit coming

Geokureli added a commit to Geokureli/flixel that referenced this pull request Apr 14, 2023
@Geokureli
Copy link
Member

Geokureli commented Apr 17, 2023

sorry, I was sick for these last few days. any more comments or concerns, @DalekCraft2 ?

@DalekCraft2
Copy link
Contributor Author

None right now. I can always bring them up in some other PR if I think of any.

@Geokureli Geokureli merged commit 65b32d6 into HaxeFlixel:dev Apr 17, 2023
@Geokureli
Copy link
Member

Geokureli commented Apr 17, 2023

Thanks!

Also note of similar changes being made to FlxTilemap in https://github.com/HaxeFlixel/flixel/pull/2734/files

Geokureli added a commit to HaxeFlixel/flixel that referenced this pull request Jun 19, 2023
* 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
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