Closed CesiumBen closed 2 years ago
Thanks @CesiumBen, tested it out in-headset and it's working well. Some feedback:
_actionModified
bool. I don't think that the VRMovementComponent should have to keep track of the current input state of the user.Testing notes:
@xuelongmu Thanks for the review!
We're overloading the term "modify": we're using it to refer to an axis value ("Input Modified") as well as to indicate when a modifier key is held down ("Action Modified"). I suggest using "Input Value" instead of "Input Modified".
I've updated it to inputValue
Nitpick, but the MoveUpwardDownward method takes a double precision float when AxisValue is only single precision float. We should avoid the upcast. The double precision is marked by the diamond symbol:
Thank you for catching that, I've updated that.
We could call it MoveForward, MoveLeft, MoveUp, as that's what Unreal does in their default FPS character class. I think readers will know that the other direction is implied.
I've set them to familiar Unreal names.
On a class design level, I wonder if the Pawn is a better place to store the _actionModified bool. I don't think that the VRMovementComponent should have to keep track of the current input state of the user.
That's a good point, it is more obvious as to what the modifications do and easier to change them around if you wanted to. I've changed it to set the "MovementModified" in the pawn instead of on the movement component.
Acceleration and deceleration are too gradual, which can cause motion sickness. (page 5 of Oculus VR Best Practices guide)
I'm going to fine tune these
@CesiumBen Looks like a solid foundation! I added notes to the Google Doc for specific wording changes.
Some additional notes: (These are actually more general to all the VR tutorials, not just this one, so please apply where needed to the first tutorial and subsequent tutorials as well!)
@argallegos Thank you for the review!
The BlueprintUE node graphs don't show the values of variables like ground snap distance, rotation degrees, etc. For readers who are not following along with the project, I recommend adding a note with each blueprint graph (or comment on the node, if BlueprintUE supports it) indicating what these variables should be set to.
I've updated all of the comments across the project to mention the default values of those numbers.
I recommend adding the tutorial number to the name in Prismic as well (E.G. CVR 1: Movement) or similar because it will hopefully make it easier to find the document you need when more in the series are created.
Sounds good, I've updated them
For tutorial placement in the learn sidebar(Learn menu document), make sure the text is set to Heading 3. Using Heading 1 will split it off into it's whole own learning center.
Thanks for pointing this out, I've updated it.
Change the aspect ratio of all BlueprintUE iframe slices to 5:4 to remove the scroll bar. You can adjust this directly on the slice itself, under the link.
Great find! I updated all of the frame aspects on tutorial 1 and 2.
@xuelongmu @argallegos This PR is ready for re-review. Thank you!
@CesiumBen Text looks good! Let me know when the text updates are moved to Prismic and I'll give it a quick flight check (har har).
One last request, if feasible - Would be very cool to see a short video of what the player movement looks like at the end of the tutorial, before the next steps section. I think it would be helpful to the user, and good for social media. Just a clip of all the movement that you implemented over the tutorial. I can help trim/upload/format that video if you can capture footage, but no worries if it's too short notice.
@CesiumBen This looks good to me - easy to follow, good length, hits key movement concerns. I edited the document with change tracking. Mostly minor copy edit changes.
@CesiumBen Oh and I strongly second @argallegos's video suggestion.
@CesiumBen I tested this out again in VR. Made a few modifications:
Right now, the calculated movement speed is multiplied by the forward vector to the "Add Movement Input" node, but that node still only applies to input and doesn't actually affect the speed beyond a factor of 1.0. For example, if you apply a scale of 100 to the input vector, you don't go 100x faster, you just go from 0 to 1.0 immediately, even if the joystick is held down just a little bit. I don't think that was the intention, so I removed those multiplication nodes from the MoveUp/MoveLeft functions and removed the variable itself.
The inputs were also still named MoveForwardsBackwards etc., so I renamed those to match the new functions.
I felt that the velocity curve was too gentle, i.e. I wanted to go faster at high altitudes, so I tripled the MaxVelocity setting.
@xuelongmu Do you recommend we wait for Ben for review/merge these changes? Or can someone else do in his absence?
@CesiumBen Made one more change:
I saw that the input threshold for moving forward and left were 0.5. I'm assuming this was done to prevent simultaneous forward and lateral movement which would cause more motion sickness. This made the controls feel less responsive, and you couldn't gently nudge on the thumbstick to go slowly. Though using a threshold is the simpler solution for the diagonal movement problem, I didn't want to compromise on how movement felt since it's so critical to a good VR experience.
I instead used an approach where whenever the MoveForward input axis is triggered, it'll do a check to see if its absolute input value is greater than that of the MoveLeft axis. Only if the input is greater, will the character actually move forward. Vice versa for MoveLeft input axis.
@CesiumBen @LisaBosCesium Updated the blueprintUE to reflect the code changes made in my prior two comments.
Currently in the process of updating the images in Prismic to reflect renamed inputs. Will merge this once it's all updated.
The input images are updated, merging now.
Summary
This PR adds in the movement functionality for tutorial #2.
Author checklist
Tutorial for #3 #4
Has the new tutorial been integrated into the main level? Still in progress
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? Yes
[x] Add in main level and incorporate the movement and teleportation tutorials
[x] Merge in #18
[x] Add in comments for tutorial 2
Testing plan
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: