Prevent setting a FlxSprite to an invalid frame

[IQAndreas]

Fixes #13

Prevents an "out of range" frame number from being set either by an
animation, or by manually setting the frame property.

Adds new getter/setter numFrames where user can manually specify how
many frames an animation has (in case some frame are empty).

To go hand in hand with this is the read-only property maxFrames which
calculates how many frames can fit on that sprite sheet. NOTE: it rounds
up, so sprites may "hang off the edge" if the sheet is the wrong
dimensions. This may be fixed in the future depending on how other
applications handle "overhanging frames".

[IQAndreas]

The code compiles just fine. I haven't tested it, but I uploaded the SWC to the sandbox if anyone has some code to test it on (it's in the lib-dev folder).

[IQAndreas]

I am a little confused here. The calculation you made here, isn't it the same as the following (code from resetHelpers())?

Odd, I didn't notice that property (and if I'm not mistaken, it's not even used anywhere in the code, which is probably why I missed it.)

The code used to calculate it is only correct if the tiles fit perfectly on the sprite sheet, but will be bugged if there are any pixels "hanging over the edge". Using FlxU.ceil() ensures that all frames are accounted for.

But you are right, there is no need to re-calculate maxFrames every time, but should instead be calculated once then stored. I'll add this to the code.

That's because all FlxButtons are setting frame to 1 and 2, but _numFrames is 0, so they are trying to set a frame that is technically out of bounds.

Actually, that's due to a typo of mine. Originally I was defaulting _numFrames to -1 until I realized it was an unsigned integer. Rather than change it to a signed one, I realized that you could never have zero frames, and let 0 be the default value. However, I forgot to change the other place which I check for this default value:

return (_numFrames == -1) ? maxFrames : _numFrames; 

[IQAndreas]

There we go, all solved! I tested it in the sandbox and it works properly, but take a look and make sure I didn't miss anything. There is one warning thrown, but the good news is, it is supposed to throw that warning, since that is the "bugged" sprite which we have been talking about.

Thanks for adding the sprite sheet Fernando, for some reason I thought it was going to require more work than that, and figured it would be easier finding a demo project that uses sprite sheets anyway.

Also, I finally found where that frames property was being used! :) 4560193

[IQAndreas]

The only problem now is that the code doesn't actually say which class throws the warning, so you pretty much have to guess and debug using a bunch of trace() statements. However, I have great plans on how to solve this in a future version by changing around the way logging works in Flixel!

[Dovyski]

Everything seems to be working now, great job! I'm just nitpicking now, but shouldn't we keep the property frames? It is public so anyone can be using that (not just Flixel itself). If that property was protected or private, there would be no problem at all renaming it to numFrames (which is a far better name).

Since the current release should not break anything, I think it's prudent to keep frames for now and rename it to numFrames in the next release. What do you think?

[IQAndreas]

I'm just nitpicking now, but shouldn't we keep the property frames?

I assumed that public variable was only used internally by Flixel (especially since it is technically read-only, setting it does nothing), I didn't think of the fact that developers might use it.

I'm also not a fan of that variable naming since the FlxAnim also has a public variable named frames, but is an Array, rather than a uint. I find numFrames is more descriptive and avoids the ambiguity.

What if I create a public getter named frames that just points to numFrames? That way, it doesn't break any existing code, and paves the way for a better future.

[Dovyski]

I totally agree with you.

What if I create a public getter named frames that just points to numFrames? That way, it doesn't break any existing code, and paves the way for a better future.

That's perfect! We can add a @deprecated tag or something similar in the getter's comment, so developers are advised to use numFrames instead of frames.

[IQAndreas]

We can add a @deprecated tag or something similar in the getter's comment

Do you know of any guides or at least an example and recommended use of the tag?

The only thing I'm finding on it after some lazy (ie, only looking on the first page of results) Google searching is the following, which only describes how to use the Flex Metadata tags, not the comment tags: Deprecated metadata tag

Should both be used, or is that unnecessary?

Does Flash Professional still have difficulties compiling Flex Metadata unless you download and specify the Flex SDK (which most beginners won't have done, and I know has caused confusion for a few)? I seem to remember that in CS3 it couldn't compile with the metadata at all, or at least not the Embed tags, but it may have changed in the current version.

[Dovyski]

I never used a Flex deprecated metadata tag, so I don't know if Flash Professional still have problems with it. I think we can keep it simple and just add a comment tag, such as:

/**
 * Description
 *
 * @deprecated This property is deprecated. Use <code>numFrames</code> instead.
 */
public function get frames(): uint {
}

[IQAndreas]

I think we can keep it simple and just add a comment tag

That simple? Excellent!

The commit has been added to this pull request.

[Dovyski]

Simplicity is key! :D Your change is perfect. The code is good and working, we can merge it.