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

[docs] Updates to aincraft tutorial and code #5380

Merged
merged 4 commits into from
Nov 14, 2023

Conversation

JonathanHarford
Copy link
Contributor

Description:

The tutorial (and corresponding html file) was partially converted to use aframe-blink-controls instead of aframe-teleport-controls and wouldn't run. With that fixed, the scene had a problem where blocks would spawn right on the controller instead of at the end of the raycaster.

Changes proposed:

  • Update aincraft code to work with aframe-blink-controls
  • Update the tutorial to describe the correct use of aframe-blink-controls.
  • Update aincraft right hand to give its raycaster a 0.5m buffer
  • Update the tutorial to note the buffer.

With these changes, I was able to finish the tutorial.

I can't tell if this is reasonably necessary or a workaround for a bug,
but these changes got the scene to work for me.

<a-entity id="teleHand" hand-controls="hand: left" teleport-controls></a-entity>
<a-entity id="blockHand" hand-controls="hand: right"></a-entity>
<a-entity id="player">
Copy link
Member

Choose a reason for hiding this comment

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

Removing aframe script intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, because I wanted to only show the code that was relevant to the section. I hadn't realized the code blocks were made to be executable on their own!

@@ -425,16 +427,9 @@ that attaches the clicking laser to VR tracked controllers. Like the
`laser-controls` component. This time to the right hand:

```html
<script src="https://aframe.io/releases/1.4.0/aframe.min.js"></script>
Copy link
Member

Choose a reason for hiding this comment

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

Keep aframe script so example is copy pastable

ground, but we can specify with `collisionEntities` to teleport on the blocks
*and* the ground using selectors. These properties are part of the API that the
`blink-controls` component was created with:
[blink-controls]: https://github.com/jure/aframe-blink-controls
Copy link
Member

Choose a reason for hiding this comment

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

These component references have been moved from the top to the bottom. Any reason? The less changes the easier to review a PR


<!-- Camera -->
<a-camera>
<!-- Uncomment the line below to place blocks on desktop or mobile -->
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be commented out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without that commented out, blocks were getting placed near the controller instead of a distance. This is on the Meta Quest 2 browser.

@dmarcos
Copy link
Member

dmarcos commented Nov 6, 2023

Thanks. I left some comments.

@JonathanHarford
Copy link
Contributor Author

I'm not super experienced with pull requests, so let me know if there's more I should do. I am not clear on the rationale for making the code blocks copy/pasteable, since the blocks are not runnable without a-scene, but I did undo some of the changes in order to make the diff easier.

@dmarcos
Copy link
Member

dmarcos commented Nov 14, 2023

I'm not super experienced with pull requests, so let me know if there's more I should do. I am not clear on the rationale for making the code blocks copy/pasteable, since the blocks are not runnable without a-scene, but I did undo some of the changes in order to make the diff easier.

You did great! thanks so much. Congrats on your first contribution 🎖️

@dmarcos dmarcos merged commit 552054a into aframevr:master Nov 14, 2023
1 check passed
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.

2 participants