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

networked-hand-controls, with gestures; also, many examples updates #355

Merged
merged 20 commits into from
Aug 9, 2022

Conversation

kylebakerio
Copy link
Member

Based on the A-Frame built-in hand-controls component. Heavily refactored for readability, performance, and functionality. Will likely make an upstream pull request to A-Frame based on these updates in the near future.

Of course, added the ability to integrate with NAF as well. Gesture updates are stored as a schema prop, and so are easy to propagate via NAF. tracked controller demo has been updated to use this component, as well. dist file has been compiled with the new component, which is necessary to be able to use the file without duplicating it.

@kylebakerio
Copy link
Member Author

To test locally, I recommend using the webxr chrome extension. Also, though, feel free to ping me to try it out.

@kylebakerio kylebakerio changed the title Update examples add networked-hand-controls that broadcast/track gestures Aug 8, 2022
@kylebakerio
Copy link
Member Author

kylebakerio commented Aug 8, 2022

failing test: "eslint": "^6.6.0", in package.json, I presume that's what we're using? eslint added support for conditional chaning in 7.5

Copy link
Member

@vincentfretin vincentfretin left a comment

Choose a reason for hiding this comment

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

That's really cool! I tested locally with the PC and the Quest by defining the certificate for https in server/easyrtc-server.js. I couldn't make the WebXR Chrome extension work.

Last time I resaved all the examples from vscode with the prettier config in the repo. Can you please open the html file in vscode and resave?

For conditional chaining, you can try updating eslint, be sure there is no error when you run npm run lint. Otherwise remove your usage of conditional chaining and replace it by a if.

Please add some documentation about the component in the README and address my comments and it will be good to merge. Thanks for this contribution!

src/components/networked-hand-controls.js Outdated Show resolved Hide resolved
src/components/networked-hand-controls.js Outdated Show resolved Hide resolved
src/components/networked-hand-controls.js Outdated Show resolved Hide resolved
src/components/networked-hand-controls.js Outdated Show resolved Hide resolved
@vincentfretin
Copy link
Member

Capture d’écran de 2022-08-08 11-51-44
Capture d’écran de 2022-08-08 11-51-51

@vincentfretin vincentfretin mentioned this pull request Aug 8, 2022
…g tracked controllers; also updated readme list of examples to include video, and to separate updated/non-updated examples, and updated audio glitch link; updated basic example by pulling relevant sections from the previous version of tracked-controlers.html; simplified color tracking in basic example; vastly simplified tracked-controllers.html to be a minimal example that only demonstrates how to add tracked controllers--so, moved all basic comments to the basic example, and removed the nametag syncing functionality; updated networked-hand-controls to respect color specified in schema, which has been broken in hand-controls component; updated networked-hand-controls to have a _much_ simpler user interface--it now updates the NAF template cache in a different way that avoids awkward workarounds within the HTML; added separate nametag example, and added random-color-update button;
@kylebakerio kylebakerio changed the title add networked-hand-controls that broadcast/track gestures networked-hand-controls, with gestures; also, many examples updates Aug 9, 2022
@kylebakerio
Copy link
Member Author

Sorry for the scope creep... As I went to write out how to implement the new component, I was dissatisfied and realized the interface needed to be improved. In the process, I went ahead and cleaned up some other stuff in the examples section as well.

  • added requested readme updates, with documentation and guide on adding tracked controllers
  • updated readme list of examples to include video, and to separate updated/non-updated examples, and updated audio glitch link
  • updated basic example by pulling relevant sections from the previous version of tracked-controlers.html
  • simplified color tracking in basic example
  • vastly simplified tracked-controllers.html to be a minimal example that only demonstrates how to add tracked controllers--so, moved all basic comments to the basic example, and removed the nametag syncing functionality
  • updated networked-hand-controls to respect color specified in schema, which has been broken in hand-controls component
  • updated networked-hand-controls to have a much simpler user interface--it now updates the NAF template cache in a different way that avoids awkward workarounds within the HTML
  • added separate nametag example, and added random-color-update button;

@vincentfretin
Copy link
Member

Most of the changes are fine. I added comments directly on commit 28dc899 but it doesn't show up here in the PR.

@vincentfretin
Copy link
Member

You should remove the player-info component in basic.html, it's not used.

@vincentfretin
Copy link
Member

Please add the new nametag example in index.html

Copy link
Member

@vincentfretin vincentfretin left a comment

Choose a reason for hiding this comment

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

I think it's ready to be merged now. Or do you still need to work on it?

@kylebakerio
Copy link
Member Author

I agree, ready when you are.

@vincentfretin vincentfretin merged commit f71377a into master Aug 9, 2022
@kylebakerio
Copy link
Member Author

remember to go update the naf-examples glitch! cheers, thanks for reviewing all that.

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