AltspaceVR / AltspaceSDK

Software Development Kit for AltspaceVR
https://developer.altvr.com/
MIT License
161 stars 73 forks source link

Initial commit of TrackJoints behavior to request feedback and guidance. #163

Closed evhan55 closed 7 years ago

evhan55 commented 7 years ago

I haven't used git in a while, so I am testing this initial PR, thank you!

evhan55 commented 7 years ago

Here is a video of this behavior in action, through a synced object: https://www.youtube.com/watch?v=sJUM4_aF3mI

stevenvergenz commented 7 years ago

The PR seems to have worked! You say it's an "initial" commit, so let us know when the feature is done, we're excited to check it out.

evhan55 commented 7 years ago

Thanks @stevenvergenz ! I guess my next question would be:

So far, I had the behavior inline in my app, so I haven't tested it as a behavior included in the altspace utilities yet. In order to test this new behavior (and the options I'd like to add to it), do I need to compile the repo somehow?

The feature is 'done' to a point so far, so it could be merged and tested already without the configuration options I need to add to it.

brianpeiris commented 7 years ago

Up to you if you want to expand on it now or commit it without the configuration options. That's ok with me, it is useful as it is. If you do intend to expand on it in a later PR, one concern might be that you'd have to introduce breaking changes to the interface, in order to add the configuration options (although I guess you could implement it in a way that doesn't change the interface)

Either way, adding an example along with the behavior would definitely be nice.

evhan55 commented 7 years ago

@brianpeiris Thank you for the feedback! Ah, yes I see the conflict about submitting now without options and then breaking the interface later. Is there a way for me to test the behavior locally as a registered behavior in the altspace namespace?

Well, the other thing is I guess I am not sure if joint cube size, joints and scale are the final options that devs would need, and I was hoping to get other devs to test it and give whatever feedback or requests they'd have. But that would require changing the interface possibly multiple times and requiring multiple merges, which is more overhead for the main developers.

So it's a tiny bit of a catch-22, and I'm happy to proceed however you prefer! Options: 1) I test locally with a few config options, if possible for me to add the behavior to the altspace namespace 2) We merge as is, and get feedback from devs for a bit

It might be beneficial for me to learn how to do option 1, I guess?

Thanks again!

EDIT: Where is a good place to put an example? In the file itself, up top?

brianpeiris commented 7 years ago

It's definitely best to test locally. You should just be able to run npm install and then npm run build to re-build the altspace.js file in dist/. You should also run npm run doc to check the generated documentation under doc/. You can stick a simple example under examples/joint-collision.html (or whatever you decide to call it).

evhan55 commented 7 years ago

@brianpeiris Ok great, I will try to test locally using npm. I will also check the generated docs, and I will make an example file. Will it be possible to update this PR with new and edited files, or will it require a new PR? (Sorry for all the questions! Been three years since I used git). :) Thank you!

brianpeiris commented 7 years ago

No worries. Yes, any changes you push to your master branch will be reflected here, so no need for a new PR

evhan55 commented 7 years ago

Ah, it diffs a whole branch, got it. Thanks! I will keep the updates coming.

evhan55 commented 7 years ago

Hello @stevenvergenz and @brianpeiris !

I tried to test my new 'joint tracking' behavior locally today and have run into a few issues:

1) The version of gulp gave me issues and I had to uninstall and reinstall it? I got some error like the one here (This may have been because I tried running npm install more than once, maybe?) 2) Some created files like tests/src/sync.js are generated with spaces instead of tabs or vice versa, so Travis fails

Thank you!

brianpeiris commented 7 years ago

Hey @evhan55. Yeah, the build process is in need of update, but it seems you've managed to get it working anyway. If you could, please exclude the bulit files and the changes to tests/src/sync.js from this PR, so that it's easier to review and merge. Generally PRs should only contains source code. The build and publish scripts will take care of generating the other files when we merge your changes.

evhan55 commented 7 years ago

@brianpeiris Great, sounds good! I will exclude anything that is not in src then?

evhan55 commented 7 years ago

Oh hm I guess it needs the gulpfile too...

brianpeiris commented 7 years ago

Yup, you should just need the behavior source and the gulpfile. Thanks!

evhan55 commented 7 years ago

@brianpeiris

So far for the PR:

Questions/Misc.:

evhan55 commented 7 years ago

@brianpeiris

This PR is ready for review/merge on my end. No scaling handled yet, but I would rather wait for dev testing.

Thank you!

Docs:

screen shot 2016-12-01 at 1 50 39 am
brianpeiris commented 7 years ago

Thanks @evhan55, the PR looks a lot cleaner now. No need to include the docs in the PR, and yes I think it will be fine to leave the scaling implementation out for now. I've left a few comments for you.

evhan55 commented 7 years ago

Thank you for the comments! Will re-view, re-test, re-generate docs and ping you again @brianpeiris !

brianpeiris commented 7 years ago

:+1: Merging this now. Will follow up with another PR for the fixes.