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

Add 'x', 'y' and 'alpha' properties to FlxGroup #124

Open Gama11 opened 11 years ago

Gama11 commented 11 years ago

For some reason, x and y properties were removed in the latest flixel versions, which is really incovenient. An alpha property would be handy as well, so that FlxGroup basically can be treated like a FlxSprite. This can be achieved really easily by adding a few getter and setter functions at the end of FlxGroup:

private var _x:int = 0;
private var _y:int = 0;
private var _alpha:Number = 1;

public function set x(nx:int):void
{
    var offset:int = nx - _x;

    for each (var object:* in members) {
        object.x += offset;
    }

    _x = nx;
}

public function get x():int {return _x}

public function set y(ny:int):void
{
    var offset:int = ny - _y;

    for each (var object:* in members) {
        object.y += offset;
    }

    _y = ny;
}

public function get y():int { return _y }

public function set alpha(n:Number):void 
{
    _alpha = n;
    if (_alpha > 1) _alpha = 1;
    else if (_alpha < 0) _alpha = 0;

    for each (var object:* in members) {
        object.alpha = _alpha;
    }
}

public function get alpha():Number { return _alpha }
moly commented 11 years ago

Can you not just use setAll for this?

group.setAll("alpha", 0.5);
IQAndreas commented 11 years ago

Of course, as for the x and y properties, if any of the "children" in the FlxGroup are then set to an absolute value (rather than incremented) it will be "offset" from it's siblings.

Flash takes into account the location and alpha of all its parents when rendering, but this can take a heavy performance hit (since you need to iterate through every parent until you get to the top). The question is, is it worth it?

Gama11 commented 11 years ago

Can you not just use setAll for this?

setAll is nice and everything, it'd work too, but what you can't do is stuff like

group.alpha += 0.1; 

Flash takes into account the location and alpha of all its parents when rendering, but this can take a heavy performance hit (since you need to iterate through every parent until you get to the top). The question is, is it worth it? It just seems much more convenient if groups behave like sprites, especially for x and y.

I'm not sure about performance.. I'm using this and I haven't really noticed any performance hits. Either way, if an alpha property is in fact too performance-heavy, at least add x and y .

There definitely is a need for x and y properties in FlxGroup. Ppl in the forums are often asking how to create a sprite that is made up of mutiple objects - FlxGroupwould obviously the way to go, but because of the lack of the FlxSprite's typical properties like x, y and alpha, it's neither intuitive nor convenient. That's why someone went ahead and created FlxGroupXY, too, which I forgot to give credit to in my initial post.

IQAndreas commented 11 years ago

IQAndreas: I'm not sure about performance. I'm using this and I haven't really noticed any performance hits.

You are correct, this version isn't performance heavy at all.

However, it will give unexpected problems with the following code (sorry, I got carried away with the illustration):

var horseRed:FlxSprite = new horseRed();
var horseBlue:FlxSprite = new horseBlue();

var racetrack:FlxGroup = new FlxGroup();
racetrack.add(horseRed);
racetrack.add(horseBlue);

// All horses start at the beginning
horseRed.x = 0;
horseBlue.x = 0;

// Now imagine each separate segment happening every new frame

// ---- frame 1 ----
// horseRed is in the lead
horseRed.x = 10;

// ---- frame 2 ----
// blue passes the red horse
horseBlue.x = 20;

// ---- (between frame 2 and frame 3) ----
// The user just opened a menu! (this likely happens in an event listener)
// Move the race track a bit to the right to make room for the menu.
function onMenuButtonClicked(event:Event)
{
    racetrack.x = 50;
    trace(horseRed.x, horseBlue.x); // Traces 60, 70
}

// ---- frame 3 ----
// Now the red horse moves forward and passes the blue horse
horseRed.x = 30;

// But wait! Blue is still in the lead!!
trace(horseRed.x, horseBlue.x); // Traces 30, 70

Do you see where the bug creeps in?

This sort of problem doesn't happen with Flash display object; when an item is rendered, it adds together all of the x and y properties of its parent, and its parent's parent, etc. However, this solution can get quite performance heavy!

Perhaps we could add a FlxContainer or something similar which does have this capability. Though, users of the class are cautioned that this it may affect performance. I'm going to set this as an idea for a future release, and in the meantime, perhaps you can create this class as a plugin or extension to Flixel? I'm sure many people would appreciate it.

Gama11 commented 11 years ago

Do you see where the bug creeps in?

Yeah, I can see the problem. Your example seems a little .. unlikely, I guess, though. You'd be much more likely to set a velocity.x for the horses or maybe call horse.x += 10; - doing it like this by setting it to an absolute value seems like bad practice to me. The benefits seem to overweigh the risk of getting a bug like this. Using a group'y x and y values is optional, anyway. And if you are actually using those properties, you're likely to know how they behave. The other solution seems really convenient, it's a bummer that it's so performance-heavy. I'd imagine it probably wouldn't be a huge issue in most cases, unless you're doing a lot of nesting.

IQAndreas commented 11 years ago

Your example seems a little .. unlikely, I guess, though.

Yeah, you are right, but how about this example? Let's say you have "loading screen" with a character that walks from left to right.

// in update()
character.x = (loader.bytesLoaded / loader.bytesTotal) * progressBarWidth;

If he is within a FlxGroup "progress bar" of sorts, and the progress bar is moved around across the screen, the character's location becomes "flawed".

You could change the code to increment the player's location, but it would make the code more complex (you would need to keep track of his previous location etc). In addition, not being able to set a child's absolute location would be confusing for developers that are used to Flash's way of handling display objects.

I'd imagine it probably wouldn't be a huge issue in most cases, unless you're doing a lot of nesting.

Good point. Come to think of it, since they are treated more like "fancy arrays" than "parents and children", they aren't often nested very deep.

Still, I'm a fan of moving that logic to a separate class, but I'll leave this issue open and see where it goes.

Gama11 commented 11 years ago

If he is within a FlxGroup "progress bar" of sorts, and the progress bar is moved around across the screen, the character's location becomes "flawed".

Now, that's a good example right there. You're wrong about the complexity of the fix though, something like this would be enough:

character.x = ((loader.bytesLoaded / loader.bytesTotal) * progressBarWidth) + progressBar.x;

So, essentially, you're doing the second approach logic manually here. It's obviously also limited to 1 deep nesting, further nesting would increase the length of the line quite a bit. But no need to keep track of previous locations whatsoever. Also, the fact alone that it's relatively hard to come up with a scenario where this issue would occur shows how rare it'd be. And even if it happens, there is an easy fix to it.

Still, I gotta admit, the second "children-parent"-approach is much nicer. Nesting often isn't very deep in flixel, the deepest nesting I got going on in my current project is level 3 I think (a sprite inside 3 other nested groups). When you're working with Adobe Flash and MovieClips, you tend to nest about everything, especially GUI.

Still, I'm a fan of moving that logic to a separate class, but I'll leave this issue open and see where it goes.

So yeah, I was about to create such a seperate class that'd make this work by myself and wondered how the actual implementation could look like. It's not quite as simple as you'd think and requires changes to more than just a single class. Let's think this through:

Say we have the simplest case possible, level 1 nesting with one FlxSprite inside of a FlxGroup. The intuitive approach would be to create a new FlxGroupXY or whatever you wanna call it as it lacks those two properties. You'd then have to update the "actual" position of the sprite inside of these getter / setters, so you'd add child.x + this.x - and put it where? Yeah, right, you gotta put it in child.x again because a sprite's x value represents the absolute position on the screen. Doing so would make you lose the actual x property of the sprite, or its offset from the group, if you will. Now, you could create an array of coordinates inside a FlxGroup for all of its members, then add those values to this.x and put it in sprite.x. In fact, that'd work fine, as long as you don't change the sprite's x somewhere in your code.

So let's create a new FlxObject, let's call it FlxObjectXY. It needs a parent variable to access the groups x value. So you'd also need an edited FlxGroup again to set that property whenever a FlxObject is added to the group. Now you need two more variables which basically replace x and y, I guess you could call them actualX and actualY. Offset is not really an option, because that name is already in use. What you'd have to do after that is turn x into a getter and setter, make it read-only where the getter function does something like this: return actualX + parent.x;. I assume it'd work fine this way, but imagine how counter-intuitive all this would be. When changing a sprite's x, you can no longer say sprite.x = 10, it'd have to be sprite.actualX = 10. I guess you could try to make both things work by having a getter function just like I just mentioned, but have the setter function of x manipulate actualX. That's really ugly and confusing though, as it causes major problems if you do things like incremeneting or decremeting: sprite.x += 5.

Esentially, this requires much more substantial changes to flixel than it seems at first glance.

Dovyski commented 11 years ago

I really like the idea, but add it to FlxGroup will makes things obscure and hard to predict. If FlxGroup has x and y properties, then when I add a new sprite to it, is the sprite's position updated? What if I want to change a group member position and keep all other members intact?

In my opinion the best approach is to create a new class (say FlxContainer like @IQAndreas read from my mind) and there we add x, y and alpha properties that propagate to all children. Flash does that, but it uses a transformation matrix.

Gama11 commented 11 years ago

In my opinion the best approach is to create a new class (say FlxContainer like @IQAndreas read from my mind) and there we add x, y and alpha properties that propagate to all children. Flash does that, but it uses a transformation matrix.

Hm, yeah... Have you read my post? I admit it's a bit of a text wall. I don't see a way to make this work (nicely) unless you use two custom classes, one for FlxSprite and one for FlxGroup. Which doesn't seem all that convenient to use.

Dovyski commented 11 years ago

Yes, I read :) I agree that creating actualX and all that stuff doesn't seem convenient.

The way Flixel is structured now, it's not easy to create something like FlxContainer and children with offset values. There is no such thing as relative position in Flixel, every object has an absolute position.

Right now the best we can do without re-writing lots of methods is propagate values instead of making them relative. That's my idea of a different class (FlxContainer).

IQAndreas commented 11 years ago

I agree that creating actualX and all that stuff doesn't seem convenient.

A simple globalToLocal() type function would do the trick, then the children wouldn't need to subclass anything particular, and could stay as a FlxSprite. But any parent property would need to be tracked in a different way.