-
-
Notifications
You must be signed in to change notification settings - Fork 50
WIP: actors evade collision #260
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
base: dev
Are you sure you want to change the base?
Conversation
|
@TheDuckCow @NoJoseJose I would appreciate your thoughts on this |
TheDuckCow
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't say I have a perfect idea in my head of everything you've pulled together, but a few thoughts as request to spark some discussion:
- Firstly, I like and agree with the objective of creating a lightweight way of letting agents move around without collisions. We'll have to make some tradeoffs of course, but I think it's off to a good start
- Acknowledging that we basically will depend on smart lane greenlight/redlight management to avoid intersections at, well, intersections. But that's ok
- Seems we have to think about edge cases at points where one lane merges into another (or multiple collapsing at once into fewer lanes)
- I also have a mild feeling that we could come up with a slightly simpler solution, but that's perhaps naive on my side since I haven't gone into the weeds trying to make this change myself. Mostly pointing to the creation of
LanePositionandto_lane_sideandflip_side(is that last one even used anywhere?) - That being said, we could also think about an alternative as to what is being tracked. In terms of the high level approach, what do you think of this?:
- What if instead of each actor trying to keep track of each lane and each lane's cars etc, instead each vehicle just tracked it's "next" vehicle in its motion direction? This bakes in the assumption that one car is likely only to slow down in reaction to another car, and thus generally only has one car to care about (the one it would collide with next in front of itself).
- Then, you're just tracking the distance between you and this next vehicle (even if that goes over a new road lane transition as prior goes to next)
- Then, the other key thing missing is the concept of the vehicle length. Could have two more vars for distance m in front of the center point and behind (something we could create some utilities to help auto calculate, or at least visualize as a sort of 3D shape in the editor one day with a gizmo to control it).
- When it comes to changing lanes OR multiple lanes merging together and a vehicle needing to decide "if there's space", it can do the check for distance between vehicles but considering the front / back bumper distances, so that cars aren't treated as point masses. So it's kind of like a zigzag or binary tree rebalancing in a way, where a lane merge just inserts or changes the linked list of cars
- As an example, imaging we have a LeftLane with the front bumper of CarA being 7 meters behind the rear bumper of CarB, and CarC in RightLane is looking to merge left in between CarA and CarB. If CarC has a length of 3meters, then it means there is in theory a range of 4 meters at which any point CarC could change to the left lane between CarA and CarB (ie such that CarC's rear bumper ends up still in front of CarA's front bumper, and so forth). Then when performing or initiating the merge, presuming one day we'll 'lerp' the transition more, it could trigger the routines on CarC, itself, and CarB to refresh/find what is the new "next" car and thus resetting any logic associated with that (such as trying to keep minimum distances at certain speeds etc).
- In fact this even creates the idea of straddling lanes, maybe a car SHOULD be registered to multiple lanes at once to ensure they can be considered taking up space in both during transitions or lane changing, or even as another way to handle intersections? And then just remove the CarC in this example from the old lane once fully moved over.
- My thought is having each agent just track at most one other vehicle (fetching it from a Road lane only when it can no longer track/find said vehicle based on registered vehicles in that lane) would simplify the number of checks and logic being done per frame, and scales just linearly with the number of vehicles
- New acceleration model: I like the idea, will need to tweak the defaults but I see the export vars still work. I do find the current model doesn't really have drag and seems to coast, I kind of liked how we had it auto slowing down, but really that's personal preferences and any dev could adjust to their own needs.
All just ideas, let me know what you think!
| set(val): side_lanes[LaneSideways.RIGHT] = val | ||
|
|
||
|
|
||
| var adjacent_lanes: Array[NodePath] = ["", ""] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this variable name is more intuitive? Adjacent to me would also mean to the left and right
| var adjacent_lanes: Array[NodePath] = ["", ""] | |
| var connected_lanes: Array[NodePath] = ["", ""] |
(not that we need to be deep diving into nit picks like this right away)
(with comments I make later, I'm less sure that connected_ is the right variable name here, we can word smith it later)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I'm not too happy with namings I have either:)
| func register_vehicle(vehicle: Node) -> void: | ||
| _vehicles_in_lane.append(vehicle) | ||
| func is_agent_list_correct(): | ||
| return true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just calling out this is exiting early, is there something more you're hoping to do here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:) just forgot to enable it back after playing around without a sanity check, good catch
| for dir in RoadLane.LaneDirection.values(): | ||
| var dir_back := RoadLane.flip_dir(dir) | ||
| var lane_next := lane.get_adjacent_lane(dir) | ||
| if lane_next && lane_next.adjacent_lanes[dir_back] == lane_next.get_path_to(lane): | ||
| lane_next.adjacent_lanes[dir_back] = NodePath("") | ||
| lane.queue_free() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll admit I'm a little confused as to what's going on here - are we actually flipping the orientation of the lane here...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no RoadLane.flip_dir(dir) (static function) is just flipping dir enum (so essentially 1 becomes 0 and vice versa), the helper function is I think not really necessary. not sure if I should make it 1-dir (at least in flip_dir we could add some checks), but yeah I'm just trying to see how the code would feel better, naming definitely should be better
| for _lane in new_lanes: | ||
| var rand_offset = randf() * _lane.curve.get_baked_length() | ||
| for _lane: RoadLane in new_lanes: | ||
| var rand_offset = randf() * (_lane.curve.get_baked_length() - before_end) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this essentially creating a no-spawn margin at the ends of lanes? Makes sense this would be a simpler solution, though could result in "seams" in traffic being generated. A more generalized solution would aim to check eligible spawning based on the length of the vehicle and how far forward/back beyond the actor point the vehicle actually needs space for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed, more of a workaround to not create colliding agents for now
|
|
||
| ## For more info see Intelligent driver model | ||
| ## https://en.wikipedia.org/wiki/Intelligent_driver_model | ||
| func compute_idm_acceleration(next_agent: RoadLaneAgent) -> float: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat! Though I haven't done the full amount of reading here, seems like a nice idea to bring a good model in place. Should this depend on an input delta though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no the idea as far as I understood - delta in this case is constant for kind of emulating acceleration/deceleration profile
it's recommended to be 4 but we could play around with it (and other actor's characteristics) to make it look more realistic
| # Get another point a little further in front for orientation seeking, | ||
| # without actually moving the vehicle (ie don't update the assign lane | ||
| # if this margin puts us into the next lane in front) | ||
| var orientation:Vector3 = agent.test_move_along_lane(0.05) | ||
| var orientation:Vector3 = agent.test_move_along_lane(rotate_to_distance) | ||
|
|
||
| if ! global_transform.origin.is_equal_approx(orientation): | ||
| look_at(orientation, Vector3.UP) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if you want to tackle this right now, but I recently realized... we really don't need to do this whole secondary check for orientation. There's a curve function sample_baked with_rotation returns a transform3D which should be able to give us the rotation information we are interested in. Saving a call to test_move_along_lane is probably a pretty decent speed improvement (the function is still worth keeping to be clear, but wouldn't be necessary in this demo code anymore).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh nice. will do that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on the other hand, I think it's less of a problem with agent remembering offset move along lane shouldn't be too heavy
and results should be marginally better it we calculate angle from the positions of front and back wheels on lane
I also thinking that it may be worth it to calculate distance to agent on lane instead of using distance_to
| # this container should contain agents in order: | ||
| # from beginning to the end, i.e. agents' lower offset to higher offset | ||
| # also offsets are expected but not enforced to be unique | ||
| var _agents_in_lane: Array[RoadLaneAgent] = [] # Registration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess a change in behavior we'd have to call out is that the "registered" entity is the agent instead of the agent's actor, just a note to self that this must be called out in the changelogs if we go with this solution. I do think it ends up being nicely more consistent in the end while remaining flexible, since someone can define the lane agent's actor reference to be anything that makes sense to them (if not just the direct parent)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah my thoughts exactly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just posting this video as something I ran into, perhaps one of the edge cases you're already thinking of is what's happened here:
- getting stuck when you overtake a car in front of you (that kind of makes sense though)
- Other agents getting stuck as multiple lanes merge together
- Assertion failure at the end as you can see
agent.collision.prevention.mp4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I'm thinking that obviously player have to have control over who to crach into and all that
I would say that in general case, player shouldn't use move along lane, and plugin's user maybe should register player on all the lanes it blocks so other cars would not crash into them - somehow I'm not planning to work on that
for my usecase I plan to have player on ai lane, which is very close to procedural generation demo, so I'm thinking of adding a path for player to ignore collisions (or remove actors they collide with)
yeah lane merging doesn't work yet. but generally speaking i plan to make it work the same way as normal lane change
assertion is because of the collision on lane the code doesn't expect multiple agents at the same offset (same point on the lane) currently. but I think I'll have to deal with that somehow later on - especially with virtual lane agents
| assert(self.is_agent_list_correct()) | ||
| assert(agent not in _agents_in_lane) | ||
| assert(agent.lane_pos) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per before, would like to think what we need to do in a production sense if these ever fail. Guessing it should be push error outputs or something like that, with a more graceful exit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on one hand I wouldn't want to have heavy checks like is_agent_list_correct in production. on the other, I suppose debugging players' problems with error logs (that I don't really do much normally) need to be more of a priority for me. some of the asserts should be errors, that's for sure
| agent.link_next_agent(self._agents_in_lane[idx + (1 if dir == LaneDirection.FORWARD else -1)], dir) | ||
|
|
||
|
|
||
| func find_adjacent_agents(agent: RoadLaneAgent) -> void: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per comment above, I read this as more about following the lane forward/backwards, as opposed to left right. Verb-func wise, find_ would imply it returns a value. I suppose this is more like update_{sequential? connected? inline?}_agents
Now that naming nit-pick aside, maybe this is also where we'd need to recognize when there are multiple lanes merging together..?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed about naming
as for lanes merging together I'm not yet completely sure how to make it right. this function is more about finding someone front/back of agent if no-one is on the lane it drives on during lane change, or if an agent is moving and doesn't know if anybody is in front of it
Yeah I didn't think much about intersections but maybe there could be a regulated thing (cyclically changing which lanes are on and which are off) and also unregulated thing where agent moving on one lane blocks another lane automatically (or as you proposed, just adds a static actor at the intersection point? but then we also need kind of width of actor to decide how much space it should take. something to think of)
this one I'm thinking about and I'm thinking of making it more of a normal merge in some way. but with more traffic I'm thinking of maybe some managing code to decide who goes next
I'm not sure about LanePosition myself. I was unsure about race conditions (mentioned in lane registration). but looking into it, I think at least with a normal scenario it shouldn't be possible. so I'm thinking of getting rid of it - and just adding offset field to the agent
Any ideas are definitely welcome:)
Yeah I think I'll do that and forbid non-player actors to move backward for now (as it's kind of an edge case), I don't use backward link much anyway
Do we want to have it in agent or in actor? it kind of make sense to have prolonged agent to analyze ai lanes' usage and it would definitely simplify things for agent logic. On the same note I was actually thinking of moving agent's position to the front of an actor. this way we only need to store one size variable and it would also stop automatically at the lane's end for example (but moving backward it won't be automatic) though I suppose rotation of the vehicle would be a bit worse if we use position on the front to pick for direction
yeah I also think something along the lines. For now I'm thinking of maybe just putting them all (virtually) in the lane they're merging into and see if they will figure out themselves. I think we could improve based on these results
yeah I was thinking to make merging/lane change work like that (with multiple virtual and one real agent). It's a good idea to try and do something similar for intersections. we may need some tricks to figure out when to create and when to remove them but I don't see it being too much of a problem.
yeah I would say if agents are not ever moving backward, looking only forward makes sense. I'll think about it
yeah I also want player's auto slowing down back (and maybe heavier breaking on pressing down). will look at how it can be done |
|
Some more thoughts for ya, good discussion points indeed:
Though there's always a risk of deadlocking, I tend to like the options of letting individual agents decide for themselves what to do. As a hypothetical more sophisticated example of this, in an intersection, we can provide the input that a lane is "disabled" (red light), but it's up to the agent to actually stop (or maybe they eased into the intersection for a left turn, they could decide to still go for it once oncoming traffic is cleared). So on that basis, I'm more in favor of having the vehicles decide amongst themselves how to merge. That logic for this level of behavior should definitely be in the road actor/procedural scene, since there's no single correct way to determine this, so our implementation there would just be an example.
Yeah I feel that could simplify things with the same end benefit. And if we can essentially cache an agents offset from a prior test/move, then it saves from processing later. In fact, that makes the physical space calculation rather easy because offset is already a figure in meters. the distance between two agents is
I'm fine with such a restriction for now if it makes things easier/simpler, this code is just demo anyways so it's ok for us to call out restrictions to make the demo as simple as possible.
I'd suggest the property to be on the agent as it would indeed be useful for other purposes I also mentally contemplated putting the offset at one side or the other, but then you still need the size input for how far backwards to read the next vehicle, and turning the agent into a spatial feels... well overkill is a strong word, but I worry even in my own games about the number of nodes as each one has overhead. As just reference as it is now, it remains a bit lighter weight and can rely on the transform of the 3D nodes that inevitably already exist.
I see it as more like a "promise" that the agent holds to release its virtual registration at some point, that way nobody needs to take care of it other than the vehicle itself. I'm not as in favor of creating new agents on the fly or duplicate paths or anything like that because it becomes quite a bit of overhead (every new node added to the scene adds up), but that does raise the question of how we keep track of the "virtual" registrations and their offsets. Could be another array of virtual registrations which could be an array of [lane, offset] pairs. Or, it could be your lane offset class you created already I suppose (though that becomes some class memory overhead, not as much as a node however).
(my above point also address this part from your original post): This level of behavior nuance also starts to get into the weeds of traffic simulation. I want to avoid forcing the RoadLaneAgent into a one size fits all solution. It's not meant to be "traffic working out of the box", but rather just provide the tools to make it easy to implement traffic. Then, the demo scenes are where we provide potentially different example implementations. At some point, I want to have a carbody physics based example to contrast our "on rails" solution. So, I'm a bit torn how much of this specific merging like logic should be in the road lane agent (which is provided to all road generator users whether they like it or not, and thus making it more complicated to understand), or trying to do more of this logic in the road actor and potentially keeping track of additional lanes there. Another idea could be that the road actor scene actually creates a new inherited version of the road lane agent that adds additional variables, showing people they have their own way of extending it further if they want. Just ideas, depending on where we go. In terms of what should go into the RoadLaneActor vs not, I'd say: utilities or properties that would be universal for any traffic system are fine, but workflow specific ones should be pushed into the demo files. Length of a vehicle is a pretty universal thing to keep track of, but the exact way and who decides to go during a zipper merge is better for the demo code (since there are many strategies to avoid deadlocking where traffic just gets jammed or one lane never gets to go).
👍 cool, but super low priority compared to the rest which is the more functional parts.
Since I didn't respond to this part before, I think it could be interesting to have a pure no- collider demo example, knowing that I intend at some point to have collider/carbody based counter demo at some point too. I would suggest we try to keep each individual demo as simple as is reasonably possible, so it makes it easier for someone looking at the code better understand how it all fits together. One day we could always have a third "super demo" which is something like combining both concepts together (cars on rails in the distance, but simulated up close to the camera with collisions) |
I think we could ask users to just place actor geometry with a transformation that makes beginning/front wheels the position for actor (and so agent). but two additional offsets are fine as well, I think it gives some flexibility without much complexity/resource demand from us
oh yeah agents should track/remove their virtual agents when it doesn't need them anymore, that's for sure.
ok that's fair, I'm fine either way. I'd argue that some "dead" (like not used in a specific project) code is not too bad, if it is marked as used for specific stuff and could be "widely" applicable to other projects.
yeah I don't think of adding anything physics related in this PR, just thoughts. sounds good. could you also look at how you want to achieve intersection/multi-container lane linkage? I don't mind trying to implement that when I get to it - ideally in this PR, as without it everything's going to kind of break on this transition - but I didn't look at the idea of how it should be done |
|
@TheDuckCow ah I remembered why I wanted to have both forward and backward links - in case if traffic comes to stop, we could put actors to sleep and when the actor in front of them starts moving again, they would wake them up but I think we should use signals anyway. if it's not too big an overhead Ah I also have somewhat a problem with disconnection. I'll need to search for an agent backward when leaving lane. |
56227b6 to
5ab75af
Compare
|
changed the idea, now tracking of obstacles is only done in lanes |
|
the problem was about reusing the actors (not cleaning up the speed) and it looks like IDM has some problem where it can get to negative speed and just unable to get out of it. updated it now with just speed clamping |
|
i still see some actors moving backward seemingly without any reason and are not trying to stop. will investigate it |
e822150 to
e14ebf2
Compare
|
for the future reference, here is a chapter from traffic simulation book about following models |
0f976a1 to
8a1acc8
Compare
|
rebased to the latest master. while attempting the second take on traffic, I discovered that I want to merge quite a bit of changes from this branch, so currently I plan to continue working on this branch and just roll back road lane shared part stuff maybe it would be better to merge kind of refactoring and general improvements merge here and do the actual traffic in another one Or I'll just rework how obstacles are being searched in this PR. not decided yet |
started preparation for agent blocking multiple lanes separated road lane obstacle from road lane agent separated move_along_lane to an object - may need to use more than one removed agent links to next/prior added end offsets to obstacles
8a1acc8 to
0811c0b
Compare
|
forgot that we actually have dev branch. rebased there instead |
|
removed my implementation of shared part and everything related to obstacle search excluding some stubs where I plan to try to implement the proposed traffic search/decision making in the next commit |
TheDuckCow
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a variety of comments and thoughts, as mentioned might be good to have a followup sync at some point, but hopefully this gives you enough direction for now.
| class Obstacle: | ||
| enum ObstacleFlags { | ||
| REAL = 0x0, # the node is on this lane | ||
| IMMINENT = 0x1, # the node from anothe lane won't be able to stop before it gets to this position |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| IMMINENT = 0x1, # the node from anothe lane won't be able to stop before it gets to this position | |
| IMMINENT = 0x1, # the node from another lane won't be able to stop before it gets to this position |
(also note to self, I'm curious how this is treated differently to INTENT, as it sounds like it's more to due with how agents interpret the "intent" of soon to be in this lane)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the idea here is that actor will be unable to stop before it gets to this point
intent is like "will you allow me to merge before you?", but I think it's more reasonable to make actor being able to ask for permission with a function in actor itself
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah seems reasonable to me that within any given traffic system, you could make it work. In real life, it's ultimately always going to be the judgement of the driver whether the other driver is going to yield or not (regardless of laws) and so seems reasonable that INTENT, PARTIAL, and REAL would cover the actual feasible states.
If though it really complicates your approach, then we could still have the IMMINENT option, so long as it's ultimately the road actors that define the state and not the agent code itself, since it's highly dependent on how the traffic solution is working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't need it right now, probably will be necessary when we get to traffic on intersections, but I'll leave it be for now.
| enum ObstacleFlags { | ||
| REAL = 0x0, # the node is on this lane | ||
| IMMINENT = 0x1, # the node from anothe lane won't be able to stop before it gets to this position | ||
| PARTIAL = 0x2, # the node from another lane but it partially blocks this lane |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| PARTIAL = 0x2, # the node from another lane but it partially blocks this lane | |
| PARTIAL = 0x2, # the node is from another lane but it partially blocks this lane |
| # ------------------------------------------------------------------------------ | ||
|
|
||
|
|
||
| const DISABLE_HEAVY_CKECKS := false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick to use positive framing (ie "do check: true" reads cleaner than "don't do check: false". Also, is this a debug only feature, or would this be important for some gamedev use cases compared to others?
| const DISABLE_HEAVY_CKECKS := false | |
| const ENABLE_HEAVY_CKECKS := true |
| get: return sequential_lanes[MoveDir.FORWARD] | ||
| set(val): assert(get_node_or_null(val) != self); sequential_lanes[MoveDir.FORWARD] = val |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| get: return sequential_lanes[MoveDir.FORWARD] | |
| set(val): assert(get_node_or_null(val) != self); sequential_lanes[MoveDir.FORWARD] = val | |
| get: | |
| return sequential_lanes[MoveDir.FORWARD] | |
| set(val): | |
| assert(get_node_or_null(val) != self) | |
| sequential_lanes[MoveDir.FORWARD] = val |
Better to have the setter on multiple lines, I'd rather not use ;
| set(val): assert(get_node_or_null(val) != self); side_lanes[SideDir.RIGHT] = val | ||
|
|
||
|
|
||
| var sequential_lanes: Array[NodePath] = ["", ""] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this default value actually be an empty array to begin with?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's easier to add elements right away to be able to read and write sequential_lanes[0] or sequential_lanes[1] at any point - as 2 links to lanes forward and backward. i think the losses are minimal here (as most lanes are going to be connected anyway), but it's just easier to work with
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo it should be a valid case that a lane has an explicit end and can't go forward, e.g. a road that dead ends. Though it's a bit more annoying to have to check before using an index every time, seems like a safer pattern
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure I understand, do you mean i should initialize it with null or something else?
currently it's the same way as we had it before - so the RoadLane has next_lane and prior_lane, they are always there. but I changed the way they are implemented with an array so we could access them by index. but to be able to access prior/next lane (with actual value or null) we need it to be initialized with 2 elements.
if we start without elements then we ned to add elements as we go and if we want only prior (index 1) link than we need to initialize next (index 0) as well
| if is_instance_valid(_vehicle): | ||
| _vehicle.call_deferred("queue_free") | ||
| for obstable in obstacles: | ||
| if is_instance_valid(obstable): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Food for thought - should each obstacle have its own handler/flag for whether to auto free? Thinking of use cases where maybe a traffic system wants to treat the player as an obstacle to keep track of and have traffic respond to, but should never be auto freed. Maybe the obstacle class itself should have an enum for "queue free, don't free, inherent from roadlane setting" or something like that.
Also for integrating this feature, I'd say it's fair to rename flags rather than have a mix of terminology - so auto_free_vehicles becomes auto_free_obstacles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not completely sure about terminology as well, but obstacles are just a pointer-counted object that is a part of RoadLaneAgent. and I think when RoadLane wants to auto-free, it actually frees RoadActor (but can potentially also free somehting else if obstacle is being created not by an actor, instead some static block)
I'm not completely sure how well auto-free works right now with my changes though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think obstacle is a fine general name. While "vehicle" made be a bit more obvious to some people if, it still makes sense we end up calling it an obstacle. One question though - if we wanted to make a stationary object, do we still always have to attach a roadlaneagent? Would we need some kind of container called roadlanestatic or something?
No worries on functionality, we can do fullter tester when it's further along - the enum option was just an idea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in my head user don't always need a RoadLaneAgent, instead they can just place an Obstacle object on lane (though they'll have to do it themself) - I think that should be kind of the normal interface object to seek for, well, any obstacles on the road
| return -1.0 | ||
|
|
||
|
|
||
| func connect_segment_lanes() -> void: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self that this block is another reason to ensure/validate we have normalized roads, as two RoadPoints facing each other will cause problems here among other places (can ahve two child segments for one RoadPoint)
| if abs(velocity.z) < 0.025: | ||
| velocity.z = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we need our own concept similar to rigid body sleeping, which includes an actor's export var for sleep time + threshold (idea only, not a mandate)
|
|
||
| ## simple RoadLane with override to despawn anyone assigned to it | ||
| class DespawnRoadLane extends RoadLane: | ||
| #TODO should be @tool? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine if you make this a tool, especially being in the demo folders
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating a file-level comment to do more the high level reply here, other comments were smaller focussed things I saw just while scrolling through.
- As we've previously discussed, similar to Godot itself, I'd like us to continue to provide base building blocks so that individual devs can create the workflows that work best for them
- Core code is common to everyone and implements the base building blocks that we can generally expect all users likely need access to (and if not, can be disabled via flags in the addon core for performance gains only, such as enabling/disabling road lanes as we have right now).
- Connectors are pieces of code that are not core to the addon, but largely should be handled as optional features that works tightly together "gluing" together parts of the addon, using only the public interfaces (making it feasible again for someone to fork). Should be recommended they can use in production if they like, while also safe to mess with to modify a copied version of any given file here. Generally should not be too specific or narrow to individual use cases, they try to be as general as possible while acknowledging that not everyone will use them anyways.
- Finally, demo scenes are those which need to make narrowed use case decisions to properly get a desired effect. These are more fully flushed out demo implementations that people could run with or fork for their projects from the demo folder, but don't translate well to other use cases necessarily.
- With that context, I think it helps us figure out where things below. In this context, I see:
- Obstacles: Should be a core building block, information that higher level systems can use. And thinking about it more, I think it makes sense to leave as a class available on RoadLane (we could potentially break it out into its own file and just reference the class as a gdscript const in the RoadLane, but leaving it where iti s for now I think is fine)
- Logic and priority on how to combine agents: this feels more workflow specific or use case specific, and may be overhead for people who don't need it. Thus, more demo folder level
- Traffic manager: if it can be made general purpose, such that it doesn't rely hyper specifically on the demo folder implementation of a road actor, but rather anything that contains its own reference to a road agent, then I could see including traffic manager under the connectors folder. Otherwise, would remain in the demo folder being very workflow specific. Same could go for the spawner.
- I'm slightly weary some of the more specific attributes like merging priority - however, having reviewed, I suppose the argument holds that this is more annotating data about the road lane than defining how road lanes should be used, so fair to keep
- I do have a slight worry about this making it more complciated to hand author custom road_lanes, if someone needs to make their own overrides.
- Likewise, maybe I just need a follow up live call to see how we think mappings will apply especially soon as we have intersections "around the corner" - I just need to convince myself we're coming up with a good general solution, not saying it isn't already.
- If someone is using roadlane agents/road lanes, but decides to not use the demo traffic manager or agent and create their own (e.g. rigid body racast-based steering actor) - do you see your solution adding extra overhead / wasted processing power that wouldn't be relevant? Or is it just extra properties that are read but not used during runtime really unless the given traffic manager is used?
Structural thoughts:
/connectors
terrain3d_road_connector.gd
traffic_manager.gd # could export var ref a traffic spawner to further config
traffic_spawner.gd
/nodes # all "public", editor-visible scenetree nodes
road_*.gd
/procgen # coming in the intersections feature branch, utilities used by /nodes/road_*.gd
intersection_*.gd
segment.gd # will create this later, making road_segment.gd leaner
/demo
/common
road_actor_rails.gd # ie our current roadlane_actor
road_actor_rigid.gd # an example I hope to create soon-ish
traffic_manager.gd # if not a connector
/procedural_generator
# references from the common folder
/intersections
# references from the common folder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds reasonable to me. I think if RoadLaneAgent is not used (and so obstacles are not used) then the only overhead in the new approach is going to be the search array for next obstacle on lane. again, that, I think can be moved to trafficmanager (as a map from road lane to array) but then we need to keep track of lane changes (geometry or sequiential links) there as well
another approach could be - in the stuff that I do now (not yet committed), I use a constant for the length of the chunk (and so how many pointers the search array has), but I think I can move it to RoadManager and if length is 0 then don't initialize the array, and the only overhead this way is going to be an empty array in RoadLane
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great - and yes, if we can make the call for populating/using the search array for the next obstacle be a public method that something such as a road actor or traffic manager itself makes calls to, I think that works well and aligns with the philosophy. Rich functionality in the roadlane agent but up to the dev's choice on how to use it or which functions are relevant for their use case, much like a character body controller has several similar but different functions to cover different needs.
If you think it's complicated to track in the road manager, that's totally fine, I'd just put the calls in the road actor instead of the agent. If there are other properties that are static/belong to specific pieces of roads that we need to track then yes, using or saving on the roadmanager could work. At some point, I'm still expecting the road manager to have a purely in-memory representation of the road network for pathfinding and lod loading purposes.
Here is my proposal for agents
The idea is to try and make agents look reasonably intelligent and interract with each other using only AI lanes.
So I would want to do that that without a need for physics and colliders.
How I'm trying to do it:
I'm also thinking of a case for changing lane continuously. I'm thinking of creating temporary personal lane that would connect one lane and another when actor decides to change the lane. and creating 2 virtual agents for lanes it's moving from and to (or more if there are multiple transition lanes it intersects). this way other actors will know that somebody is getting on their lane and will be able to evade them
as for colliders - I'm thinking along the lines that all of them should have a collider that is not checked against each other and the road. then we add a collider of other group to player (and maybe other disrupting actors), and if they collide with normal actors, we change collision group and they'll be able to collide with other actors, propagating this collision group change
what doesn't work yet: