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

Organize FlxG and FlxU a tad better #142

Open
IQAndreas opened this issue Jan 13, 2013 · 32 comments
Open

Organize FlxG and FlxU a tad better #142

IQAndreas opened this issue Jan 13, 2013 · 32 comments
Assignees
Milestone

Comments

@IQAndreas
Copy link
Member

This would break much existing code, but I feel FlxG and FlxU are needlessly cluttered and could be split into smaller classes.

The full list of proposed changes can be found in the following forum thread:
http://forums.flixel.org/index.php/topic,5675

The actual changes can be found in the following branch:
https://github.com/IQAndreas-testprojects/flixel/commits/dev_organizing_flixel

I had already merged in several other fixes into that branch; only the last 4 commits are related to this issue.

@Gama11
Copy link

Gama11 commented Jan 13, 2013

Sounds like a great idea to me, the 4 "sections" the functions should be split into in your opinion make a lot of sense, too!

I'm thinking it should be a lot easier for newcomers to grasp what each of the classes actually does, while right now, it's more like pretty much everything is in FlxG.

One thing that's debateable though is the naming / the actual organization. For example, if FlxMath should be abbreviated with FlxM, or if vcr and effects should be plugins. In my mind, plugins are something which is not part of vanilla flixel, something you add yourself afterwards to enhance it, like the powertools. But I'm not that familiar with the whole plugin system, and the only thing I ever saw using it are the powertools, so correct me if I'm wrong.

Either way, FlxVCR instead of FlxG.vcr and FlxEffects instead of FlxG.effects would make more sense to me.

@IQAndreas
Copy link
Member Author

Most of that logic was my attempt to make Flixel more modular. I'm still not a fan of how much of Flixel is static. It may be helpful for beginners, but just seems "dirty" to me.

Either way, FlxVCR instead of FlxG.vcr and FlxEffects instead of FlxG.effects would make more sense to me.

The problem is, some of those classes need access to variables, such as the current FlxGame instance. That instance is always available in FlxG, but if the logic is moved to a separate class, that information is no longer accessible.

My solution was to create the separate FlxVCR as an instance, which is instantiated with the required parameter, the FlxGame.

static public var vcr:FlxVCR;
//...
FlxG.vcr = new FlxVCR(_game);

This static variable can then be replaced with a new instance if the _game variable is reset or changed to a different game.

In my mind, plugins are something which is not part of vanilla flixel, something you add yourself afterwards to enhance it, like the powertools.

There were two classes created by AdamAtomic already set up as plugins:
https://github.com/FlixelCommunity/flixel/tree/dev/org/flixel/plugin

I see anything in the plugin package as "not actually necessary" or as you said, "something you add yourself afterwards to enhance it". In my mind, the "camera flash" effect isn't actually part of "vanilla flixel", it's just a "bonus effect" created by Adam which people can use. It should be a plugin, but it's more of an "official plugin".

@moly
Copy link
Member

moly commented Jan 14, 2013

I've had similar thoughts about making flixel more modular, and turning some of the default functionality into plugins. The problem I see is that a lot of the stuff (debugging, replays, etc) is tightly intertwined with FlxG/FlxGame and would take a lot of refactoring to fully extract out. You would also need to improve the plugin system a lot, adding the ability to intercept key presses amongst other things.

@Dovyski
Copy link
Member

Dovyski commented Jan 14, 2013

I liked the structure you suggested, @IQAndreas . I would still polish some edges (such as using FlxMath instead of FlxM), but the overall idea is great.

I really like that organization, but let me share some of my experience with Flixel. When I started using it, it was simple to understand the meaning (and methods) of FlxG. While working, every time I typed FlxG. I learned something new just by reading the methods in the auto-complete list. I think that is priceless.

I agree with @moly that we should turn some features into plugins (looking at you, replays!). It will be a huge work, but it will definitely make Flixel better. For me, a new and overhauled plugin system seems the way to go.

@Dovyski
Copy link
Member

Dovyski commented Jan 15, 2013

There is an open improvement request regarding Flixel plugin system. I've added some ideas about a new plugin system there.

@IQAndreas
Copy link
Member Author

FlxMath

I agree with @Dovyski, it's a better idea to be clear and name this FlxMath than save three characters by calling it FlxM; the full name is also clearer to beginners.

I would like to move the following "pure math" functions from FlxU to FlxMath (see this commit):

  • abs(Value:Number):Number
  • floor(Value:Number):Number
  • ceil(Value:Number):Number
  • round(Value:Number):Number
  • min(Number1:Number,Number2:Number):Number
  • max(Number1:Number,Number2:Number):Number
  • bound(Value:Number,Min:Number,Max:Number):Number
  • computeVelocity(Velocity:Number, Acceleration:Number=0, Drag:Number=0, Max:Number=10000):Number

The following functions require FlxPoint. I feel they should also be moved to FlxMath, but perhaps someone instead feels they should be static (or even instance) methods of FlxPoint or in their own FlxVectorMath:

  • rotatePoint(X:Number, Y:Number, PivotX:Number, PivotY:Number, Angle:Number,Point:FlxPoint=null):FlxPoint
  • getAngle(Point1:FlxPoint, Point2:FlxPoint):Number
  • getDistance(Point1:FlxPoint,Point2:FlxPoint):Number

There is only one method for random number generation. At the moment I feel it also belongs in FlxMath, but perhaps one day a class used along the lines of new FlxRandom(seed) with methods such as getUInt() or getRandomElement(array) can be used instead.

  • srand(Seed:Number):Number

@IQAndreas
Copy link
Member Author

FlxMath from Flixel Power Tools

The following "pure math" functions are added in FlxMath from FlixelPowerTools:

  • atan2(y:Number, x:Number):Number
  • sinCosGenerator(length:uint, sinAmplitude:Number = 1.0, cosAmplitude:Number = 1.0, frequency:Number = 1.0):Array
  • getSinTable():Array
  • getCosTable():Array
  • sqrt(val:Number):Number
  • maxAdd(value:int, amount:int, max:int):int
  • wrapValue(value:int, amount:int, max:int):int
  • isOdd(n:Number):Boolean
  • isEven(n:Number):Boolean
  • wrapAngle(angle:Number):int
  • angleLimit(angle:int, min:int, max:int):int
  • asDegrees(radians:Number):Number
  • asRadians(degrees:Number):Number

"Pure math" vector functions - see earlier thoughts on FlxVector

  • vectorLength(dx:Number, dy:Number):Number
  • dotProduct(ax:Number, ay:Number, bx:Number, by:Number):Number

And it also contains the following random functions (see earlier thoughts about FlxRandom):

  • miniRand():int
  • rand(min:Number = NaN, max:Number = NaN, excludes:Array = null):int
  • randFloat(min:Number = NaN, max:Number = NaN):Number
  • chanceRoll(chance:uint = 50):Boolean
  • randomSign():Number

There are a few more methods in the Flixel Power Tools' FlxMath, but they are a bit inconsistent:

Does not use either FlxPoint or FlxRect

  • pointInCoordinates(pointX:int, pointY:int, rectX:int, rectY:int, rectWidth:int, rectHeight:int):Boolean

Uses FlxRect - Shall these be moved to things like FlxRect#contains() and FlxRect#containsMouse(useWorldCoords:Boolean)? (instance methods, not static).

  • pointInFlxRect(pointX:int, pointY:int, rect:FlxRect):Boolean
  • mouseInFlxRect(useWorldCoords:Boolean, rect:FlxRect):Boolean

Uses the AS3 Rectangle, but not Point or FlxPoint - we already have a pointInFlxRect function. If we want to make Flixel port well and not require public methods to contain AS3's Rectangle, shall we remove this function? After all, Rectangle already has a contains() method which does just this.

  • pointInRectangle(pointX:int, pointY:int, rect:Rectangle):Boolean

@ghost ghost assigned IQAndreas Nov 29, 2013
@IQAndreas
Copy link
Member Author

Today I noticed there is also a FlxVelocity with the appropriate static math functions in the Power Tools:

@IQAndreas
Copy link
Member Author

FlxCoreUtils

With only three methods, I feel the FlxCoreUtils from the Flixel Power Tools should be imported like this.

Move copyObject() to FlxU.

Move gameContainer and mouseIndex either as static methods of FlxG, or as instance members of FlxGame.

@Dovyski
Copy link
Member

Dovyski commented Nov 30, 2013

About FlxU and FlxMath

I would like to move the following "pure math" functions from FlxU to FlxMath (see this commit):
abs(Value:Number):Number
floor(Value:Number):Number
ceil(Value:Number):Number
round(Value:Number):Number
min(Number1:Number,Number2:Number):Number
max(Number1:Number,Number2:Number):Number
bound(Value:Number,Min:Number,Max:Number):Number
computeVelocity(Velocity:Number, Acceleration:Number=0, Drag:Number=0, Max:Number=10000):Number

Ok to me. The only thing I don't like is the name bound(). It think clamp() would be a much better name.

The following functions require FlxPoint. I feel they should also be moved to FlxMath, but perhaps someone instead feels they should be static (or even instance) methods of FlxPoint or in their own FlxVectorMath:

rotatePoint(X:Number, Y:Number, PivotX:Number, PivotY:Number, Angle:Number,Point:FlxPoint=null):FlxPoint
getAngle(Point1:FlxPoint, Point2:FlxPoint):Number
getDistance(Point1:FlxPoint,Point2:FlxPoint):Number

They should be static methods of FlxPoint, in my opinion. All those methods act upon a point (or points), so they behave much more like "services", which fits quite well as static methods (e.g. String.format() in Java).

There is only one method for random number generation. At the moment I feel it also belongs in FlxMath, but perhaps one day a class used along the lines of new FlxRandom(seed) with methods such as getUInt() or getRandomElement(array) can be used instead.

srand(Seed:Number):Number

I agree it should belong to FlxMath. I don't see a huge gain for creating something like FlxRandom right now.

@Gama11
Copy link

Gama11 commented Nov 30, 2013

I guess it might also be worth investigating whether the duplicates of the standard-math-functions like abs(), floor(), ceil() make sense / are actually faster than their Math-counterparts. I find it hard to believe that the flash-implementation is that bad, but there had to be a reason for creating these in the first place.

In HaxeFlixel, those actually turned out to be slower and have been removed.

@Dovyski
Copy link
Member

Dovyski commented Nov 30, 2013

About FlxMath from Flixel Power Tools

The following "pure math" functions are added in [FlxMath from FlixelPowerTools (https://github.com/photonstorm/Flixel-Power-Tools/blob/master/src/org/flixel/plugin/photonstorm/FlxMath.as):

  • atan2(y:Number, x:Number):Number
  • sinCosGenerator(length:uint, sinAmplitude:Number = 1.0, cosAmplitude:Number = 1.0, frequency:Number = 1.0):Array
  • getSinTable():Array
  • getCosTable():Array
  • sqrt(val:Number):Number
  • maxAdd(value:int, amount:int, max:int):int
  • wrapValue(value:int, amount:int, max:int):int
  • isOdd(n:Number):Boolean
  • isEven(n:Number):Boolean
  • wrapAngle(angle:Number):int
  • angleLimit(angle:int, min:int, max:int):int
  • asDegrees(radians:Number):Number
  • asRadians(degrees:Number):Number

Those are the functions I think we should merge into FlxMath:

  • atan2(y:Number, x:Number):Number
  • sqrt(val:Number):Number
  • isOdd(n:Number):Boolean
  • isEven(n:Number):Boolean
  • asDegrees(radians:Number):Number
  • asRadians(degrees:Number):Number

Excluding the sin/cos functions (which I don't think vanilla Flixel should have), the remaining functions are just "hiding" tasks that are easy to solve, e.g. add a value then bound()/clamp() the result. For me simplicity is key, so I've rather keep things simple and easy to remember/discover than adding lots of methods.

"Pure math" vector functions - see earlier thoughts on FlxVector

  • vectorLength(dx:Number, dy:Number):Number
    • dotProduct(ax:Number, ay:Number, bx:Number, by:Number):Number

They should be part of FlxPoint or a new class, FlxVector. I created an issue (#188) about this a while ago. I would like them to be part of FlxPoint instead of a new FlxVector class.

And it also contains the following random functions (see earlier thoughts about FlxRandom):

  • miniRand():int
  • rand(min:Number = NaN, max:Number = NaN, excludes:Array = null):int
  • randFloat(min:Number = NaN, max:Number = NaN):Number
  • chanceRoll(chance:uint = 50):Boolean
  • randomSign():Number

The only method I would like to merge is randomSign():Number, the rest can be easily implemented with FlxMath.random(). e.g. chanceRoll() is the same as FlxMath.random() <= chance. Again I think simplicity is key.

There are a few more methods in the Flixel Power Tools' FlxMath, but they are a bit inconsistent:

Does not use either FlxPoint or FlxRect

  • pointInCoordinates(pointX:int, pointY:int, rectX:int, rectY:int, rectWidth:int, rectHeight:int):Boolean

Uses FlxRect - Shall these be moved to things like FlxRect#contains() and FlxRect#containsMouse(useWorldCoords:Boolean)? (instance methods, not static).

  • pointInFlxRect(pointX:int, pointY:int, rect:FlxRect):Boolean
  • mouseInFlxRect(useWorldCoords:Boolean, rect:FlxRect):Boolean

I guess they all should be implemented as FlxRect#contains(), including the containsMouse() one (since the mouse itself is a FlxPoint).

Uses the AS3 Rectangle, but not Point or FlxPoint - we already have a pointInFlxRect function. If we want to > make Flixel port well and not require public methods to contain AS3's Rectangle, shall we remove this function? > After all, Rectangle already has a contains() method which does just this.

  • pointInRectangle(pointX:int, pointY:int, rect:Rectangle):Boolean

I agree, we should remove this function.

@Dovyski
Copy link
Member

Dovyski commented Dec 1, 2013

@Gama11 I don't like that duplication too, but I guess a reason why Adam did this was to make Flixel "standalone". As a consequence it's easier to port it to another platform, such as iOS (you don't have to mimic any Flash package, only port Flixel code).

I think we can make Flixel math function way better than their math-counterparts by using these tricks. For example, FlxMath.abs() would be 2500% faster than Math.abs().

@IQAndreas
Copy link
Member Author

I don't like that duplication too, but I guess a reason why Adam did this was to make Flixel "standalone". As a consequence it's easier to port it to another platform

Although it probably wouldn't work in most AS3 libraries, in Flixel I feel it was actually a good idea (this is the type of library that ports well to other languages, as we have seen). (but of course, if the functions are slower, maybe they aren't such a good idea)

I think we can make Flixel math function way better than their math-counterparts by using these tricks.

Good idea, I created an issue for that so we don't forget about it: #196

Ok to me. The only thing I don't like is the name bound(). It think clamp() would be a much better name.

I personally think bound() is clearer, but it may be a good idea to look at other libraries and see which name they use.

@IQAndreas
Copy link
Member Author

Ka Wing's idea of using a spreadsheet was a rather good one, so I put these two up which may help make it clear which methods everyone agrees should be moved, and which ones are still up for debate:

  • FlxMath - bold is math functions which still need to be moved, and blue is for functions related to random
  • FlxColor

Does anyone disagree with the FlxG.vcr and FlxG.effects changes, or are they good to go?

@Dovyski
Copy link
Member

Dovyski commented Dec 11, 2013

I personally think bound() is clearer, but it may be a good idea to look at other libraries and see which name they use.

Yeah, that would be great. I suggested clamp() because it's widely used with CG, e.g. in shaders.

@Dovyski
Copy link
Member

Dovyski commented Dec 11, 2013

Does anyone disagree with the FlxG.vcr and FlxG.effects changes, or are they good to go?

I got lost in so much discussions :) Are you saying the use of FlxG.effects pretty much like FlxG.effects.someEffect() instead of FlxEffec.someEffect()?

@IQAndreas
Copy link
Member Author

I got lost in so much discussions :) Are you saying the use of FlxG.effects pretty much like FlxG.effects.someEffect() instead of FlxEffec.someEffect()?

Almost. Right now, it's FlxG.flash(), and I'm recommending changing that to FlxG.effects.flash().

See:

@IQAndreas
Copy link
Member Author

I'm not getting comments showing up in any of the Google Spreadsheet. Are they removed after a while, or am I doing something wrong?

Also, there are several edits by anonymous users, but I can't see what they actually changed. Is there any way to view only the diff like in Git or Wikipedia edits?

@WingEraser
Copy link

The tab at the bottom you'll see a white number on a black background next to the label. The number says the total comments on the sheet. Click on it and you'll get all comments.

You can go back to previous changes by File > See revision history.

@IQAndreas
Copy link
Member Author

The tab at the bottom you'll see a white number on a black background next to the label. The number says the total comments on the sheet. Click on it and you'll get all comments.

I can't see any of that. I checked it again in Chrome, and it appeared; must be a FireFox thing.

@WingEraser
Copy link

I can see it in Firefox (v26, Windows 7 x64) and I'm not logged in.

@Dovyski
Copy link
Member

Dovyski commented Jan 26, 2014

Almost. Right now, it's FlxG.flash(), and I'm recommending changing that to FlxG.effects.flash().

I like it!

@Gama11
Copy link

Gama11 commented Jan 26, 2014

I suggest you guys take a look at the front ends in FlxG of HaxeFlixel, I'm very happy with that system.

I don't think the three camera effects there are are really worth it to create a new class for. In HaxeFlixel we have the CameraFrontEnd for that.

@Dovyski
Copy link
Member

Dovyski commented Mar 6, 2014

Any updates on this issue, @IQAndreas ?

@IQAndreas
Copy link
Member Author

I'm looking at the spreadsheet, and thinking about in which order would be the "most efficient" when applying all the changes.

Would you guys be alright with adding FlxRandom in this release? In that case, does anyone mind if I add FlxRandom first, and move the remaining FlxMath functions (this issue) after?

@Dovyski
Copy link
Member

Dovyski commented May 2, 2014

No problem, go ahead and add FlxRandom.

@IQAndreas
Copy link
Member Author

I suggest you guys take a look at the front ends in FlxG of HaxeFlixel, I'm very happy with that system.

Does the front end system for FlxG sound like a good approach to everyone? I feel it tidies things up very nicely, and would prefer this organizational system over the one I originally proposed.

@Dovyski
Copy link
Member

Dovyski commented May 17, 2014

Yeah, it like that approach. HaxeFlixel has several front ends, some we can keep, some we might drop. I would be happy with the following:

  • FlxG.inputs: keep only if input methods are moved to this front end (e.g. keys);
  • FlxG.log: keep (probably as an "alias" to a debugger method);
  • FlxG.watch: keep (same as FlxG.log);
  • FlxG.console: keep (same as FlxG.log);
  • FlxG.vcr: Drop (I'm planning to move all VCR code to a plugin, as I mentioned);
  • FlxG.debugger: keep;
  • FlxG.bitmap: drop (we don`t need that now);
  • FlxG.cameras: keep (shake() and flash() will be moved to this front end, right?);
  • FlxG.plugins: keep;
  • FlxG.sound: keep;

I think I messed up the concept of front end here because I labelled some entries as aliases. If I did mess things up, I need some more info :)

@Gama11
Copy link

Gama11 commented May 17, 2014

Not sure you'd want to move the inputs themselves into FlxG.inputs, that creates some very long paths - for example FlxG.inputs.keys.justPressed("SomeKey");

The inputs frontend is very similar to the plugins one, in that it simply manages the different registered inputs.

@Dovyski
Copy link
Member

Dovyski commented May 17, 2014

Ok, now I got it. I remember you guys handle inputs separately e.g. FlxG.gamepads and FlxG.android. Does HaxeFlixel register the available input methods and add them to FlxG.inputs?

@Gama11
Copy link

Gama11 commented May 17, 2014

Yeah.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants