fix(Unreal): port signal height, trigger box, and trigger volume fixes#9653
fix(Unreal): port signal height, trigger box, and trigger volume fixes#9653youtalk wants to merge 1 commit intocarla-simulator:ue5-devfrom
Conversation
Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
|
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would update our CHANGELOG.md based on your changes. |
There was a problem hiding this comment.
Pull request overview
Ports Unreal-side fixes related to traffic signal/sign placement and trigger volume handling, aligning CARLA’s UE5 implementation with prior ue4-dev bug fixes for OpenDRIVE-generated signage.
Changes:
- Snap traffic sign/light spawn Z to ground via line trace before spawning.
- Add (currently unused) post-pass adjustment logic for already-spawned traffic signs, including per-mesh Z offsets and a positioned flag.
- Change traffic sign bounding box selection to use a single trigger volume (first) to preserve rotation rather than merging volumes.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
Unreal/CarlaUnreal/Plugins/Carla/Source/Carla/Util/BoundingBoxCalculator.cpp |
Switch traffic-sign bounding box computation to return only the first trigger volume and preserve its rotation. |
Unreal/CarlaUnreal/Plugins/Carla/Source/Carla/Traffic/TrafficSignBase.h |
Add a bPositioned flag intended to prevent repeated repositioning. |
Unreal/CarlaUnreal/Plugins/Carla/Source/Carla/Traffic/TrafficLightManager.h |
Declare new helper methods for ground-snapping and post-adjustment. |
Unreal/CarlaUnreal/Plugins/Carla/Source/Carla/Traffic/TrafficLightManager.cpp |
Apply ground-snapping during spawning; add post-adjustment routine for already spawned signs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| auto TriggerVolumes = TrafficSign->GetTriggerVolumes(); | ||
| if (TriggerVolumes.Num() > 0) | ||
| { | ||
| FBoundingBox Box = UBoundingBoxCalculator::CombineBoxes(TriggerVolumes); | ||
| UBoxComponent* FirstTriggerVolume = TriggerVolumes[0]; | ||
| FTransform Transform = Actor->GetActorTransform(); | ||
| Box.Origin = Transform.InverseTransformPosition(Box.Origin); | ||
| Box.Rotation = Transform.InverseTransformRotation(Box.Rotation.Quaternion()).Rotator(); | ||
| return Box; | ||
|
|
||
| FBoundingBox TVWorld; | ||
| TVWorld.Origin = FirstTriggerVolume->GetComponentLocation(); | ||
| TVWorld.Extent = FirstTriggerVolume->GetScaledBoxExtent(); | ||
| TVWorld.Rotation = FirstTriggerVolume->GetComponentRotation(); |
There was a problem hiding this comment.
GetTriggerVolumes() can return an array containing nullptr (e.g., when the Blueprint GetTriggerVolume() isn't implemented or returns null). In that case TriggerVolumes.Num() > 0 is true but TriggerVolumes[0] will be null and this will crash when dereferenced. Consider skipping invalid entries and falling back to GetTriggerVolume() when the first valid trigger volume isn't found.
| FBoundingBox TVWorld; | ||
| TVWorld.Origin = FirstTriggerVolume->GetComponentLocation(); | ||
| TVWorld.Extent = FirstTriggerVolume->GetScaledBoxExtent(); | ||
| TVWorld.Rotation = FirstTriggerVolume->GetComponentRotation(); | ||
|
|
||
| FBoundingBox TVLocal; | ||
| TVLocal.Origin = Transform.InverseTransformPosition(TVWorld.Origin); | ||
| TVLocal.Extent = TVWorld.Extent; | ||
| TVLocal.Rotation = Transform.InverseTransformRotation(TVWorld.Rotation.Quaternion()).Rotator(); | ||
|
|
There was a problem hiding this comment.
FBoundingBox documents Extent as being relative to its owner, but here TVLocal.Extent is copied from GetScaledBoxExtent() (world-scaled). If the actor (or any parent) has non-1 scale, the origin/rotation are converted to actor-local but the extent is not, producing an inconsistent local bounding box. Consider computing extent in actor-local space (e.g., based on relative transform / relative scale) rather than copying the world-scaled extent.
| UFUNCTION(BlueprintImplementableEvent) | ||
| UBoxComponent *GetTriggerVolume() const; | ||
|
|
||
| UPROPERTY(Category = "Traffic Sign", VisibleAnywhere, BlueprintReadOnly) |
There was a problem hiding this comment.
bPositioned is introduced as a public UPROPERTY(VisibleAnywhere, BlueprintReadOnly), but it’s only used as internal bookkeeping by ATrafficLightManager. Exposing it on the base actor makes it part of the reflected/Blueprint API surface and allows accidental changes from editor/Blueprints. Consider making it non-reflected (plain bool), moving the tracking into the manager, or at least making it private with controlled access.
| UPROPERTY(Category = "Traffic Sign", VisibleAnywhere, BlueprintReadOnly) |
| bool AdjustSignHeightToGround(FVector& SpawnLocation) const; | ||
|
|
||
| void AdjustAllSignsToHeightGround(); | ||
|
|
There was a problem hiding this comment.
AdjustAllSignsToHeightGround() is declared/defined but not called anywhere in the codebase (search shows only declaration + definition). As a result, the new bPositioned flag and the mesh-level Z adjustment path appear to be dead code and the “below ground trigger boxes” fix described in the PR may not actually run. Either invoke this at an appropriate point (e.g., after spawning/generating traffic signs) or remove it to avoid carrying unused code.
| // Get all static mesh components | ||
| TArray<UStaticMeshComponent*> StaticMeshComps; | ||
| TS->GetComponents<UStaticMeshComponent>(StaticMeshComps); | ||
|
|
||
| for (UStaticMeshComponent* MeshComp : StaticMeshComps) | ||
| { | ||
| if (!MeshComp) continue; | ||
|
|
||
| // Skip if this has a mesh parent (it's a child) | ||
| USceneComponent* ParentComp = MeshComp->GetAttachParent(); | ||
| if (ParentComp && Cast<UStaticMeshComponent>(ParentComp)) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| // Move the mesh component down | ||
| FVector CompLocation = MeshComp->GetRelativeLocation(); | ||
| CompLocation.Z += ZOffset; | ||
| MeshComp->SetRelativeLocation(CompLocation); | ||
|
|
||
| MeshComp->UpdateBounds(); | ||
|
|
||
| UE_LOG(LogCarla, Log, TEXT("Moved mesh %s by %f cm"), *TS->GetName(), ZOffset); | ||
| } |
There was a problem hiding this comment.
AdjustAllSignsToHeightGround() logs once per mesh component moved at Log level. When adjusting many traffic signs this can generate a large amount of log output (especially in editor tools / map generation). Consider lowering the verbosity (e.g., Verbose) and/or logging once per actor instead of once per component.
| float ZOffset = AdjustedLocation.Z - OriginalLocation.Z; | ||
|
|
||
| TS->GetRootComponent()->SetMobility(EComponentMobility::Movable); | ||
|
|
||
| // Get all static mesh components | ||
| TArray<UStaticMeshComponent*> StaticMeshComps; | ||
| TS->GetComponents<UStaticMeshComponent>(StaticMeshComps); | ||
|
|
||
| for (UStaticMeshComponent* MeshComp : StaticMeshComps) | ||
| { | ||
| if (!MeshComp) continue; | ||
|
|
||
| // Skip if this has a mesh parent (it's a child) | ||
| USceneComponent* ParentComp = MeshComp->GetAttachParent(); | ||
| if (ParentComp && Cast<UStaticMeshComponent>(ParentComp)) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| // Move the mesh component down | ||
| FVector CompLocation = MeshComp->GetRelativeLocation(); | ||
| CompLocation.Z += ZOffset; | ||
| MeshComp->SetRelativeLocation(CompLocation); | ||
|
|
||
| MeshComp->UpdateBounds(); | ||
|
|
||
| UE_LOG(LogCarla, Log, TEXT("Moved mesh %s by %f cm"), *TS->GetName(), ZOffset); | ||
| } | ||
|
|
||
| TS->UpdateComponentTransforms(); | ||
| TS->GetRootComponent()->SetMobility(EComponentMobility::Static); | ||
| } |
There was a problem hiding this comment.
AdjustAllSignsToHeightGround() unconditionally sets the root component mobility to Movable and then to Static. This can silently change signs that were authored with a different mobility setting. Consider capturing the original mobility and restoring it at the end (or only changing mobility when necessary).
Description
Port of 3 ue4-dev bug fixes for traffic signal placement:
Traffic signals spawned in air (
f856729ee) — AddAdjustSignHeightToGroundmethod using line trace to place traffic signs/lights at ground level instead of floating in air.Below ground trigger boxes (
c1ba7342c) — ImproveAdjustAllSignsToHeightGroundwith validity checks,bPositionedtracking, mesh-component-level Z adjustment.Trigger volume for OpenDRIVE signals (
2fed7136e) — Return only the first trigger volume (preserving rotation) instead of merging all volumes into one combined bounding box.Where has this been tested?
Possible Drawbacks
Traffic sign Z positions may change slightly on existing maps due to the new ground-snapping behavior.
This change is