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

Debugger: add "FlxObjects-are-draggable"-mode #1862

Merged
merged 47 commits into from
Aug 21, 2016

Conversation

Dovyski
Copy link
Contributor

@Dovyski Dovyski commented Jun 3, 2016

This pull request contains a port of Flixel community's interactive debug feature. The new "interactive debug" mode can be activated by clicking the pencil icon at the top of the debugger.

Available tools (functionalities) during interaction:

  • Pointer: select this tool on the toolbar on the right, then click any object on the screen. Multiple objects can be selected by holding CTRL during the clicks;
  • Mover: move selected object(s) by holding SHIFT while clicking and dragging the objects.
  • Eraser: delete the selected objects. It's also possible to delete selected objects by pressing DELETE; by using SHIFT + DELETE the object will be deleted and removed from memory.

This implementation has the idea of a toolbar (appears at the left of the screen when the interactive debug is activated). I am waiting for feedback on this. If you guys like the idea, I can fix any bugs and improve the toolbar (make it look more like a toolbar), otherwise I can remove it all together.

I implemented the toolbar because I envision more tools in the future, like a tile manipulator, rotation, etc.

_NOTICE_: a lot of the methods I created are not documented. I will add documentation after the review process has settled all details/changes.

Here is a gif of the whole thing working:

interactive_debug_move_delete

@Gama11
Copy link
Member

Gama11 commented Jun 3, 2016

On Neko, Mode seems to crash immediately:

Invalid operation (-)
Called from flixel.system.debug.interaction.Interaction::keyJustPressed line 315
Called from flixel.system.debug.interaction.tools.Eraser::update line 36
Called from flixel.system.debug.interaction.Interaction::preUpdate line 167
Called from flixel.util._FlxSignal.FlxSignal0::dispatch0 line 292
Called from flixel.FlxGame::update line 723
Called from flixel.FlxGame::step line 663
Called from flixel.FlxGame::onEnterFrame line 530
Called from openfl._legacy.events.EventDispatcher::dispatchEvent line 98
Called from openfl._legacy.display.DisplayObject::__dispatchEvent line 182
Called from openfl._legacy.display.DisplayObject::__broadcast line 161
Called from openfl._legacy.display.DisplayObjectContainer::__broadcast line 286
Called from openfl._legacy.display.Stage::__render line 1103
Called from openfl._legacy.display.Stage::__checkRender line 351
Called from openfl._legacy.display.Stage::__pollTimers line 1084
Called from openfl._legacy.display.Stage::__doProcessStageEvent line 430

@Gama11
Copy link
Member

Gama11 commented Jun 3, 2016

The UI is a bit confusing to me. 100% alpha seems to mean that the tool is not active, while a lowered alpha means that the tool is active? I'd have expected this to behave the other way around, similar to the debugger window toggle buttons.

I'm also not sure how it makes sense that multiple tools can be selected at once. I haven't really been able to make the eraser work. And the cursor only ever seems to change to the first tool?

It also seems like whenever the cursor changes to the first tool, the system cursor is also shown, which is distracting because of the mouse lag.

@Gama11
Copy link
Member

Gama11 commented Jun 3, 2016

a lot of the methods I created are not documented. I will add documentation after the review process has settled all details/changes.

I wouldn't worry about documentation too much. These are just internal debugger methods (not public API), there isn't a whole lot to be documented about them as long as they are aptly named.

@Dovyski
Copy link
Contributor Author

Dovyski commented Jun 3, 2016

On Neko, Mode seems to crash immediately:

I forgot to check Neko (and all other platforms but Flash). I'm sorry, single platform habit. I will fix the problem.

The UI is a bit confusing to me. 100% alpha seems to mean that the tool is not active, while a lowered alpha means that the tool is active? I'd have expected this to behave the other way around, similar to the debugger window toggle buttons.

Yeah, the UI is not final. I want to replace the current alpha buttons with "real" buttons (the ones with background, border, etc). I think it will make everything easier to understand/use. I will only go for that if you approve the idea of a toolbar.

I'm also not sure how it makes sense that multiple tools can be selected at once. I haven't really been able to make the eraser work. And the cursor only ever seems to change to the first tool?

Those are all bugs that I will fix after we decide on the use of a toolbar or not.

It also seems like whenever the cursor changes to the first tool, the system cursor is also shown, which is distracting because of the mouse lag.

Hum, I need to add some custom mouse handling code to the debugger to fix that (as we previously discussed).

@Dovyski
Copy link
Contributor Author

Dovyski commented Jul 14, 2016

I've made the changes you asked for, @Gama11

  • No more active tools by default when use is finished;
  • Fix all mouse issues (flickering, hard to hit clicks, etc);
  • Tools window is now horizontal (no empty spaces);
  • Add 2px screen padding;
  • Pencil icon has more antialiasing and is placed to the left of bitmap log;
  • Interactive debug works with vcr (e.g. when game is paused or a replay is being played);
  • Removed the eraser tool from tools bar (it's still possible to delete things by pressing DEL on keyboard though);

Some thoughts:

  • Buttons can be selected/moved using the interactive tools when the game is paused. This is the same behavior when any other interaction happens when the game is running (e.g. moving the main character in Mode is affected by gravity).
  • Selecting tiles/tilemaps will require a dedicated interaction tool because there are too many corners cases to cover in the current pointer tool.
  • I want to create another window to house actions to be performed when something is selected. For instance, if you select a sprite, the actions window will show buttons to delete/show info, etc. It's pretty much what happens when you use a tool in Photoshop/Gimp.

I would rather like to gradually add the actions window and the tile tool in the future using dedicated PRs than continue to bloat this PR here. What do you think?

@Gama11
Copy link
Member

Gama11 commented Jul 14, 2016

I would rather like to gradually add the actions window and the tile tool in the future using dedicated PRs than continue to bloat this PR here. What do you think?

Makes sense, I prefer small PRs anyway.

@Dovyski
Copy link
Contributor Author

Dovyski commented Jul 14, 2016

Great! I will wait for any issues with this PR then. I will work on the new tools after it gets merged.

@Gama11 Gama11 added this to the 4.2.0 milestone Aug 21, 2016
@Gama11
Copy link
Member

Gama11 commented Aug 21, 2016

I feel like at this point, this is polished enough to be merged - thanks for the amazing contribution! 👍

Without knowing the keyboard shortcut(s), it's a bit cumbersome right now, so it'd be nice to have tooltips or somethings as hints. Of course, that'd require implementing tooltips for the debugger first.. :D The need to counteract the velocities of moving objects also bothers me a bit when using this in Mode.

Minor polish issue: there's a fraction of a second at startup, at least on Flash, where the "move" cursor is visible instead of Mode's default cursor.

It would be nice if the new functionality could be documented here: http://haxeflixel.com/documentation/debugger/ (sources are located in the flixel-docs repo, you can also click "Edit" in the upper right).

Note that this should target the dev branch of flixel-docs, not master.

@Gama11 Gama11 merged commit 8e9858a into HaxeFlixel:dev Aug 21, 2016
Gama11 added a commit that referenced this pull request Aug 21, 2016
see #1862

- use for-each loops
- replaced selectedItems with a FlxTypedGroup<FlxObject>
- make use of (default, null) properties
- invert if-statements to reduce nesting
- remove empty doc comments
@Gama11
Copy link
Member

Gama11 commented Aug 21, 2016

I did a bit of code cleanup. I hope I didn't break anything in the process.

Your code is quite consistent and clean (I also like the architecture), but a bit AS3-y. ;)

@Dovyski
Copy link
Contributor Author

Dovyski commented Aug 21, 2016

I feel like at this point, this is polished enough to be merged - thanks for the amazing contribution! 👍

Thank you for all the guidance! It's a pleasure to contribute to HaxeFlixel 😄 🎉

Without knowing the keyboard shortcut(s), it's a bit cumbersome right now, so it'd be nice to have tooltips or somethings as hints. Of course, that'd require implementing tooltips for the debugger first.. :D

Agreed! I am planning to give UX a bit of love in the upcoming interactions (e.g. animated, dashed line for selected items, better handling of clicks to prevent unselecting things by accident, probably a Ctrl+Z functionality). I will add tooltips to that list too.

The need to counteract the velocities of moving objects also bothers me a bit when using this in Mode.

You can avoid that by using the "pause" button of VCR. I will add that info to the docs.

Minor polish issue: there's a fraction of a second at startup, at least on Flash, where the "move" cursor is visible instead of Mode's default cursor.

I will check on that.

It would be nice if the new functionality could be documented here: http://haxeflixel.com/documentation/debugger/ (sources are located in the flixel-docs repo, you can also click "Edit" in the upper right). Note that this should target the dev branch of flixel-docs, not master.

Sure! I think I could write the docs as a mini tutorial, with small gifs explaining each tool and how they can be used. How about that?

Additionally since more things will be added in the future (e.g. the tile tool), I suggest we create a separate page for interactive debug, otherwise http://haxeflixel.com/documentation/debugger/ will get a lot of text and images.

Just let me know how you prefer it.

I did a bit of code cleanup. I hope I didn't break anything in the process.

I've made some tests and everything looks ok.

Your code is quite consistent and clean (I also like the architecture), but a bit AS3-y. ;)

Thanks! I've carefully inspected your changes (sorry about giving you that much work!) and will adherence to that in the future. About the AS3-y, I'm still migrating to Haxe 😄

@Gama11
Copy link
Member

Gama11 commented Aug 21, 2016

Agreed! I am planning to give UX a bit of love in the upcoming interactions

Looking forward to it.

You can avoid that by using the "pause" button of VCR. I will add that info to the docs.

Yeah, I noticed that that's working now, great work on that. :) Definitely something that should be noted in the docs.

[...] I suggest we create a separate page for interactive debug

Sounds very sensible, that page is already fairly long.

I've made some tests and everything looks ok.

Awesome!

sorry about giving you that much work!

Please don't apologize for contributing! Besides, it's my problem if I'm this obsessed about code style. ;)

@Dovyski
Copy link
Contributor Author

Dovyski commented Aug 21, 2016

Ok, awesome! I will start working on the docs soon.

Besides, it's my problem if I'm this obsessed about code style. ;)

You have to be this obsessed, otherwise we will end up with a mess at the end of the day :D

@Gama11
Copy link
Member

Gama11 commented Aug 23, 2016

Something you should probably know, since it's bitten me a few times: OpenFL has a bit of code in its externs that leads to what would otherwise have been a compiler error being a runtime error (for field access of DisplayObjcet instances specifically). See openfl/openfl#1065 for more info.

To fix this, go to C:\HaxeToolkit\haxe\lib\openfl\3,6,1\extern\flash\display\DisplayObject.hx and remove this code:

implements #if flash Dynamic #else Dynamic<DisplayObject> #end

@Dovyski
Copy link
Contributor Author

Dovyski commented Aug 23, 2016

Ok, thanks for the heads up!

@MSGhero
Copy link
Member

MSGhero commented Sep 6, 2016

When my game first starts up (flash target, haven't tried others), the cursor is the drag one instead of the regular one. It seems like it resolves itself by frame 2, but it's apparent for me because my frame 1 takes a while with random generation.

@Dovyski
Copy link
Contributor Author

Dovyski commented Sep 11, 2016

I will check on that @MSGhero, thanks for the heads up.

@Gama11
Copy link
Member

Gama11 commented Sep 11, 2016

I actually just fixed that: 80cd87f

Aurel300 pushed a commit to larsiusprime/haxeflixel that referenced this pull request Apr 18, 2018
Aurel300 pushed a commit to larsiusprime/haxeflixel that referenced this pull request Apr 18, 2018
see HaxeFlixel#1862

- use for-each loops
- replaced selectedItems with a FlxTypedGroup<FlxObject>
- make use of (default, null) properties
- invert if-statements to reduce nesting
- remove empty doc comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants