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

Establishing Connection tools #131

Merged
merged 10 commits into from
Oct 27, 2023
Merged

Establishing Connection tools #131

merged 10 commits into from
Oct 27, 2023

Conversation

TheDuckCow
Copy link
Owner

@TheDuckCow TheDuckCow commented Oct 16, 2023

Includes necessary utility function to work around the autofix cyclic functionality that was getting in the way.

Still in draft due to existing bug, see comment below.

Includes necessary utility function to work around the autofix cyclic
functionality that was getting in the way.

Acknowledging there is one bug in this commit whereby clicking on a
RoadSegment to grab a RoadPoint results in not being able to use its
built in 3D transform gizmo as everything is treated as a drag.
@TheDuckCow
Copy link
Owner Author

Acknowledging there is one critical bug present in dev and thus also here, whereby clicking on a RoadSegment to grab a RoadPoint results in thereafter not being able to use a RoadPoints's built in 3D transform gizmo. Everything gets treated as a drag. I will edit this PR once that is resolved. With that being just a (albeit nasty) UI interaction bug, all tests we currently have are still passing.

Checking out commits one-by-one, this issue first presented itself and has been there ever since: commit 17a16f4f1cd28423d7055d899031c17d1cea0552. Unfortunately that was one of the larger commits, so will need to dissect it further.

@TheDuckCow
Copy link
Owner Author

Need to call it a night after a fair bit of troubleshooting. In short, the issue remains:

  1. Try selecting the roadpoint via the hierarchy viewer, or by clicking the blue square
  2. Try manipuating the built-in 3D gizmo, it should work fine.
  3. Now try clicking on the road segment to trigger an auto-selection of the nearest roadpoint
  4. Now try using the built-in gizmo, you will be unable as it will continually be trying to do a "drag" selection there and after
  5. Now select the RoadPoint by clicking the blue square or via the hierarchy viewer
  6. Using the 3D gizmo should now again work as intended.

Basically it boils down to treating the click event as consumed or not:

  • If marked not consumed, then even though we set the selection, the built in editor immediately overrides that selection based on what the click would do on its own. ie clear selection if nothing underneath, or select the mesh/object underneath the mouse (if any)
  • If marked as consumed: We get stuck in a drag state. I'm not 100% why yet, but it comes down to what I think is an issue of 3D gizmos not marking their own clicks as "consumed". We are doing the selection change on release only, and that's only after first checking that the pressed position hasn't changed since the release position (to support dragging, such as any manipulation of the 3D gizmo). So if we mark the initial press as consumed, we completely disable any use of built in gizmos. But if we don't mark it, then we get stuck in the weird drag state where it's waiting for its ending "consumed" release press later.

I'll need to keep thinking on this. We may just be simply approaching this the wrong way, and need to roll up sleeves and actually update the gizmo mesh to include half of road segments in each direction. The simpler fallback would be to revert to the way we had it before, where we just tag a node to be re-selected right after the built in editor does it thing (I'm more likely to do that in the interest of time).

This is the only way I was able to get around the issue of fighting
internal 3D gizmo handles and the drag and drop operator.
@TheDuckCow TheDuckCow marked this pull request as ready for review October 16, 2023 14:49
@TheDuckCow TheDuckCow requested a review from bdog2112 October 16, 2023 14:49
@TheDuckCow
Copy link
Owner Author

Ok I've reverted the selection code - so I'd say it's now ready for review! Side note, I am expected to update documentation separately, so no need to comment on that

@TheDuckCow TheDuckCow linked an issue Oct 16, 2023 that may be closed by this pull request
@TheDuckCow
Copy link
Owner Author

Acknowledging this issue I found, regarding making connections that go in opposing orders (ie prior pt is set as the prior path on the newly connected RP, instead of prior connecting to next). Not sure yet if this is an issue of the connection tool itself, or a wider consequence of the restructure we engaged with for this 0.4.0 release. Ideally we do resolve this before merging though.

reverse.direction.connection.issue.mp4

At a first look of this video, it seems like the correct prior and next point paths are indeed being assigned to the property values, but it's clearly not being handled correctly and leading to duplicative geometry.

@bdog2112
Copy link
Collaborator

bdog2112 commented Oct 18, 2023

The newly developed functionality was focused on a new hierarchy for creating roads as well as some new tools for selecting, adding, and removing RoadPoints. So, I approached these tests from the angle of how things ought work given how they used to work.

Tests:

  • Recreated previous process of adding a 2x2 road using new functionality. New behavior was to add a RoadManager. Then select it and add a RoadContainer. Then, add a 2x2 Road.
    • (Previous behavior was to add a RoadNetwork. Then select it to reveal a "Create" menu in the 3D view, which contained a "2x2 road" selection)
    • Created a scene and clicked "+" button in the Scene dock to add a new node. Searched for "RoadManager" in the node list. It showed up as expected along with RoadContainer, RoadIntersection, RoadLane, and RoadPoint.
    • Selected "RoadManager" and clicked "Create" to add it to the scene.
    • Selected RoadManager in Scene dock, which revealed a new toolbar in the 3D view.
      • Toolbar lacked tooltips to explain what the buttons did. But, there were 3 buttons and testing revealed that they were: 1 Select RoadPoint, 2 Add/Connect RoadPoint/Segment, 3 Delete RoadPoint/Segment.
      • There was also a "Roads" menu with the following items: "Refresh roads", "Select container", "RoadContainer", "RoadPoint", "RoadLane (AI path)", "2x2 road".
    • Went to "Roads" menu and clicked "RoadContainer". A new RoadContainer named "Road_001"was added as a child of the RoadManager in the Scene dock. The new RoadContainer was selected and a yellow warning icon was shown next to it that read: "Node configuration warning: Add RoadPoint nodes as children to form a road, or use the create menu in the 3D view header".
    • Went to the "Roads" menu in the 3D view, again, and clicked "2x2 road". 2 RoadPoints were added to the 3D view with a segment drawn between them. The 2nd RoadPoint was selected both in the Scene dock and the 3D view. A blue lane widget with blue handles was displayed on the selected RoadPoint and pink magnitude handles were also shown.
    • THE YELLOW NOTIFICATION ON THE ROADCONTAINER DID NOT GO AWAY WHEN THE 2X2 ROAD WAS ADDED. Toggling RoadManager/RoadContainer/RoadPoint selection in Scene dock and 3D view had no effect. Notification was cleared by going to the "Roads" menu and clicking "Refresh roads".

(continued in next post)

@bdog2112
Copy link
Collaborator

bdog2112 commented Oct 18, 2023

  • Tested new RoadPoint toolbar in the 3D view.
    • Observed that the buttons were identical to Godot's path editing buttons.
    • Toggled between each of the buttons and observed that the icons changed (slightly) to indicate which button was selected.
    • Select RoadPoint button caused 3D cursor to select objects in the 3D viewport as it normally would.
    • Add/Connect RoadPoint/Segment button performed two tasks. 1 Add a new RoadPoint, 2 Connect two RoadPoints.
      • The behavior was determined by the current selection and the location where the user clicked.
      • If an end RoadPoint was selected and the user hovered over another end RoadPoint, then blue dots were drawn on the source and destination RoadPoints and a blue line was drawn between them. If the user clicked on the RoadPoint where they were hovering, then a segment was created between the source and destination.
      • User could only connect to RoadPoints at the end of a chain. They could not connect to RoadPoints that were already fully connected.
      • If user selected a middle RoadPoint and engaged the Add/Connect RoadPoint/Segment button, then red dots were displayed on the source and destination RoadPoints and a red line was drawn between them. Clicking the destination RoadPoint resulted in the segment being removed between them.
      • THERE WERE SEVERAL INCONSISTENCIES IN THE WAY THIS BEHAVIOR WORKED. DUE TO LIMITED TIME FOR TESTING, I WON'T GO INTO MUCH GREATER DETAIL. BUT, DEVELOPER SHOULD CONSIDER CODING FOR SEVERAL SCENARIOS INCLUDING BUT NOT LIMITED TO:
        • Create road with 3 RoadPoints.
        • Select 3rd RoadPoint. Engage Add/Connect button and try disconnecting and re-connecting 2nd RoadPoint. (Cannot re-connect them.)
        • Select 3rd RoadPoint. Engage Add/Connect button and try disconnecting and re-connecting 1st RoadPoint. (Doesn't yield expected result.)
        • Repeat above steps. But, try selecting the 1st RoadPoint and disconnecting/re-connecting the 2nd and 3rd RoadPoints. (Doesn't yield expected result.)
        • Each one of this scenarios seemed to have erratic behavior and each outcome differed slightly from the others in terms of whether a blue or red connector line was or wasn't displayed and whether a connection could be restored.
        • If the above scenarios were prevented, then the developer wouldn't have to facilitate them. So, that's always an option.
      • Delete RoadPoint/Segment button allowed the user to delete RoadPoints.
        • Selected a RoadPoint and engaged Delete RoadPoint/Segment button.
        • When hovering over segments, a red "X" was displayed on the nearest RoadPoint. When the segment was clicked, the RoadPoint was removed.
        • (If there was no segment, then a RoadPoint could not be removed with this tool.)
      • Tested each of the elements in the "Roads" menu. Limited this testing for brevity and time.
        • Successfully added RoadContainer, RoadPoint, RoadLane, and 2x2 road.
        • WHEN A ROADLANE WAS ADDED, IT ACTIVATED A "ROADLANE TOOLBAR" IN ADDITION TO THE "ROADPOINT TOOLBAR". (BOTH WERE DISPLAYED SIMULTANEOUSLY.)
        • IT WAS UNCLEAR HOW TO INTERACT WITH ROADLANES. WERE THEY ATTACHED TO ROADPOINTS? COULD THEY BE MOVED SEPARATELY? (THEY WERE IMMOVEABLE.) SHOULD THEY BE CHILDREN OF THE ROADPOINT OR ROADCONTAINER? SHOULD THE RED "X" TOOLBAR BUTTON DELETE ROADLANES? (IT DELETED ROADPOINTS.)
        • Maybe the RoadPoint toolbar should be hidden when the RoadLane toolbar is shown and vice-versa.
          (continued in next post)

@bdog2112
Copy link
Collaborator

bdog2112 commented Oct 18, 2023

  • Confirmed some unexpected behavior with RoadPoint gizmo. Although, it was slightly different than the reported behavior.
    • When segment was clicked, nearest RoadPoint became selected. But, blue lane widget WAS NOT displayed. Although, blue lane widget handles WERE displayed.
    • Observed that lane widget refreshed if user hovered over the blue handles or clicked on the RoadPoint selector. (Methinks this is an issue centered around plugin.gd on_selection_changed. Something needs to trigger a lane widget refresh.)
    • Observed that dragging the blue handles added and removed lanes as expected.
  • Developer described the following issue: "Clicking on a RoadSegment to grab a RoadPoint results in thereafter not being able to use a RoadPoints's built in 3D transform gizmo. Everything gets treated as a drag."
    • Unable to confirm or recreate this issue. When a segment was clicked, the nearest RoadPoint became selected. The RoadPoint's transform widgets then behaved as expected. Rotate, Translate, and Add/Remove lanes all worked.
    • Observed two unusual, but not necessarily unexpected behaviors when click-dragging.
    • First, if user click-dragged on the surface of a segment, a dragbox appeared and the selected item became deselected. One might have expected a different result. But, this outcome seemed normal and acceptable.
    • Second, if user click-dragged an unselected RoadPoint selector, then it remained unselected and the selected RoadPoint was dragged, instead.
    • One might expect a RoadPoint to become selected by a click-drag on its selector element. However, it only became selected with a click-release. Once the mouse button was released, the RoadPoint became selected. Then, it could be click dragged.
    • To the best of my knowledge, the developer had limited control of this behavior since some of it was pre-determined by Godot's internal processes. Some events were triggered on mousedown and others on mouseup. For instance: In the scene dock, the visual selection changed on mousedown. But, the "selection changed notification" may not have occured until mouseup.
    • This odd behavior was observeable in the 3D viewport. When changing selections in the Scene dock, the built-in 3D transform widget jumped immediately to the new selection on mousedown. But, the plugin widget didn't move to the new selection until mouseup.

Recommendations/Suggestions:

  • Update RoadContainer warning message behavior so that the warning goes away consistently when RoadPoints are added.
  • Add tooltips to buttons on "RoadPoint toolbar" in 3D viewport.
  • Review and update behavior of Add/Connect RoadPoint button. Sometimes blue connector line isn't shown when it should be. Sometimes red disconnector line is shown when it shouldn't be or it doesn't perform the expected disconnection between a length of several RoadPoints.
  • Review behavior of RoadLane toolbar. Insure that Delete button works as expected. Consider hiding RoadPoint toolbar when a RoadLane is selected.

Copy link
Collaborator

@bdog2112 bdog2112 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(See above notes.)

@TheDuckCow
Copy link
Owner Author

Thanks a ton @bdog2112 for the detailed review! A number of good points and identifies some things I didn't consider would be confusing (like the road panel being active with RoadLanes selected) but you're totally right, they are not meant to work together at all.

I'll be going through all this feedback and making according changes. On the point of RoadLanes, just to be clear the point of having them in the menu is only so that they are quick and the user is aware they exist. There's no "correct" way of using them really, it's up to the developer to make any use of them at all. They just provide a path which has a canonical "forward" direction and a visualizer to match.

As for the warning triangle notification: I added the convenience "check if need refreshing" function but seems to be overly picky right now, needing extra refreshes when they aren't actually needed. Not sure if I'll have that resolved this round, and generally I'd prefer it was over-complaining that refreshes were needed rather than not warn of meshes being outdated, but indeed a goal to have it improved.

Will seek to improve what I can before the end of this weekend and hopefully we'll be in good-enough shape to call 0.4.0 a launch.

@TheDuckCow
Copy link
Owner Author

Fixes addressed with recent commits:

  • Adding 3 RP's then disconnecting the last one created a bad state. Was due to a typo in the "dot" function
  • Toolbar hides when RoadLanes are selected
  • Toolbar now has tooltips for all functions including sub menus
  • Can now delete a RoadPoint if the delete mode is active and the current selection is unconnected to anything (unless you hover over another area that is connected, or your mouse is too far away). Though this still isn't that convenient that you'd have to select said point first.

Open issues in my mind not yet addressed, and may be better for a v0.4.1 release:

  • Improving the delete function to actually run as a dissolve, although this is the most egregious issue and leaves many errors and even a bad state of nodes in the editor.
  • Attempt to refresh RoadPoint handles at the same time the gizmo refreshes.
  • Triangle warning to refresh being too aggressive, doing some investigation but not resolved yet

@TheDuckCow TheDuckCow linked an issue Oct 24, 2023 that may be closed by this pull request
4 tasks
@TheDuckCow
Copy link
Owner Author

Now addressed: a hack but end-user very functional and reliable dissolve function. Going to merge this in the meantime and then work on making connections between different RoadSegments after.

@TheDuckCow TheDuckCow merged commit e2e406a into dev Oct 27, 2023
@TheDuckCow TheDuckCow deleted the connection-tools branch October 27, 2023 13:55
TheDuckCow added a commit that referenced this pull request Nov 23, 2023
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.

Create tool to connect RoadPoints and RoadContainers Add, delete, and dissolve roadpoint buttons in 3D menu
2 participants