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

Fix issue 206 #217

Closed Dovyski closed 9 years ago

Dovyski commented 10 years ago

The interactive debug plugin is usable. Even though some code comments and docs are missing, I created this pull request to receive some feedback about the code and the plugin usability. I want to write some more docs before this gets merged.

If you are going to test it, here's a mini howto:

I've made a few tests using the game Mode, by Adam.

Fix: https://github.com/FlixelCommunity/flixel/issues/206

Dovyski commented 10 years ago

I would like some special feedback on two topics.

_The overlays property_:

Commit https://github.com/Dovyski/flixel/commit/4f1462261f25bee4f9d71bbcc0b05f67db070595 adds the FlxG.debugger.overlays property, which is a flash.display.Sprite. As I explained in the commmit log message, I think an overlays property is much better than making FlxDebugger extend Sprite.

I also thought about not providing a getter to access the overlays property, requiring developers to use a specific API like FlxG.debugger.addOverlay() and FlxG.debugger.removeOverlay().

_Mixing Flixel and Flash graphic classes_:

Adam coded all debug overlays using flash.display.Sprite (which I've followed when implemented the interactive debugger). I think that's a good idea, because it makes the debug code decoupled from "Flixel land". The downside is that we cannot use Flixel components as overlays, such as FlxButton.

It also brings some problems, specially regarding OOP, as discussed here.

What do you guys think of those topics?

greysondn commented 10 years ago

Of course you know my opinion on the second. But I do feel somewhat compelled to ask: can we not do something about that mess? I struggle to see why unifying it would be disadvantageous, though as I understand it verges on a rewrite of decently-large parts of the codebase.

I guess my pondering is: Is this a "it is impossible" issue? or "it would take a lot of work" issue? or a "that would be poor architecture" issue? Or some other fourth thing I can't think of as at least a possibility?

To me, the lack of unification across the codebase could be considered bad, but I struggle to wade through "pure" as3 a lot of the time.

IQAndreas commented 10 years ago

I'm not well versed on the Flixel Debugger, and I haven't followed the progress closely, so pardon me if my advice is deeply flawed.

I also thought about not providing a getter to access the overlays property, requiring developers to use a specific API like FlxG.debugger.addOverlay() and FlxG.debugger.removeOverlay().

That's probably safer, so that developers don't accidentally remove someone else's overlay. Don't forget to include options of changing which "layer", or order, you want to add the overlay to or move it to.

The downside is that we cannot use Flixel components as overlays, such as FlxButton.

I agree, I think the debug overlay should be a flash.display.Sprite. What if we add some sort of display object that can contain and render FlxSprite objects, some sort of "bridge" between the two types of dispaly lists? This can either be a container that can hold multiple Flixel objects, or a display object that can hold a single Flixel item and just work as a "proxy".

greysondn commented 10 years ago

What if we add some sort of display object that can contain and render FlxSprite objects, some sort of "bridge" between the two types of dispaly lists? This can either be a container that can hold multiple Flixel objects, or a display object that can hold a single Flixel item and just work as a "proxy".

In the background, it wouldn't be hard to stitch them together with an interface (based on expected behavior), once you star going that direction.

Dovyski commented 10 years ago

Thanks for the feedback, guys!

I guess my pondering is: Is this a "it is impossible" issue? or "it would take a lot of work" issue? or a "that would be poor architecture" issue? Or some other fourth thing I can't think of as at least a possibility?

Yeah, I'm afraid this is a "it would take a lot of work" issue. Porting the current debug overlays code to "Flixel land" would require the creation of several UI elements that are already implemented in the flash. package. E.g. input texfields, selectable text (in the console), draggable windows.

As @greysondn pointed out, unifying the code base is a good thing, I just don't see much gain in doing so. Our effort porting all that code could be used somewhere else, like implementing new (and awesome) features :)

I agree, I think the debug overlay should be a flash.display.Sprite. What if we add some sort of display object that can contain and render FlxSprite objects, some sort of "bridge" between the two types of dispaly lists?

We could do that, but it will require a significant amount of work too. I think we could keep things simple by just adding some signals to the debugger in the future.

Developers will be able to choose between flash.display and "Flixel land". Signals could be used to control overlays visibility/funcionality, for instance.

greysondn commented 10 years ago

If a blueprint can be drawn for the refactor, then I could probably do it a hunk at a time. A gradual process.

I've just resumed classes after three years out. I would be happy to clean up the mess I myself had to address, but I have no sense of where to even begin.

Edit: I just remembered the Moly fixes to quadtrees! OH NO!

Dovyski commented 9 years ago

Ok, guys, I've made the changes pointed here. FlxG.debugger no longer has a overlays property, developers must use the addOverlay*() and removeOverlay*() API.

As a side (and good) effect, I've moved all VCR code to FlxReplay. Finally it is completely contained as a plugin, so the record/pause/stop/etc buttons at the top of the screen will only appear if the FlxReplay plugin is active. I made it disabled by default, so if anyone wants to use the record functionality, it must be enabled by calling FlxG.addPlugin(new FlxReplay()) during the game's creation.

If you think we should keep FlxReplay active by default, I have no problems with it.