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

Fix a regression with FlxSprite's _maxFrames calculation. #181

Merged

Conversation

Dovyski
Copy link
Member

@Dovyski Dovyski commented Oct 14, 2013

The current approach assumed perfectly filled spritesheets rows. It used ceil() to adjust any floating value, which resulted in a blank column at the right of some graphics.

I've noticed that testing with a game that uses particles. Some particles had that blank column. Removing the ceil (which was Adam's original approach) fixed the problem.

The current approach assumed perfectly filled spritesheets rows. It used ceil() to adjust any floating value, which resulted in a blank column at the left of some graphics.

I've noticed that testing with a game that uses particles. Some particles had that blank column. Removing the ceil (which was Adam's original approach) fixed the problem.
@Dovyski Dovyski mentioned this pull request Oct 14, 2013
5 tasks
@IQAndreas
Copy link
Member

We discussed this one before:

The problem with not using any sort of rounding is that the value of maxFrames will be completely off.

Say, if your sprites are 10px each, the sprite sheet 45px wide and 40px tall, using Math.ceil() with maxFrames will say there are 18 frames, when really there can only be either 16 or 20 frames depending on if you count the "overhang" as a valid frame or not.

Personally, I would prefer using Math.floor() and just completely ignore any overhanging pixels. I also mentioned outputting a warning if this was the case, but forgot to implement that.

@Dovyski
Copy link
Member Author

Dovyski commented Oct 15, 2013

I remember our discussion. For some reason I had in my mind the idea that we used Math.ceil() as a test. If everything worked, we would keep it, otherwise it would be changed.

Using Math.floor() is the same as my current fix: I don't floor() the results, but they will be automatically casted to uint during the variable assignment.

I think we should merge this pull request, since it will prevent lots of "blank" or "half-blank" frames all over the place after people start using Flixel Community (my game included).

@IQAndreas
Copy link
Member

they will be automatically casted to uint during the variable assignment.

Oh, right. Yes, that would do the trick.

Although, would it be possible to explicitly run it through FlxU.floor() just to make it clear that those extra pixels are actually being trimmed off? It shouldn't affect performance as _maxFrames is only re-calculated when the the sprite sheet is set.

Do you want to add that warning if the sprite sheet is a few pixels too large as well, or push it up to a future version?

@Dovyski
Copy link
Member Author

Dovyski commented Oct 18, 2013

I've just added the FlxU.floor() to make the trimming explicit. About the warning, I think we shoud push it up to the future version.

@IQAndreas
Copy link
Member

The code looks flawless (there's not much one can botch by accident in two lines ;) ).

Could test this in one of your games, and if it fixes the problem, go ahead and merge this?

@Dovyski
Copy link
Member Author

Dovyski commented Oct 20, 2013

haha, indeed there's not much to mess up in two lines :) I've tested and everything looks good.

Dovyski added a commit that referenced this pull request Oct 20, 2013
…ames

Fix a regression with FlxSprite's _maxFrames calculation.
@Dovyski Dovyski merged commit c1827dc into FlixelCommunity:dev Oct 20, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants