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 CesiumOriginShiftComponent #1217

Merged
merged 28 commits into from
Oct 1, 2023
Merged

Add CesiumOriginShiftComponent #1217

merged 28 commits into from
Oct 1, 2023

Conversation

kring
Copy link
Member

@kring kring commented Sep 15, 2023

Fixes #1154

This component can be attached to any Actor, and it will transition into and out of sub-levels based on the position of that Actor. It also - optionally - allows the Unreal world origin to be shifted to stay close to that Actor. It can shift the origin by either changing the CesiumGeoreference origin or by setting Unreal's OriginLocation property.

Be sure to set your Cesium3DTileset to Movable if you use the "Change Cesium Georeference" mode. Otherwise, you'll see a bad "warp" artifact while moving when Temporal Anti-Aliasing is enabled. I spent a lot of time trying to understand why this happens. I think it has something to do with a static mobility primitive's render proxy being removed and then re-added when its transform changes, and this causes the velocity of the primitive to be missing or incorrect. But I couldn't see a straightforward way to fix this, or even confirm it as the cause. In any case, the object really does move within the UE world when using this origin shift mode, so it's reasonable-ish that Movable mobility is required.

@cesium-concierge
Copy link

Thanks for the pull request @kring!

  • ✔️ Signed CLA found.

Reviewers, don't forget to make sure that:

@j9liu
Copy link
Contributor

j9liu commented Sep 15, 2023

Otherwise, you'll see a bad "warp" artifact while moving when Temporal Anti-Aliasing is enabled.

I know we briefly discussed TAA offline, but I wanted to fish around UDN to see if anyone had experienced a similar problem. I didn't see a post that was exactly the same, but I'm gleaned some hints about how TAA works.

Ghosting with animated textures:

The ghosting problems are when it becomes high frequency texture generating to much noise in the input of TAA, that also happen especially on disocclusions. To notice this noise, I invite you to look at the raw input scene color with the command: "vis SceneColor UV0"

Ghosting with moving animated meshes

TAA used for r.PostProcessAAQuality=4 have this AA_DYNAMIC_ANTIGHOST heuristic that compared was dynamic (for instance a car moving) and a static (environement) and reject accordingly. This tends to clean up this sort of rejection problem like you are having, but this requires making sure all object in the scene have correct mobility. TAA knows whether a pixel is static of dynamic based on whether it is respectively black or rendered. You can see the Velocity buffer dirrectly with "vis SceneVelocity" or "vis GBufferVelocity" commands.

There is also this regular UE forum thread TAA ghosting when moving instanced meshes. Again, not the exact same issue, but I found this snippet very interesting:

I was moving the instances individually using Update Instance Transform, but I suspected it was causing issues with how the velocity buffer was calculated for the instances, and was causing the ghosting (I very well could be wrong about this, there is very little information for this issue). So to remedy this, I spawned a scene component to act as the “mover” for the instances and attached the ISM components to it. This allows you to move the scene component (I used add local offset) instead of the instances directly, which will correctly calculate the velocity buffers for the instances themselves.

Anyway, it would probably be more effort than it is worth to come up with some workaround so that Static Cesium3DTilesets don't ghost. But the more knowledge the merrier?

@kring kring added this to the v2.0 milestone Sep 18, 2023
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.

@kring I have one concern about the interaction of "Change World Origin Location" and the DynamicPawn flights, which I wrote about in the relevant comment. Otherwise, everything else is either a doc comment or a question for clarification. Overall, looks great!

Also, it is very funny to fly around with the warp effect. Reminds me of the hyperspace effects from Star Wars 🚀

CHANGES.md Outdated Show resolved Hide resolved
Source/CesiumRuntime/Public/CesiumSubLevelComponent.h Outdated Show resolved Hide resolved
Source/CesiumRuntime/Public/CesiumGeoreference.h Outdated Show resolved Hide resolved
@@ -401,15 +404,13 @@ void ACesiumGeoreference::Tick(float DeltaTime) {
#if WITH_EDITOR
_showSubLevelLoadRadii();
Copy link
Contributor

Choose a reason for hiding this comment

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

I was going to redundantly comment on this, but remembered we already discussed it in the initial sublevels PR. So I wrote up the issue #1230 for my own memory's sake :)

* otherwise, as long as they respond correctly to Unreal's ApplyWorldOffset
* method.
*/
ChangeWorldOriginLocation
Copy link
Contributor

Choose a reason for hiding this comment

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

So just for fun, I tried setting the mode to ChangeWorldOriginLocation on the DynamicPawn in 04_CesiumSublevels. And then I took flights around the Earth... and it deviates from the intended flight path quite a bit. This is the view when you press '2' to fly to the Grand Canyon.

wee

This happens both here and in fly-to-component so it's not related to the flight behavior. I'm not surprised that this breaks but if it is expected to, then we probably should document this. If not... 😬

Copy link
Member Author

Choose a reason for hiding this comment

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

This happens because a globe anchored Actor's ECEF position is only a function of the CesiumGeoreference origin (not its transform) and the Actor position itself. Changing the World Origin changes the Actor position, so it inadvertently affects the longitude/latitude/height.

It should be possible to modify CesiumGeoreference to make it take the WorldOrigin into account in its ECEF<->Unreal transformations. But it will be a hassle, and I'm not sure it's worth it. I think for now I will just remove this ChangeWorldOriginLocation option entirely. We can certainly re-add it in the future if we see value in doing so.

Source/CesiumRuntime/Public/CesiumOriginShiftComponent.h Outdated Show resolved Hide resolved
Source/CesiumRuntime/Public/CesiumOriginShiftComponent.h Outdated Show resolved Hide resolved
@@ -303,7 +294,8 @@ void UCesiumSubLevelSwitcherComponent::_updateSubLevelStateGame() {

// Double-check that we're not actively trying to unload this level
// already. If we are, wait longer.
if (IsValid(pStreaming) && pStreaming->ShouldBeLoaded()) {
if ((IsValid(pStreaming) && pStreaming->ShouldBeLoaded()) ||
this->_pTarget.Get()->GetWorldAsset().IsNull()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just checking, what does it mean if the target level's world asset is null? Do we want the current sublevel to have a null world asset?

Copy link
Member Author

Choose a reason for hiding this comment

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

It means that we have an ALevelInstance that doesn't actually have any level associated with it at all. We can still switch to such a sub-level, it just doesn't have any content. There's a comment on a similar GetWorldAsset check above.

@j9liu
Copy link
Contributor

j9liu commented Sep 28, 2023

Pushed a commit for the typos / changelog nitpicking so you wouldn't have to worry about it.

Base automatically changed from globe-anchor-improvements to ue5-main September 28, 2023 22:34
@j9liu j9liu mentioned this pull request Sep 29, 2023
@kring kring merged commit 7c00400 into ue5-main Oct 1, 2023
22 of 33 checks passed
@kring kring deleted the origin-shift-component branch 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.

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 UCesiumOriginShiftComponent
3 participants