Alpha changing incorrectly

[FlixelCommunityBot]

Issue #219 by: mouseas

I have the following code in a FlxSprite:

override public function update():void {
    // Here is my +1 error, the last time this branch is true, it sets the frame one past the end of the spritesheet.
    if (frame < frames) {  
        frame = frame + 1;
        trace(frame);
    }

    lifespan += FlxG.elapsed;

    if (lifespan > 3) { // 3 seconds before starting fade
        alpha = (1 - ((lifespan - 3) / 15)); // <--- this line...fade slowly over 15 seconds
    }

    if (alpha < 0.01) {
        State.s.remove(this, true);
    }
    super.update();  
}

Instead of taking 15 seconds to fade out, the sprite fades out in less than a second. I took a quick poke into the alpha-related code in FlxSprite, and it looks like alpha is adjusted in discrete increments. Problem is, if the amount the alpha is supposed to change is too small, it still drops it one increment. The end result? The FlxSprite is invisible when its alpha value is actually at around 0.95 - 95% opaque, i.e. ever-so-slightly see-through.

Edit: I was able to isolate the problem quite a bit. It was trying to fade on a non-existant frame (thanks to a +1 error on my part), ie the frame it was currently on was a copy of the last frame. I tried using makeGraphic() and the same bug happened. Basically, this bug occurs when the graphic is unique and there's no sprite/spritesheet to reference back to. If the frame is a valid image on a spritesheet, this bug does not occur.

Expected behavior: Sprite fades slowly, based on alpha value.

Actual behavior: If sprite graphic is unique, it fades quickly. The change in actual transparency is not in line with the alpha value.

To reproduce:

1. Create a sprite and either (a) use makeGraphic() and possibly some draw methods to create a unique graphic, or (b) load an animated spritesheet and set the frame outside the range of existing frames.
2. Adjust the alpha of the sprite slowly over many frames. Display the numeric alpha value somewhere for comparison; the alpha value should be much higher than the sprite's visibility.
3. For comparison, use an existing frame or the default image, and adjust the alpha slowly as before. This time the alpha value matches the sprite's visibility.

[FlixelCommunityBot]

Comment by: mouseas

Did a little more poking. My guess is that when the bug does occur, the graphic has alpha applied, and the alpha-applied version is assigned to the graphic's place. Thus each frame the alpha is adjusted, the graphic becomes that much more transparent.

[moly]

I couldn't reproduce this by following the users steps. Has anyone else tried yet?

[Dovyski]

Yes, I tried and I could reproduce the problem. You have to reproduce the exactly same scenario described by the author.

I've spent some hours trying to narrow down the problem but had no success. It seems to be some really obscure behavior...

[IQAndreas]

@Dovyski If you still have it, could you throw up the sample code for this issue in the FlixelSandbox?

[Dovyski]

Sure! I've just pushed the testing code to FlixelSandbox (it is called TestFlxSpriteAlpha). The class SpriteTestBug is the buggy one.

[IQAndreas]

Still trying to find the cause of this bug, but I removed some unnecessary code from the test (and also updated the code so we don't have to modify Main.as in order to change tests) and came up with two results:

• Resetting the test does not reset the second sprite's "displayed" alpha. This is VERY likely related to the bug!
• The trace output of mSpriteTestBug.alpha will always be identical to the first sprite's, even if the second one "displays" a completely different alpha value.

[IQAndreas]

Found it. :)

In case anyone is curious, it all came from this little function in FlxSprite (unnecessary code removed):

protected function calcFrame():void
{
    _flashRect.x = _curIndex*frameWidth; // <---(1)
    _flashRect.y = 0;

    //Handle sprite sheets
    if(_flashRect.x >= _pixels.width) // <---(2)
    {
        _flashRect.x = uint(_flashRect.x/widthHelper)*frameHeight;
        _flashRect.y %= widthHelper;
    }

    //Update display bitmap
    framePixels.copyPixels(_pixels,_flashRect,_flashPointZero); // <---(3)
    _flashRect.x = _flashRect.y = 0;
    if(_colorTransform != null)
        framePixels.colorTransform(_flashRect,_colorTransform); // <---(4)
}

In our example, the sprite was sized 50x50, with the "expected" sprite being on frame 0, and the "bugged" sprite on frame 1. Let's also assume that the alpha is always set to 0.8.

• _curIndex is the current frame, with the "expected" sprite being on frame 0, and the "bugged" sprite on frame 1.
• frameWidth in this case will always be 50
• _pixels is the "raw" BitmapData, without any color or alpha modifiers applied to it
• framePixels is the "modified" BitmapData, with the alpha applied. This is what later gets copied to the camera
• _flashRect is the rectangle that decides what area to copy, in this function, only the x and y properties are being changed depending on which frame you are playing (it "moves around" the sprite sheet to switch frames)

By the point you reach (1), the location for the "expected" sprite's frame is set to {x:0, y:0}, and the "bugged" sprite is {x:50, y:0}.

By section (2), Flixel realizes the "bugged" sprite's rectangle is too far right to be on the image, so it moves it down to what it thinks is "the next line" of the sprite sheet. New values (only for the "bugged" sprite) {x:0, y:50}. Little does Flixel know, this new location is ALSO outside the bounds of the sprite sheet.

Once it gets to section (3), two different things happen:

For the "expected" sprite: the old BitmapData gets copied over with entirely new data, retrieved from _pixels. Not one of the old pixels shine through (since _flashRect is the same size as framePixels). Then, the transform gets applied (4), rendering the alpha at 0.8.

For the "bugged" sprite: Notice now that framePixels is at {x:0, y:50}, which is outside of the bounds of the image. Flash assumes, "well, nothing to copy here", and basically ignores the copyPixels command, leaving the old BitmapData where it was. Then it gets transformed to an alpha of 0.8 (4). The next time calcFrame is called, the same thing happens, and this old BitmapData is still lying there. The old BitmapData then gets an alpha value of 0.8 applied to it (4), meaning 0.8*0.8=0.64. This transform keeps happening every frame until the alpha value approaches 0, which it eventually does (and does rather quickly if 'alpha' is being tweened down to 0 as well, which it was in @mouseas code).

[IQAndreas]

As for a solution, all this can be avoided by preventing the user from ever setting the current frame number above the maximum number of frames, either directly (through the frame property) or by playing an animation where the frame number is too high (haven't checked if there is any code that prevents the second action).

I'm taking a break on this issue for a while, so if anyone wants to take the information I have gathered and propose a solution, feel free to do so. I'm going to loaf about on the couch for a few hours.

[Dovyski]

Nice catch! I can take a look at this issue and implement your solution, @IQAndreas. If you agree, just assign the issue to me and I will work on it as soon as I have some free time.

[IQAndreas]

Sorry, I didn't realize it had gone an entire week and I still hadn't updated this issue.

Actually, there is no reason to read up on the details on why the bug occurs. As you correctly deduced in the FlixelSandbox, it happens because the current frame number is set above the maximum number of frames; that is all that needs to be fixed.

One "gotcha" to keep in mind, _curFrame is used only for animations and says where inside the frames array you are. The "frame number" that gets set outside of bounds is named _curIndex and is the only one we need to worry about. This variable indicates where on the sprite sheet the "current frame" is.

Where should the "frame number" check be done?

There are two places an invalid frame number can be set:

• With the public frame property
• In an animation (for instance, the sample "idle" animation can go from frame 0 to frame 1, and will cause this error if there is no frame 1) in updateAnimation()
• (The frame number can also be set in updateAnimation() for rotated sprites, but this operation is always "safe" and won't ever go outside of bounds as far as I can see)

Should this error (or better yet, a warning) be thrown:

• When the person sets the frame/adds the animation?
• OR every time calcFrame() is run and the current frame?

The latter will produce an error message every frame (not ideal).

However, the former will produce other problems if they add the animation before loading the graphics, or if they change the graphics any time after the animations have been added, and the new sprite sheet doesn't contain enough frames. Is this a valid concern?

How many frames are there actually?

Finally, there is a slight inconsistency in how to calculate the amount of available frames. In randomFrame(), it is assumed that all of the sprites are laid out on the sprite sheet in the following manner:

[0][1][2][3][4][5]

But in calcFrame(), if Flixel realizes you are outside of the bounds on the x axis, it will automatically switch to the next row, allowing for sprite sheets to look like this:

[0][1][2][3]
[4][5][6][7]

Should there be an additional check to make sure the frame index isn't outside of the y axis? Should it then loop around to frame 0, or should it just stop and whine?

So once again, I guess I'm waiting for my "moment of inspiration" to magically clarify my mind and help me know what to do. But I seem to be "groggy minded" still, so feel free to tackle it yourself. If you only want to tackle the questions, I will gladly tackle the code as long as I know what to do.

[Dovyski]

I've been thinking about this issue and I guess there is a simple solution. The whole problems is related to the frame index, so we should fix/sanitize the value before assigning it.

Why would Flixel show a warning when a wrong frame value is used? To make the developer aware of the problem, so he can fix it. Why doesn't Flixel fix the problem already? If there are 4 frames, then the index should range from 0...3. Anything out of range means the animation should start from 0 again.

How about that?

[IQAndreas]

I forgot to finish this issue, thanks for resurfacing it.

The whole problems is related to the frame index, so we should fix/sanitize the value before assigning it.

That works. But, which of the two methods do we use to calculate how many frames there are? (see under the heading "How many frames are there actually?" in my previous comment)

If we use the first method (requiring all the frames to be on the same row) we know exactly when the frame index is out of bounds.

If we use the second method (i.e. assuming frames can wrap around to the next line), we have no way of telling if frames [5] and [6] are empty, which means if the frame index is set too high, the FlxSprite would just turn invisible those frames, with no warning message.

Why would Flixel show a warning when a wrong frame value is used? To make the developer aware of the problem, so he can fix it.

That's a good point, displaying the same warning several times in a row in the output isn't really that big of deal then. And you are right, these questions only become relevant if the developer is making a mistake in the first place; all we really are doing is writing the warning system to assist in debugging.

Sorry for over-thinking things, let me know if I get out of hand.

[Dovyski]

The over-thinking is good :)

First about the output message, I guess I was not clear enough when I made my point. I think Flixel should not output any error message when an incorrect frame is used. Flixel should analyze the value and sanitize/fix it silently.

Finally, there is a slight inconsistency in how to calculate the amount of available frames

Yeah, I noticed that now. randomFrame() assumes all frames are in the same row. That's a bug we should probably fix or investigate a little further.

Checking the code I think the only property we should care about is _curIndex. Our sanity check should be placed here (in FlxSprite):

public function set frame(Frame:uint):void
{
    _curAnim = null;
    _curIndex = Frame;
    dirty = true;
}

That's the only "entrance" point where someone can mess up with the animations.

[IQAndreas]

I think Flixel should not output any error message when an incorrect frame is used. Flixel should analyze the value and sanitize/fix it silently.

Keeping in mind, since Flixel only alerts you of warnings if the console is enabled and open, we don't run the risk of bothering "users/players" with the error message.

Imagine you are creating a game and for no reason one of your sprites suddenly resets to the first frame (frame 0). Assuming you even notice it, first you have to spend several minutes debugging if it's your code to find out where the problem is: is it in the tile sheet, the animation code, or a bug in Flixel?

Wouldn't you rather have an error message tell you "Warning: Trying to set the EnemyGoomba frame 7, but it only contains 8 frames." immediately when the problem happens?

[Dovyski]

Yeah I agree with you. A warning will not harm anybody, but I keep on thinking if we should fix the mistake on the fly too.

Using your example, if you try to set a frame above the upper range, Flixel should set the frame as the last one available. It will not cause the sprite to reset its animation. The same happens if a negative value is used in a frame (-2, for instance, would be converted to 0, the first frame).

[IQAndreas]

Using your example, if you try to set a frame above the upper range, Flixel should set the frame as the last one available.

Perfect!

So, the only issue that remains is, how many frames are there in a tilemap?

[Dovyski]

Yes, that's the only thing we must figure out. It will fix the frame setter and should also be used to fix randomFrame().

[IQAndreas]

How about this, we add a public getter/setter named numFrames. it will automatically set itself based on the size of the sprite sheet, however, developers can set this value manually (which they would if their sprite sheets were not "entirely filled", something like this (with [ ] representing empty tiles):

[0][1][2][3]
[4][5][ ][ ]

• If they try to set the frame number above this new numFrames value, they get a warning, and the last frame of the sprite is displayed. If they set the frame number to a negative value, frame 0 is displayed, again with a warning.
• If they try to set numFrames so high that some frames don't even fit on the tile sheet, it will give the developers a stern warning, but will continue anyway. I'm hesitant to just keep this "alpha bug" in, and I would prefer if the default Flixel sprite was displayed instead so they know which FlxSprite has it's bounds set too high. It's either that, or we render the bugged sprite as entirely transparent, which helps no one.

Does that sound like a good solution? Any other thoughts?

[Dovyski]

I like the idea of adding numFrames, but I think it should be ready-only. If we allow developers to set numFrames we are replicating the frame problem, there will be two properties with "boundary" issues.

numFrames should be calculated when the sprite art is loaded. Using your example, numFrames would be 8, even though there are two empty frames (frames 6 and 7) . Flixel is not able to tell if a frame is empty because it's lacking content or because it's empty on purpose.

Since numFrames contains the number of frames available, it can be used on the frame setter.

[IQAndreas]

I like the idea of adding numFrames, but I think it should be ready-only.

But that would still make functions like randomFrame() worthless for situations like the example where some of the frames are empty.

numFrames should be calculated when the sprite art is loaded. Using your example, numFrames would be 8, even though there are two empty frames (frames 6 and 7) .

How about instead making that read-only property named maxFrames? The read/write property numFrames will default to this value, but can be changed to any value as long as it's lower than maxFrames. This will ensure that developers can tell Flixel how many frames an animation has, but if they try to set the number too high (or too low, there has to be at least 1 frame), they get an error message.

[Dovyski]

How about instead making that read-only property named maxFrames?

Perfect!

[IQAndreas]

I was working on this when I noticed another potential bug (albeit, a rare case one).

If the width of your sprite sheet doesn't evenly divide by the width of each tile (say, the sprite sheet is 25 pixels wide, and each tile is 10 pixels wide), Flixel will currently assume that frame 2 (zero based) will be located at x:20, and will only render "half of" a sprite.

I'm going to change the way Flixel handles such cases and require that a frame be entirely inside of the sprite sheet. I don't see any reason to throw an error or warning, the "overhanging pixels" will just be silently ignored. If anyone has a good argument against this, I will revert back to Flixel's old way.

[Dovyski]

I guess Flixel calculates tiles that way to maintain the uniformity between the game code and tile editors such as DAME (or any image editor with grids enabled). If you change that behavior developers might face inconsistencies beteween what the tile editor is showing and what Flixel is actually showing.

We should test that change with DAME and other editor to check if everything is ok.

[Dovyski]

Issue has been fixed in df5dfc6.