CesiumGS / cesium-unreal-vr-tutorial

Unreal Engine project, assets, and code used in the Cesium for Unreal VR Tutorial Series
Apache License 2.0
34 stars 6 forks source link

Viewing the Globe #37

Closed CesiumBen closed 2 years ago

CesiumBen commented 2 years ago

Summary

This PR adds the dragging the globe functionality.

Author checklist

Testing plan

Open up the dragging the globe level and grab the earth with either grip trigger. Test moving the head around as well.

In the main level you can return to normal view by teleporting on the Earth by pointing to teleport as you normally would on the ground. While in the main level you can also enter the "Earth View" mode by flying high enough.

Reminders for reviewers

Thank you for taking the time to review this PR. By approving a PR you are taking as much responsibility for these changes as the author. Please keep these points in mind throughout the review process:

xuelongmu commented 2 years ago

Thanks Ben! Sorry for the delayed review on this. I noticed a few things while testing in standalone:

1) Loading the terrain took a long time, about one minute or so before the continents appeared and I wasn't just looking at a blue ball. Might be worth making a note of so users don't get confused (subsequent loads were much faster, indicating this was a matter of caching downloaded tiles) 2) Unfortunately, having an object fixed to the center of your vision felt nauseating in VR. I think it'd be less nauseating if it was in front of you but persisted in the same world-space location, so it's acting more like an object in the real world. 3) Picking up the globe with your controller was also a little too jittery for a comfortable experience, and I started feeling nauseous there too. I think we maybe we could have "dragging" simply rotate the globe in-place (like the joystick inputs are wired to do at the moment) instead of treating the globe as something you can pick up and move around.

CesiumBen commented 2 years ago

Thank you @xuelongmu!

Great points. The dragging functionality has been removed for now and will be added in after some fine tuning so that it runs smoothly. The Earth viewer now positions the earth in front of the player while transitioning, but after that you can move freely around the globe or rotate it with thumbstick input.

Ready for re-review.

xuelongmu commented 2 years ago

Thanks @CesiumBen! This looks great overall. Few minor things:

image

On the BP_VRPawn_EarthViewer, should SetEarthViewCoordinates automatically call PositionEarthInView? This would be a cleaner API as the developer wouldn't need to worry about calling the second function every time. Unless there's a situation where you're setting coordinates but delaying updating the view.

image

Similar feedback, it seems like the EarthViewer component should be able to update its own position on Tick rather than have the developer remember to put it on its owning actor's Tick. This comment in the SetEarthView function would also make a bit more sense:

image

as after reading that comment, I was looking for a Tick function on the component but that functionality is handled on the tick of the owning actor.

Finally, quick naming thing - BP_EarthViewer should have Component appended to the end.

CesiumBen commented 2 years ago

Thanks @xuelongmu, updates below


BP_EarthViewer should have Component appended to the end.

Updated, thanks for catching that

BP_EarthViewerComponent


On the BP_VRPawn_EarthViewer, should SetEarthViewCoordinates automatically call PositionEarthInView? This would be a cleaner API as the developer wouldn't need to worry about calling the second function every time. Unless there's a situation where you're setting coordinates but delaying updating the view.

I've updated the "SetEarthViewCoordinates" function to also set the Earth in view, moving that functionality from the input graph. I also added in a check to only set the Earth in view if you are in Earth view, so that if you're in normal view you can update safely update your coordinates without any Earth visualization.

UpdatedInputOnPawn

CoordinatesSetEarthInView


Similar feedback, it seems like the EarthViewer component should be able to update its own position on Tick rather than have the developer remember to put it on its owning actor's Tick. This comment in the SetEarthView function would also make a bit more sense:

I've moved it to the tick of its own component, earlier control of that tick was needed because of the grabbing which is no longer applied. In the future we'd want to disable tick of the component dynamically.

EarthViewerTick

xuelongmu commented 2 years ago

This looks great Ben! Merging now.