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

FlxObject: allow configuring debug rect colors #1847

Merged

Conversation

@starry-abyss
Copy link
Contributor

starry-abyss commented May 10, 2016

So I want to cut the decorative (tile.allowCollisions == FlxObject.NONE) tiles' rects from debug draw: http://forum.haxeflixel.com/topic/8/flxtilemap-debug-draw/2
Desirable by default :-)

@@ -115,6 +115,8 @@ class FlxTilemap extends FlxBaseTilemap<FlxTile>
private var _scaledTileHeight:Float = 0;

#if FLX_DEBUG
public var debugHideDecorativeTiles:Bool = true;

This comment has been minimized.

Copy link
@Gama11

Gama11 May 10, 2016

Member

I don't like the idea of introducing new terminology for this (what is considered a "decorative" tile? seems a bit ambigious). In general, I would prefer a more flexible solution (setting the color per debug tile type like you originally proposed sounds good, that way you'd have full control, similar to debugBoundingBoxColor in FlxObject. Which makes me wonder what that does for FlxTilemap, if anything...).

@starry-abyss

This comment has been minimized.

Copy link
Contributor Author

starry-abyss commented May 10, 2016

Ok, I'll work further on it. Can you provide old terminology maybe? This is from docs:

The behavior of tiles in FlxTilemap is slightly different:

Blue for allowCollisions == FlxObject.NONE
Green for allowCollisions == FlxObject.ANY
Pink for other values of allowCollisions

Should I maybe make a function var like Int -> FlxColor ? E.g. collision flags to color converter, and refactor the original code as the default function.
@Gama11 what do you think about hiding blue ones by default?

@starry-abyss

This comment has been minimized.

Copy link
Contributor Author

starry-abyss commented May 10, 2016

As a variant I could also draw lines instead of rects to indicate the sides of enabled collisions :-) That would simplify three-color scenario to two-color. One could be changed by current debugBoundingBoxColor, other - by adding second color parameter.

@Gama11

This comment has been minimized.

Copy link
Member

Gama11 commented May 10, 2016

I had something like Map<FlxDebugTileType, FlxColor> in mind, but that requires setting up an entire enum. Using allowCollisions is a lot more elegant. Not sure whether a map or a function is better. Are there any particular advantages to using a function?

I'm not opposed to hiding them by default. I'll have to think about it a bit.

@Gama11

This comment has been minimized.

Copy link
Member

Gama11 commented May 10, 2016

Hm, that sounds interesting as well. I'd like to see how that looks! :)

@starry-abyss

This comment has been minimized.

Copy link
Contributor Author

starry-abyss commented May 10, 2016

Can't say about particular function advantages...

I tried different coloring approaches, this is the least confusing so far (with green lines, but also with pink rects):

http://i.imgur.com/bsJGVoC.png

But I still have user-coloring not implemented yet :D

@starry-abyss

This comment has been minimized.

Copy link
Contributor Author

starry-abyss commented May 10, 2016

But it draws collider lines inside while they seem to have more meaning outside of the tile (in the opposite direction) :/

@Gama11

This comment has been minimized.

Copy link
Member

Gama11 commented May 10, 2016

Drawing the "collider lines" on the outer rectangle doesn't work well?

@starry-abyss

This comment has been minimized.

Copy link
Contributor Author

starry-abyss commented May 10, 2016

Did the outer version, it feels natural to me:

http://i.imgur.com/rSMy8xX.png

@Gama11

This comment has been minimized.

Copy link
Member

Gama11 commented May 10, 2016

I don't like how that makes the debug draw bigger than the tile bounds. What I meant was drawing the collision lines on top of the bounding rectangle.

@starry-abyss

This comment has been minimized.

Copy link
Contributor Author

starry-abyss commented May 11, 2016

But you don't see the direction that way - to which of neighbouring tiles the edge belongs среда, 11 мая 2016г., 01:12 +0300 от Gama11 < notifications@github.com> :

I don't like how that makes the debug draw bigger than the tile bounds. What I meant was drawing the collision lines on top of the bounding rectangle.

You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub

@starry-abyss

This comment has been minimized.

Copy link
Contributor Author

starry-abyss commented May 11, 2016

So that's the benefit of using customizable function: you can provide your the renderer for one tile without copy-pasting whole drawDebugOnCamera, and this function can be theoretically reused for standalone FlxObjects. среда, 11 мая 2016г., 01:12 +0300 от Gama11 < notifications@github.com> :

I don't like how that makes the debug draw bigger than the tile bounds. What I meant was drawing the collision lines on top of the bounding rectangle.

You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub

@starry-abyss

This comment has been minimized.

Copy link
Contributor Author

starry-abyss commented May 11, 2016

@Gama11 so I post the version you requested, that I shot during the experimenting: http://i.imgur.com/NjGNuZZ.png

And now I have a common drawDebugBoundingBox subfunction between FlxObject and FlxTilemap. (except renderBlit mode, which I didn't touch yet)
The only difference is in FlxObject typedef-variable, which is set in constructor:

// FlxObject
debugColorScheme = { solid: FlxColor.RED, highlighted: FlxColor.GREEN, notSolid: FlxColor.BLUE };
// FlxTilemap
debugColorScheme = { solid: FlxColor.GREEN, highlighted: FlxColor.PINK, notSolid: null };

Also I can override now just the small drawDebugBoundingBox to provide fancy collision edges. Not sure if I need the callback function I proposed here, postponing it for now :-)

@starry-abyss

This comment has been minimized.

Copy link
Contributor Author

starry-abyss commented May 12, 2016

Now you can change colors for tiles in Flash with limitations:

  1. Apparently it's slow
  2. Colors are only updated when you change the whole debugColorScheme, and not only a field of it

Also I think about customization of alpha values too.

Can I implement allowCollisions vis only for non-RenderBlit? (is it called RenderTiles when I'm on neko?)
Or should I leave it for user/addons?

@starry-abyss

This comment has been minimized.

Copy link
Contributor Author

starry-abyss commented May 15, 2016

I hacked Mode demo a bit here to test (compile in Debug mode): https://github.com/starry-abyss/flixel-demos/tree/tilemap_debug_color_test/Platformers/Mode
Press E to randomly change color scheme for tilemap debug draw.
FPS drops a bit in neko, not sure if it's my fault, any recommended way of finding leaks, profiling in Haxe?

@Gama11 Gama11 added the FlxDebugger label Aug 21, 2016
@starry-abyss

This comment has been minimized.

Copy link
Contributor Author

starry-abyss commented Sep 11, 2016

@Gama11, sure, did it work?

@Gama11

This comment has been minimized.

Copy link
Member

Gama11 commented Sep 11, 2016

Looks like it did, thanks. :)

@Gama11

This comment has been minimized.

Copy link
Member

Gama11 commented Sep 12, 2016

I checked your hacked Mode demo - it seems like after pressing E, you still need to move enough for the tilemap to be redrawn for color changes to show? Didn't you implement the setter for this?

@starry-abyss

This comment has been minimized.

Copy link
Contributor Author

starry-abyss commented Sep 14, 2016

I changed the key for switching sprites' color scheme in the demo to E too. It's applied immediately, but tilemap's is not (on neko both are OK).

@Beeblerox

This comment has been minimized.

Copy link
Member

Beeblerox commented Oct 2, 2016

So, what should be done to make decision about this PR? What's missing?

@Gama11

This comment has been minimized.

Copy link
Member

Gama11 commented Oct 2, 2016

There are some things I don't like about the API:

  • I don't like the name highlighted in DebugColorScheme. I can't really think of something better right now though
  • I dislike that you can change the colors, but they won't be updated if you don't trigger the setter (reassign the structure)

Also, with blitting, the colors don't update right away even if you do trigger the setter. See #1847 (comment).

@starry-abyss

This comment has been minimized.

Copy link
Contributor Author

starry-abyss commented Oct 2, 2016

For some reason colors are updated on Flash only when you move camera

/**
* Can be overriden for RenderBlit mode to re-render stuff with new colors
*/
public function setDebugColorScheme(debugColorScheme:DebugColorScheme)

This comment has been minimized.

Copy link
@Beeblerox

Beeblerox Oct 2, 2016

Member

why not change this method to debugColorScheme setter?

public var debugColorScheme(default, set):DebugColorScheme;

private function set_debugColorScheme(value:DebugColorScheme):DebugColorScheme
{
    return debugColorScheme = value;
}

This comment has been minimized.

Copy link
@starry-abyss

starry-abyss Oct 3, 2016

Author Contributor

Because as Gama11 said it triggers on assigning the structure, but doesn't on field value change

This comment has been minimized.

Copy link
@Beeblerox

Beeblerox Oct 3, 2016

Member

i don't quite understand this statement. Could you explain?

I've made this change in my fork and it works like i expect it to work. Maybe there is some misunderstanding?

This comment has been minimized.

Copy link
@starry-abyss

starry-abyss Oct 3, 2016

Author Contributor

What you propose is the original way I've done it.
On native targets it works fine because it just gets the colors every frame and uses them.
On Flash (+ FlxTilemap) there is some update stuff to be done on each change.

This comment has been minimized.

Copy link
@starry-abyss

starry-abyss Oct 3, 2016

Author Contributor

Apparently _tileMap.debugColorScheme.solid = Std.random(0xFFFFFF); doesn't trigger the update

This comment has been minimized.

Copy link
@starry-abyss

starry-abyss Oct 3, 2016

Author Contributor

I agree with @Gama11 that it's a bit confusing this way :-/ Maybe he has better idea?

This comment has been minimized.

Copy link
@Gama11

Gama11 Oct 3, 2016

Member

Ony way to work around this is to simply have a separate class field for each color.

This comment has been minimized.

Copy link
@Beeblerox

Beeblerox Oct 3, 2016

Member

yes, it would be a simple solution.

This comment has been minimized.

Copy link
@Beeblerox

Beeblerox Oct 3, 2016

Member

@starry-abyss so do you agree to add 3 debug color properties instead of sebugColorScheme?

This comment has been minimized.

Copy link
@starry-abyss

starry-abyss Oct 3, 2016

Author Contributor

Yeah, no problem

endDrawDebug(camera);
}

function drawDebugBoundingBox(gfx:Graphics, rect:FlxRect, allowCollisions:Int, highlight:Bool)

This comment has been minimized.

Copy link
@Beeblerox

Beeblerox Oct 2, 2016

Member

maybe rename highlight parameter to partial as well?

private var _debugTileNotSolid:BitmapData;
private var _debugTilePartial:BitmapData;
private var _debugTileSolid:BitmapData;
private var updateDebugTileBoundingBoxesInit = false;

This comment has been minimized.

Copy link
@Beeblerox

Beeblerox Oct 2, 2016

Member

i'd rather remove this var and check if _tileWidth and _tileHeight are initialized (have values more than 0).

so we could have something like:

override private function set_debugColorScheme(value:DebugColorScheme):DebugColorScheme
    {
        this.debugColorScheme = value;

        if (_tileWidth > 0 && _tileHeight > 0)
        {   
            updateDebugTileBoundingBoxes();
        }

        return value;
    }
_debugTileNotSolid = makeDebugTile(FlxColor.BLUE);
_debugTilePartial = makeDebugTile(FlxColor.PINK);
_debugTileSolid = makeDebugTile(FlxColor.GREEN);
if (_debugTileSolid == null)

This comment has been minimized.

Copy link
@Beeblerox

Beeblerox Oct 2, 2016

Member

too much repetitions.

Maybe it would be better to have this?

private function updateDebugTileBoundingBoxes():Void 
    {
        if (FlxG.renderBlit)
        {
            _debugTileSolid = updateDebugTile(_debugTileSolid, debugColorScheme.solid);
            _debugTilePartial = updateDebugTile(_debugTilePartial, debugColorScheme.partial);
            _debugTileNotSolid = updateDebugTile(_debugTileNotSolid, debugColorScheme.notSolid);
        }
    }

    private function updateDebugTile(tileBitmap:BitmapData, color:FlxColor):BitmapData
    {
        if (tileBitmap != null && (tileBitmap.width != _tileWidth || tileBitmap.height != _tileHeight))
            tileBitmap = FlxDestroyUtil.dispose(tileBitmap);

        if (tileBitmap == null)
            tileBitmap = makeDebugTile(color);
        else
        {
            tileBitmap.fillRect(tileBitmap.rect, FlxColor.TRANSPARENT);
            drawDebugTile(tileBitmap, color);
        }

        return tileBitmap;
    }
updateDebugTileBoundingBoxes();
}

private var _debugTileNotSolid:BitmapData = null;

This comment has been minimized.

Copy link
@Beeblerox

Beeblerox Oct 2, 2016

Member

and don't forget to dispose these bitmaps in destroy() method. Change lines

_debugTileNotSolid = null;
_debugTilePartial = null;
_debugTileSolid = null;

to

_debugTileNotSolid = FlxDestroyUtil.dispose(_debugTileNotSolid);
_debugTilePartial = FlxDestroyUtil.dispose(_debugTilePartial);
_debugTileSolid = FlxDestroyUtil.dispose(_debugTileSolid);

(this don't have to be the part of this PR, but it will make code a tiny-tiny bit better :)

@starry-abyss

This comment has been minimized.

Copy link
Contributor Author

starry-abyss commented Oct 4, 2016

@Beeblerox So... any clue how to force tile update on Flash without waiting for camera movement?

public var debugColorScheme(default, null):DebugColorScheme;
public var debugBoundingBoxColor(default, set):Null<Int> = null;
public var debugBoundingBoxColorNotSolid(default, set):Null<Int> = null;
public var debugBoundingBoxColorPartial(default, set):Null<Int> = null;

This comment has been minimized.

Copy link
@Gama11

Gama11 Oct 4, 2016

Member

I think these should be FlxColor instead of Null<Int>? And also be initialized with their default values right away?

This comment has been minimized.

Copy link
@starry-abyss

starry-abyss Oct 4, 2016

Author Contributor

I'm not sure. I want to use debugBoundingBoxColor which is the original field prior to my PR. Won't changing the type cause any breaking? Or should I not mess with the original field at all and should create a new one?

This comment has been minimized.

Copy link
@Gama11

Gama11 Oct 4, 2016

Member

I'm not sure that one should be touched at all. Isn't the use-case for that "use the color I assigned to it no matter what value for solid etc is"?

This comment has been minimized.

Copy link
@starry-abyss

starry-abyss Oct 4, 2016

Author Contributor

OK, makes sense

@Gama11 Gama11 changed the title Debugger: hide decorative tiles by default in tilemaps. FlxObject: allow configuring debug rect colors Oct 9, 2016
@Gama11 Gama11 merged commit 5cb841d into HaxeFlixel:dev Oct 9, 2016
Aurel300 added a commit to larsiusprime/haxeflixel that referenced this pull request Apr 18, 2018
Improved FlxTilemap's drawDebug:
- partially solid tiles are now colored differently
- non-solid tiles are now transparent by default
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.