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

Construct road-edge curves #135

Closed
TheDuckCow opened this issue Nov 19, 2023 · 12 comments · Fixed by #156
Closed

Construct road-edge curves #135

TheDuckCow opened this issue Nov 19, 2023 · 12 comments · Fixed by #156
Assignees
Labels
enhancement New feature or request

Comments

@TheDuckCow
Copy link
Owner

TheDuckCow commented Nov 19, 2023

Right now, we generate RoadLanes (for AI) but we don't have any way to fetch the edge along a curve/road. Someone who wants to, for example, create a sidewalk or barrier following the edge of a road would have to place their own curve.

Let's use the same approximation mentioned here to construct one or more "Road edge curves".

For Wheel Steal Game, since RoadPoints are always added one RoadPoint at a time, we will need to be able to have these curves be per-RoadSegments. But there should also be an option/mode to create them per RoadContainer. The RoadContainer use case is likely much nicer for many users, as they can have one barrier that extends the entire road, whereas per RoadSegment they'd have to set up the configuration on each individual RoadPoint.

Some considerations:

  • In order to be useful, these generated 3D paths need to be visible in the viewport hierarchy
  • This also means great effort should go into reusing the same Path node, instead of deleting / re-adding. This is because we expect the user to place children underneath.
  • For that reason, likely the approach should be something like:
    a. If the check mark is checked, start this process
    b. See if the target curve children exist (following some naming convention); if not, create it
    c. Now update the curve resource accordingly (the safe part)
    c. If the checkbox is later unchecked, give a popup warning if there are any children to say that it will be deleted.
  • It should be an option. Perhaps by default, on for RoadContainers but off for RoadPoints.
  • The way this works for non-intersections is pretty clear. But this will be tricky to maintain, most likely, for intersections. If we really want to support use cases like sidewalks however, it will be important this generalized to intersection use cases as well, even if it's in a secondary step.
@bdog2112
Copy link
Collaborator

bdog2112 commented Feb 25, 2024

As we discussed, Edge curves will be placed on each RoadPoint. However, the "Create Edge Curves" flag that enables/disables them will be on the RoadContainer.

Per the requirements, if Create Edge Curves is disabled, it it will clear all of the edge curves on scene load or when a rebuild is triggered.

Also, the logic that drives much of the program behavior is such that only the selected RoadPoint gets updated when a change is made. Not the entire network. Hence, edge curves only get created for RoadPoints that are updated in some way (and on scene load). This may or may not be the desired behavior. Feel free to weigh in.

Lastly, since each RoadPoint will have a left and a right curve, they will be named "edge_L" and "edge_R". They will be unique on a per RoadPoint basis. Thus, complex naming isn't needed.

@bdog2112
Copy link
Collaborator

Minor update: The edge curves will actually be called "edge_F" and "edge_R" for "Forward" and "Reverse".

@TheDuckCow
Copy link
Owner Author

Hey @bdog2112! Thanks for laying out the additional details here. Agreed on all points above, including the naming. I appreciate the revision there, since that nicely parallels the pre-existing terminology. Couple things to upack a little further:

Per the requirements, if Create Edge Curves is disabled, it it will clear all of the edge curves on scene load or when a rebuild is triggered.

To be more explicit, let's go with:

  1. If "Create Edge Curves" is disabled, we can outright delete the Path3D nodes (which would delete all children as well).
  2. If "Create Edge Curves" is enabled, and any rebuild is triggered (via a roadpoint update, container-wide rebuild), we should only update the curve reference within the pre-existing Path3D nodes. This is because users will be manually placing children on these road edges, we don't want to delete their work!

Another thought I had is whether we should try to create some kind of warning or popup if a user tries to disable edges on a container when such edges already have pre-existing children. This would probably be tricky, especially since we'll still need to use export vars. So, I'm shelving that idea for now but something to consider in the future.

One other thing to watch out for is how the behavior works with undo (and redo). There's already some cases today that aren't ideal (bug here: #84) so we could potentially just tack onto that if we have new problems arise. So, don't fret if it's not perfect out of the box.

@TheDuckCow
Copy link
Owner Author

Ah I'm now realizing I already wrote much of this in the above description - anyhow, main priority is getting the reusable edges in place, other polishing features like popups and undo/redo we can take in a second stride.

@bdog2112
Copy link
Collaborator

bdog2112 commented Mar 1, 2024

Need some input on desired curve behavior when manipulating RoadPoints.

During RoadPoint Rotation/Translation (manipulation), RoadLanes are cleared. But, when mouse button is released, RoadLanes are rebuilt. Meanwhile, road geometry is updated throughout the manipulation process. What is the desired behavior for RoadEdges?

  1. Hide on RoadPoint manipulation start. Then, show on manipulation end.
  2. Update continuously during RoadPoint manipulation.
  3. Leave visible during RoadPoint manipulation. But, don't update until manipulation end.

RoadEdges will have attached objects. Hence, the above alternatives are applicable, there, as well. What is the desired behavior for attached objects?

For the moment, I will proceed with option 1 for RoadEdges and option 3 for attached objects.

FYI: Attached objects, more or less, follow the starting RoadPoint. Hence, no further action is needed for them unless an alternative behavior is desired.

@TheDuckCow
Copy link
Owner Author

TheDuckCow commented Mar 1, 2024 via email

@bdog2112
Copy link
Collaborator

bdog2112 commented Mar 2, 2024

Alright! Just pushed an update. Here are the deets:

  • When user selects RoadContainer in Scene dock and toggles "Create Edge Curves" in Inspector panel, edge curves update automatically. (Before, user had to save and reload scene.)
  • Curves update continuously when RoadPoints are manipulated
  • Curves are persistent (no longer re-created) during manipulation
  • Attached objects are persistent unless "Create Edge Curves" is toggled
  • When edge curves are deleted, curve children are also explicitly deleted to avoid orphans

TTD:

  • Adjust curve position based on number of lanes.

@bdog2112
Copy link
Collaborator

bdog2112 commented Mar 2, 2024

Curves appear fine on first RoadSegment. But, something is wrong on subsequent segments. Investigating...

@TheDuckCow
Copy link
Owner Author

Nice update @bdog2112! Had fun playing around with it here :D

demow.working.CSG.mp4

Only comment is it seems there might be some global to local mixup (I guess that's the godot equivalent of "off by 1"), see here, just in case you weren't already aware. Really excited to see this working once we have intersection offsets accounted for, making great progress.

demo.transform.issue.mp4

@bdog2112
Copy link
Collaborator

bdog2112 commented Mar 2, 2024

Pushed latest WIP. Previously, refactored code so that it would update instead of add curve points if they already existed. Determined that a couple of local/global coordinate conversions were needed in the refactored code. Curves should now plot as expected.

@TheDuckCow TheDuckCow linked a pull request Mar 2, 2024 that will close this issue
@TheDuckCow
Copy link
Owner Author

Nice - we can start moving the discussion more into the PR itself now, so we don't have to post in both locations - my latest comment (and future ones) will be there

@TheDuckCow
Copy link
Owner Author

I'm going to mark this task as done, since they are working in dev to spec. Good work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants