Closed drbbgh closed 2 days ago
@mapedorr LGTM, almost all boilerplate code, but there are small changes to the engine code you may want to review.
Changes discussed with @mapedorr and @drbbgh:
_process()
and spare a lot of computation. This needs to be investigated and tested.I'm going to answer your comments step by step so I don't forget something:
- If I'm editing an object's Polygon and press ESC or click on another object within the Canvas Editor, the previous object's Polygon keeps visible.
Fixed: now with esc or wild click, you go back to the object holding the polygon.
- If I change the InteractionPolygon of a PopochiuRegion inside the room, changes are lost when the scene is closed and opened again.
UPDATE: I'm also losing changes made to InteractionPolygon in props and hotspots.
Everything has been fixed and I also optimized the code that saves those propertied in the PopochiuClickable. Now syncing doesn't happen on every frame but only on saving. The editing_polygon
variable was then useless and I removed it.
Recomendations 😳
* How about hiding the Walk-to Point gizmo if the **PopochiuClickable** `clickable` property is `false`?
Considering this, we don't think this is good: in 2.0 you can change the clickable
property by script, and you may want to set the WTP for a prop or hotspot which is not initially clickable, from the start.
We'll leave the visibility setting to the user, by mean of the toolbar buttons.
* Change the Walk-to Point icon to green. The current icon uses Cyan, which causes confusion with the baseline gizmo color. Likewise, the Baseline icon could be Cyan.
Checked that they don't conflict.
* Create an icon for the Dialog Position button. It should be Magenta to match the color of the gizmo.
Done, it was obvious but we forgot in the first iteration.
* Add a configuration option (Project Settings) to define whether one wants the **PopochiuWalkableArea** polygons to always be visible.
Done
About the icon colors, and knowing devs can customize the color of their gizmos, maybe those icons could be black and white, or in grayscale, so we can change their
modulate
property to match the colors defined in the Editor Settings.
Done. There is also a preference for that behavior.
Question 🤔
This can be addressed later, but I want to know your opinion.
* What if we use our custom toolbar instead of the `EditorPlugin.CONTAINER_CANVAS_EDITOR_MENU`? We have some options like: `CONTAINER_CANVAS_EDITOR_SIDE_LEFT`, `CONTAINER_CANVAS_EDITOR_SIDE_RIGHT`, and `CONTAINER_CANVAS_EDITOR_SIDE_BOTTOM`.
We would leave the toolbar where it belongs: that's where the user goes and doesn't break the usual pattern. We added a label to tell Popochiu buttons apart from the rest :) It should work pretty fine.
With all of that, the PR should be over. Let us know if you notice something.
@mapedorr I addressed your suggestions (god bless GitHub's suggestions).
I think I should add the clickable elements polygon to the group programmatically on _ready() and fix the references to the icons, before this can be closed, right?
@stickgrinder Yes. After that I'm gonna make a couple of tests to check everything is fine and then I'll merge.
After making some tests I found a couple of bugs and have some additional comments.
When editing characters, pressing the Interaction Polygon button the Popochiu CanvasEditor menu doesn't select the InteractionPolygon node.
https://github.com/carenalgas/popochiu/assets/4536477/45fda27d-4767-4df3-8764-c7b9bc51ffa1
After creating a new project with a room, the PC didn't move, even though the walkable area is properly configured. The only difference I noticed between an older project (the editor in the background) and the new one (the editor in the foreground) is that the new one appears to have all the layers disabled in the parsed_collision_mask
property of the NavigationPolygon.
This is due to a bug in the editor_config.gd
script. You can still play the game by pressing F12 when the error is shown in the Debug panel.
For this I suggest doing this:
GIZMOS_FONT_SIZE: (func() -> int:
if Engine.is_editor_hint():
return EditorInterface.get_editor_theme().default_font_size
else:
return 16
).call(),
But feel free of putting that in a separate private static function if you think is too dirty.
When you are editing the InteractionPolygon of any object and click another one, the polygon of the previous one keeps visible on screen. Is not a problem from my point of view, so feel free to change this in this PR or leave it for later.
https://github.com/carenalgas/popochiu/assets/4536477/1f453287-559b-4abd-84cd-eec744f4d805
I miss being able to see the interaction polygon when selecting a Popochiu object (prop, hotspot, etc.) without needing to click the button for editing it. If this is the expected behavior, maybe devs should be able to configure whether they want the interaction polygons to be visible when selecting a Popochiu object. Or maybe in the future it will be necessary to have two buttons: one to show/hide the polygon, another to allow editing it.
@mapedorr, please check that the changes to
PopochiuUtils.get_screen_coords_for()
have no unwanted impact.