TheDuckCow / godot-road-generator

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

Establishing Connection tools #131

Closed TheDuckCow closed 8 months ago

TheDuckCow commented 8 months ago

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.

TheDuckCow commented 8 months ago

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 commented 8 months ago

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:

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).

TheDuckCow commented 8 months ago

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 commented 8 months ago

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.

https://github.com/TheDuckCow/godot-road-generator/assets/2958461/e73f5f3d-bf9c-4162-8081-45dcb6fae2fc

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 commented 8 months ago

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:

(continued in next post)

bdog2112 commented 8 months ago
bdog2112 commented 8 months ago

Recommendations/Suggestions:

TheDuckCow commented 8 months ago

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 commented 8 months ago

Fixes addressed with recent commits:

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

TheDuckCow commented 8 months ago

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.