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

Teleportation, Line Tracing, and input binding #18

Closed CesiumBen closed 2 years ago

CesiumBen commented 2 years ago

Summary

This PR pushes up the tutorial for line trace teleportation. The tutorial takes place in sunny grand canyon, with plenty of cliffs to teleport against.

Grand Canyon

Teleportation

Author checklist

New Blueprints

Teleportation > Blueprints > BP_LineTraceRenderer Teleportation > Blueprints > BP_VRPawn_Teleportation Teleportation > Blueprints > BP_VRTeleportationComponent

Testing plan

Enter the start up level, which should be set to Content > Teleportation > Teleportation. Test useing the teleport feature as described above in the summary.

List of comments

BP_LineTraceRenderer

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:

CesiumBen commented 2 years ago

Link to draft text

CesiumBen commented 2 years ago

@argallegos Thank you for your review of this and the text! I've updated all items from your suggestions. There are still some text changes I'll need to do after fixing the PR here. I'll ping you when it's in a final review state.

BP_VRTeleportComponent has the following warnings: Get Hand : Usage of 'Hand' has been deprecated. Please replace or remove it. Get Hand : Usage of 'Get Tracking Source' has been deprecated. Please use the Motion Source property instead of Hand

It's true these warnings are there. Unfortunately there is not a static "GetMotionControllerData" function which takes an FName for the parameter, they still require EControllerHand. Converting the MotionSource of a MotionControllerComponent from FName to EControllerHand seemed a little messy and verbose since branching would be needed. I wanted to keep the blueprints compact to increase confidence in viewing them, however warnings are not great either. This needs to go into draft for a short time and will probably change it soon.

CesiumBen commented 2 years ago

@xuelongmu I've tested the method of attaching another camera pass to the line trace and it's not performing as well as it probably would need to in order to be a solid solution for multiple applications. In some cases if your connection is just too slow then the tiles won't supply the needed LOD in time or at all in order to gather the correct point.

I think it would be better to take a VR Pawn correction approach where the pawn has functionality to bring itself above ground whenever needed on tick. This way under a number of circumstances the pawn will be placed in the correct location. This functionality will also be able to be easily disabled under more complicated movement types.

See videos below, the LOD loading is visible on the end of the line trace, however when I was testing the application it was common to find yourself under the terrain when teleporting to far distances.

I think the immediacy of VR movement and this feature would benefit from a custom solution over a change to the Cesium for Unreal plugin. There isn't a one size fits all approach.

https://user-images.githubusercontent.com/79323980/165826472-ac7e8438-0eab-40c9-b67a-7d844ac7679b.mp4

CesiumBen commented 2 years ago

@xuelongmu I've added in the Pawn correctional idea we discussed and it seems to be working very well. The only way I could efficiently test it was to teleport to the portions of the tile which bleed below the backface. The correction is immediate and I don't notice any frame while being underground after teleporting.

I still need to incorporate text from the tutorial into the comments on all blueprints. Then I'll take it out of draft.

TeleportCorrection

xuelongmu commented 2 years ago

Thanks @CesiumBen ! Tested in headset and it's working great, including the position correction. Have a few comments mostly related to styling:

image There's a hanging Motion Source node here. Some of the nodes also overlap with the comment titles/frames.

image The default gray comment background makes the comment titles hard to read and reduces contrast overall. IMO a dark gray or black is easier on the eyes.

image I noticed that we set the tick group in code, rather than in the class settings. Are we doing it this way so it's more explicitly visible to readers?

CesiumBen commented 2 years ago

@argallegos @xuelongmu This PR is ready for full review.

The doc is here.

@xuelongmu

The default gray comment background makes the comment titles hard to read and reduces contrast overall. IMO a dark gray or black is easier on the eyes.

I've cleaned up the comments to be darker, it's easier to manage and the contrast does help.

NEwComments

There's a hanging Motion Source node here. Some of the nodes also overlap with the comment titles/frames.

It's all cleaned up now. Also to reiterate something I mentioned above, the "MotionSource" is an FName and while a warning is thrown, I think it's adds more clutter to have to convert an FName to an EControllerHand. Unfortunately there isn't a method of GetMotionControllerData which takes an FName yet, but I'd expect it soon if they plan on phasing out EControllerHand.

I noticed that we set the tick group in code, rather than in the class settings. Are we doing it this way so it's more explicitly visible to readers?

It was originally but it's just extra clutter so I took it out and pointed to the setting in the details on the tutorial text.

Thank you!

CesiumBen commented 2 years ago

@xuelongmu @argallegos

Thanks for your comments and edits so far. All the text has been updated on the docs, and the page now reflects what the tutorial would look like. Please take a look when you get the chance.

updates to doc updates to page

LisaBosCesium commented 2 years ago

@CesiumBen I've added comments and a few minor edits to the first part of the tutorial. I need to step into another call and I know you want to start incorporating feedback to stay on schedule, so don't wait for me - looking at the intro and the overall organization were my main goals and I've done that.

I'll also make a final copy edit review after you accept all changes - can do in Prismic directly. Let me know when you're ready.

xuelongmu commented 2 years ago

Great work @CesiumBen !

LisaBosCesium commented 2 years ago

@CesiumBen I made a small number of copy edits in Prismic - looking great. You can see what I changed in the history if you want.

Please review capitalization of the captions in the embedded blueprints. Eg, "Id" should be "ID", and references to classes etc should be capped to match the labels in the blueprint itself ("Cesium camera manager" should be "Cesium Camera Manager").

Observation: Generally, this tutorial (and I assume the series) takes the approach of describing how the code works as opposed to providing numbered step-by-step instructions. For example:

This component requires some understanding of what direction to point in and how to show the line trace. To achieve this, we create a Setup function within BP_VRTeleportationComponent, where we store references to other components and values.

rather than

  1. This component requires some understanding of what direction to point in and how to show the line trace. Create a Setup function within BP_VRTeleportationComponent to store references to other components and values.

This is appropriate given that we expect readers for this series to be more experienced than readers of many of the other tutorials. In many cases they will be reading and scanning the code examples to understand the general approach as opposed to performing each step of the tutorial themselves.

But, I wanted to state this explicitly to make sure the above is accurate and that we're explicitly making this choice in the interest of our readers.

Also, for future tutorials where we take this approach, be sure to keep each section fairly short to help the reader keep track of where they are. One of the benefits of numbered steps is that it's easier for users to remember where they stopped reading when they come back from coding or otherwise move around in the tutorial - gives them a visual hook. Most of this tutorial's sections aren't very long (aside from the embedded blueprints) so it's not an issue. And the blueprints themselves provide reference points for the reader.

argallegos commented 2 years ago

To Lisa's point - even if you avoid the step-by-step approach, it still might be useful to number the headings so that users can keep track of where they are on the page. It looks like this tutorial is pretty sequential so it wouldn't be out of place. You also don't have to use the "Step X:..." auto-numbering - you could manually number the sections so it could say "1.1: Teleportation" or something.

Not requesting a change, just an idea.