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

Add UCesiumFlyToComponent #1227

Merged
merged 23 commits into from
Oct 1, 2023
Merged

Add UCesiumFlyToComponent #1227

merged 23 commits into from
Oct 1, 2023

Conversation

kring
Copy link
Member

@kring kring commented Sep 27, 2023

Fixes #1155

This takes the "flight" functionality that was previously in GlobeAwareDefaultPawn and makes it into a component that can be added to anything. So you can do flights with other pawns, or even fly random models around the world or whatever.

Backward compatibility was tricky, but basically Blueprints using the old flight functionality on GlobeAwareDefaultPawn should continue to work (with warnings). C++ code will need changes. DynamicPawn includes a CesiumFlyToComponent. A GlobeAwareDefaultPawn saved in previous versions will gain one when it's upgraded, but users will have to add it manually to new GlobeAwareDefaultPawns.

Other changes here:

  • Added CesiumActorComponentWithGlobeAnchor, which is just a convenience base class for ActorComponents that require that a GlobeAnchor also be attached to the same Actor. CesiumOriginShiftComponent and CesiumFlyToComponent both derive from this.
  • Renamed the various curve assets used in the flights to have more descriptive names. Redirectors should upgrade references to the old names.
  • Removed the very old FloatingPawn that has been deprecated for over two years now.

@cesium-concierge
Copy link

Thanks for the pull request @kring!

  • ✔️ Signed CLA found.

Reviewers, don't forget to make sure that:

@j9liu j9liu self-requested a review September 28, 2023 20:11
Copy link
Contributor

@j9liu j9liu left a comment

Choose a reason for hiding this comment

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

Looks great @kring ! All of my comments are doc / style related, so I can push commits for them and save you time. If you disagree with any of my changes feel free to git revert.

Otherwise, happy to merge this once #1217 is merged!

@@ -29,6 +29,8 @@ struct CESIUMRUNTIME_API FCesiumCustomVersion {
// built into the CesiumGeoreference.
OriginShiftComponent = 5,

FlyToComponent = 6,
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment for this version as well?

Suggested change
FlyToComponent = 6,
// Fly-to behavior is now an independent component rather than built into the GlobeAwareDefaultPawn.
FlyToComponent = 6,

class UCesiumGlobeAnchorComponent;

UCLASS(ClassGroup = "Cesium", Abstract)
class CESIUMRUNTIME_API UCesiumActorComponentWithGlobeAnchor
Copy link
Contributor

Choose a reason for hiding this comment

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

This may be nitpicky, but can we move "Component" to the end of the class name for consistency with the Unreal component naming schemes? Maybe something like this would be better?

Suggested change
class CESIUMRUNTIME_API UCesiumActorComponentWithGlobeAnchor
class CESIUMRUNTIME_API UCesiumGlobeAnchoredActorComponent

Copy link
Contributor

Choose a reason for hiding this comment

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

UCesiumActorWithGlobeAnchorComponent was another alternative, but I went with UCesiumGlobeAnchoredActorComponent. But feel free to disagree if you think the new names are inaccurate.

void _onFlightComplete();

UFUNCTION()
void _onFlightInterrupted();

/**
* @brief Advance the camera flight based on the given time delta.
Copy link
Contributor

Choose a reason for hiding this comment

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

_handleFlightStep and most of the variables below it are no longer used. Let's remove them

* East-South-Up.
*
* If the component is attached to an Actor that is not a Pawn, or if the Pawn
* does not have a Controller, this option is equivalent to the the "Actor"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* does not have a Controller, this option is equivalent to the the "Actor"
* does not have a Controller, this option is equivalent to the "Actor"

/**
* A curve that controls what percentage of the maximum height the Actor
* should take at a given time on the flight. This curve must be kept in the 0
* to 1 range on both axes. The {@see FlyToMaximumAltitudeCurve} controls the
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* to 1 range on both axes. The {@see FlyToMaximumAltitudeCurve} controls the
* to 1 range on both axes. The {@see MaximumHeightByDistanceCurve} controls the

@kring
Copy link
Member Author

kring commented Oct 1, 2023

All your changes look great to me. Thanks for the review and fixes @j9liu!

Base automatically changed from origin-shift-component to ue5-main October 1, 2023 21:40
@kring
Copy link
Member Author

kring commented Oct 1, 2023

I'm merging this for the v2.0 preview. Further review comments still welcome.

@kring kring merged commit 14823c7 into ue5-main Oct 1, 2023
@kring kring deleted the fly-to-component branch October 1, 2023 21:40
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.

Add UCesiumFlyToComponent
3 participants