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

Viewing the Globe #37

Merged
merged 9 commits into from
Sep 22, 2022
Merged

Viewing the Globe #37

merged 9 commits into from
Sep 22, 2022

Conversation

CesiumBen
Copy link
Contributor

Summary

This PR adds the dragging the globe functionality.

Author checklist

  • Tutorial for Tutorial: Viewing the globe #5
  • Has the new tutorial been integrated into the main level? Yes
  • Has a new section of the readme been added for this tutorial? Yes
  • Have you done a full self-review of the code and blueprints? Yes
  • Have you tagged a primary reviewer? Yes
  • Are there any remaining tasks to do or blocking questions to answer before we can merge this? No

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:

  • Review and run all parts of the test plan on this branch and verify it matches expectations.
  • Review the code and make sure you do not have any remaining questions or concerns. You should understand why this code change fixes the issue and agree that it's the best approach to doing so.
  • Review the automated tests and make sure there are no missing tests or edge cases.
  • When you feel this is ready bump it to a second reviewer or merge.

@CesiumBen CesiumBen self-assigned this Sep 8, 2022
@CesiumBen CesiumBen mentioned this pull request Sep 9, 2022
4 tasks
@xuelongmu
Copy link
Contributor

xuelongmu commented Sep 16, 2022

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
Copy link
Contributor Author

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.

@CesiumBen CesiumBen changed the title Dragging the Globe Viewing the Globe Sep 21, 2022
@xuelongmu
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

This looks great Ben! Merging now.

@xuelongmu xuelongmu merged commit 373ae09 into main Sep 22, 2022
@xuelongmu xuelongmu deleted the dragging-the-globe branch December 21, 2022 15:34
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.

None yet

2 participants