FlixelCommunity / flixel

Community fork of Adam “Atomic” Saltsman's popular game engine Flixel. Distilled from a variety of Flash games he worked on over the last couple years, including Gravity Hook, Fathom and Canabalt, its primary function is to provide some useful base classes that you can extend to make your own game objects.
http://flixelcommunity.org/
Other
84 stars 17 forks source link

FlxSprite.drawLine() Shows Incorrect Behavior When Full Transparency Set In Color #170

Closed greysondn closed 11 years ago

greysondn commented 11 years ago

A simple class:

package net.darkglass.sacchar.state.scores
{
    import org.flixel.FlxSprite;

    /**
     * Class to arbitrarily create boxes in flixel.
     *
     * TODO: Write Docs
     *
     * @author greysondn
     */
    public class FlxBox extends FlxSprite
    {

        public function FlxBox(X:Number, Y:Number, width:Number, height:Number, lineWidth:uint, fillColor:uint, lineColor:uint)
        {
            // let parent handle basic construction and placement
            super(X, Y);

            // create blank box graphic
            makeGraphic(width, height);

            // flood fill
            this.fill(fillColor);

            // Create outline with given dimensions
            for (var i:int = 0; i < lineWidth; ++i)
            {
                // top and bottom
                drawLine(0, (0 + i), width, (0 + i), lineColor);
                drawLine(0, (height - i), width, (height - i), lineColor);

                // left and right
                drawLine((0 + i), 0, (0 + i), height, lineColor);
                drawLine((width - i), 0, (width - i), height, lineColor);
            }
        }

    }

}

Attempted construction (highest level of main FlxState):

    //                                       AARRGGBB    AARRGGBB
    this.add(new FlxBox(32, 32, 64, 64, 8, 0x00FF0000, 0x0000FF00));

A snapshot of this result is available here: https://lh4.googleusercontent.com/-qvQjI6WOWb8/Uf3Mbzj_i3I/AAAAAAAACHQ/-o8QnFji-XE/s800/flixel%2520issue.png

I may find time to fix it myself if it's not too complex, but I wanted to go ahead and post it for confirmation. (If someone else fixes it, I won't be the least bit upset.)

Update

I originally thought this was all values alpha. It turns out that it does behave somewhat for all values /but/ full transparency. This is still either undocumented or incorrect behavior; however, the title of this issue and actual filing have been changed to reflect this. (Try calling it with various values of alpha to test; the corners should be more opaque than the edges due to how it's drawn so don't panic when you see that if you're using the class provided in this report ;) )

Dovyski commented 11 years ago

I've confirmed the bug here.

drawLine() seems to work with all alpha values but 00 (full transparency). Inspecting FlxSprite code, the mistake is explicit (some lines were omitted for the sake of understanding):

public function drawLine(StartX:Number,StartY:Number,EndX:Number,EndY:Number,Color:uint,Thickness:uint=1):void
{
        var alphaComponent:Number = Number((Color >> 24) & 0xFF) / 255;
        if(alphaComponent <= 0) // <---------------- HERE!
            alphaComponent = 1;
    }

If alphaComponent is less or equal to zero, it's set to 1. It's really odd why Adam used <= instead of <. The only reason I could think of this is some sort of visual "warning" for when you are drawing a fully transparent line.

However I believe it's is a bug and should be fixed. Any thoughts?

IQAndreas commented 11 years ago

I agree, it looks like a typo, but I would probably have set alphaComponent to 0 instead.

Is there any possible value of Color that would even cause it to alphaComponent to ever be negative? (I must confess that I sometimes do checks like that "just in case". I'm not sure why, it just makes me feel safe.)

The "warning thought" is a good idea, but I would just let developers set the alpha to 0, just to avoid errors if they do things like "tween" the alpha value by redrawing the line at lower and lower alpha values.

Dovyski commented 11 years ago

I would implement it the very same way you describe, @IQAndreas (setting alphaComponent to 0 to allow things like tween). As far as I can tell, alphaComponent is always greater or equal to zero, so I think the <=0 test is there "just in case" (I do thinks like that very often, better safe than sorry).

I've just sent a pull request to fix this issue. If everything looks ok, I can merge it right away.

IQAndreas commented 11 years ago

Fixed by commit a731d166b927eb2ffaff5805f35c42ca90385c88

(edit: Woah, GitHub automatically closed this issue when I merged the pull request. Is this feature new, or did I just not notice it before?)

Dovyski commented 11 years ago

It's a new feature: a merged pull request automatically closes the issue it fixes :)

greysondn commented 11 years ago

okay then :3 Sorry, the miss had a minor operation and was busy but glad to see I'm not insane. I wonder why, ultimately, he didn't just set it to zero before..? Hm....