TheDuckCow / godot-road-generator

A godot plugin for creating 3D highways and streets.
MIT License
313 stars 16 forks source link

Select nearest RoadPoint on RoadSegment click #94

Closed TheDuckCow closed 1 year ago

TheDuckCow commented 1 year ago

This is to address usability feedback from an interview conducted, where the most unintuitive thing is that it's hard to select the RoadPoint.

Idea of how this would work:

The two options above would mostly only have different behavior in extreme cases where we have long handles extending out but then both roadpoints are place on top of each other or are overlapping in some sense. I'm slightly leaning towards the second method for what it's worth.

We have click handling code in the Wheel Steal project, can be shared as necessary (though this is not in a tool context, so some research may still be necessary, especially if there's a built in way to do this without making our own handler)

TheDuckCow commented 1 year ago

Assigning to you @bdog2112 but we can discuss more / I can share code snippets when I get the chance.

bdog2112 commented 1 year ago

Select-RoadPoint-via-click is partially working. But, there are conflicts with Translation, Rotation, Scale and Box selection widgets. We need to figure out a way to make everything play nicely together.

In order to implement selection, we have to monitor ALL mouse events. Then, filter down to "click" events. We can further filter on "left/middle/right" clicks as well as "press/release".

When a user left-clicks in the 3D view, the "selection changed" event is invariably triggered whether we like it or not. As it turns out, this has the effect of overriding whatever selection we make via code. If the user clicks a RoadSegment and we mark it as selected, "selection changed" comes along and deselects it.

We can prevent the triggering of "selection changed" by flagging "left-click" events as "handled" when a RoadSegment is clicked. But, then Translation, Rotation, and box selection largely cease to work.

Some potential solutions include:

Code example forthcoming in this branch.

TheDuckCow commented 1 year ago

Hey @bdog2112, just wanted to say I did a little research and experimentation myself. I tried your branch, and generally speaking found it working, but did indeed run into some of the awkwardness this solution as-is creates. It also felt rather "sluggish", which might be a consequence of the approach.

In a new branch based on dev (not pushed), I tried making these changes below - making it so that the road segment itself could track clicks on itself. We already set up static bodies with colliders, that's how the cars are able to drive on the road. There are signals on static bodies for capturing inputs, which we use in the wheel steal game to click on a car to jump into it (before we develop a more sophisticated hijack sequence anyways). Copying this over to the road generator, I found this code triggers as we expect in the runtime of the game (printing as you'd expect), but not in the editor unfortunately. But there may still be some merit to this approach.

road-generator $ git diff addons/road-generator/road_segment.gd
diff --git a/addons/road-generator/road_segment.gd b/addons/road-generator/road_segment.gd
index d351c53..f45b8e5 100644
--- a/addons/road-generator/road_segment.gd
+++ b/addons/road-generator/road_segment.gd
@@ -4,6 +4,7 @@
 ## the structure stays light only until needed.
 extends Spatial
 class_name RoadSegment, "road_segment.png"
+tool

 const LOWPOLY_FACTOR = 3.0

@@ -306,6 +307,21 @@ func _build_geo():
        road_mesh.create_trimesh_collision() # Call deferred?
        road_mesh.cast_shadow = GeometryInstance.SHADOW_CASTING_SETTING_OFF

+       for static_body in road_mesh.get_children():
+               print("Children of roadseg:", static_body, static_body.name)
+               if not static_body.is_connected("input_event", self, "_static_input_event"):
+                       print("Not connected")
+                       var res = static_body.connect("input_event", self, "_static_input_event")
+                       assert(res == OK)
+               else:
+                       print("(was connected)")
+
+
+## Captures events on the static body generated on Road Segments.
+func _static_input_event(_camera, event, _click_position, _click_normal, _shape_idx):
+       if event is InputEventMouseButton and event.pressed and event.button_index == BUTTON_LEFT:
+               print("clicked "+ self.get_parent().name + " from " + self.name)
+

 func _insert_geo_loop(
                st: SurfaceTool,

I assume the key issue is that the static body signals do not execute in the editor, and we probably don't have a way to directly change that. But nonetheless, on a whim, I turned to chatgpt to see what it had to say. Complete warning that this is not validated and could be garbage, but maybe it sparks some ideas of what we could do (sorry for the format of this, the best I could manage):

screencapture-chat-openai-2023-04-30-21_08_36

The interesting part here to me: Extending static body to be a node that does what we need. Maybe we can make this custom static body act as a tool and bubble up signals if (and only if) the editor is active.

bdog2112 commented 1 year ago

All in all, we also mustn't lose sight of the fact that we need the ability to ignore widget clicks. That's what we're currently missing.

It's difficult to find good editor plugin code examples because the keywords are too easily misinterpreted to mean other things.

The thought of overriding "staticbody" is intriguing. But, I'm not sure where that gets us. It's unclear if a signal-enabled staticbody would ignore widget clicks or if the clicks would trigger both widget and staticbody events simultaneously.

As you know, staticbody's signals are primarily only accessible in the runtime. There may be the possibility that we can, somehow, enable them in the editor. If it's at all possible, it will most likely be done by enabling a setting in a class such as "viewport". We need to find the class and the setting.

The "viewport" class provides subroutines that may (or may not) be useful in our efforts. A couple of interesting ones: _gui_has_modalstack and _get_modal_stacktop. Ideally, they would allow us to view all ray collisions as an ordered stack. It sounds like they may only be useful for modal popups at runtime. But, it would be awesome if they could allow us to differentiate between a gizmo and staticbody!

Some other viewport subroutines with potential: is_input_handled, set_input_handled, unhandled_input, handle_input_locally. Gizmos should, theoretically, handle some inputs and ignore others. Maybe we can use this to our advantage.

Lastly, it's important to note that _forward_spatial_guiinput essentially gives us access to mouse events in the editor that would, otherwise, be inaccessible.

Addendum: Physics picking uses a system of 20 layers for collisions. It would be useful to know if widgets are on one of those layers.

bdog2112 commented 1 year ago

I posted an inquiry on Godot's Q&A website. Who knows, maybe we'll get a hit. (Go go Godot! ;-)

As I was posting, I also noticed that the transform widget works fine if a RoadPoint is selected manually via the Scene dock. It only behaves strangely when selected via code.

I'm sure it probably has something to do with marking the "release-click" event as "handled". At any rate, there may be some additional steps that resolve the problem or maybe a different approach, altogether.

bdog2112 commented 1 year ago

I felt like this issue was close to a fully working solution. So, I devoted some of my personal time to work through the problem we observed. (FYI: ChatGPT was full of red herrings and should not be used. Seriously! ;-)

It was clear that setting the mouse click event as "handled" was causing the problems. So, the solution had to, somehow, work around that. Hence, I set the event to "unhandled", stored a reference to the target RoadPoint, and selected it in the _on_selection_changed routine, which was a bit ironic. Now, it works! See the latest commit.

I'm happy with the progress I've made. But, more work is needed to fully bring the solution over the finish line.

If you're satisfied with the approach taken and you want to allocate more resources to the effort, then let me know and I will finish it up. In the meantime, I'll move on to the other tasks in the queue.

TheDuckCow commented 1 year ago

Hey @bdog2112 that's encouraging to hear. I just tried it locally, indeed seems better. Giving you the go ahead to pursue. Though we still have some "infighting" in that trying to click on our blue square gets overwritten depending on which side of the blue square (might initially select that RoadPoint, but then pop the selection to the next one down). Ideally we can skip the extra check if we know we just performed a changed selection via click on a roadpoint. Though hacky, a small timer could provide the value we need there.

Could you open a PR (you can open it as a "draft" from the drop down to the right of where the create PR button would be on github), so we can link it to this branch?

bdog2112 commented 1 year ago

There will be additional logic to get the RoadPoint closest to the click location. That should take care of the "infighting" issue.

Yes, I'll open a PR.

bdog2112 commented 1 year ago

Doh! Almost forgot, Segment click-to-select needs additional requirements.

Aight. It's time for sleep!

TheDuckCow commented 1 year ago

Responding to questions here:

If any of this isn't directly possible or deeply troublesome, let me know - selecting by click is a convenience, but we shouldn't be totally reinventing the wheel. I'm reading the above points and already wondering whether it's getting too complex. I'd say for the next version for me to QA review, let's just focus on single selection, but I shared my answers above just to state what I think the long term ideal would be.